Bug 985827. Make Navigator::DoNewResolve not double-create objects no matter what JS is doing. r=khuey

dom/base/Navigator.cpp

@@ -142,6 +142,7 @@ NS_IMPL_CYCLE_COLLECTION_CLASS(Navigator)
 NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(Navigator)
   tmp->Invalidate();
   NS_IMPL_CYCLE_COLLECTION_UNLINK(mWindow)
+  NS_IMPL_CYCLE_COLLECTION_UNLINK(mCachedResolveResults)
   NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER
 NS_IMPL_CYCLE_COLLECTION_UNLINK_END
 
@@ -173,6 +174,7 @@ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(Navigator)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mTimeManager)
 
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mWindow)
+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mCachedResolveResults)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
 
@@ -1852,9 +1854,26 @@ Navigator::DoNewResolve(JSContext* aCx, JS::Handle<JSObject*> aObject,
         }
       }
 
-      domObject = construct(aCx, naviObj);
-      if (!domObject) {
-        return Throw(aCx, NS_ERROR_FAILURE);
+      nsISupports* existingObject = mCachedResolveResults.GetWeak(name);
+      if (existingObject) {
+        // We know all of our WebIDL objects here are wrappercached, so just go
+        // ahead and WrapObject() them.  We can't use WrapNewBindingObject,
+        // because we don't have the concrete type.
+        JS::Rooted<JS::Value> wrapped(aCx);
+        if (!dom::WrapObject(aCx, naviObj, existingObject, &wrapped)) {
+          return false;
+        }
+        domObject = &wrapped.toObject();
+      } else {
+        domObject = construct(aCx, naviObj);
+        if (!domObject) {
+          return Throw(aCx, NS_ERROR_FAILURE);
+        }
+
+        // Store the value in our cache
+        nsISupports* native = UnwrapDOMObjectToISupports(domObject);
+        MOZ_ASSERT(native);
+        mCachedResolveResults.Put(name, native);
       }
     }
 
@@ -1871,32 +1890,48 @@ Navigator::DoNewResolve(JSContext* aCx, JS::Handle<JSObject*> aObject,
 
   nsresult rv = NS_OK;
 
-  nsCOMPtr<nsISupports> native(do_CreateInstance(name_struct->mCID, &rv));
-  if (NS_FAILED(rv)) {
-    return Throw(aCx, rv);
-  }
-
-  JS::Rooted<JS::Value> prop_val(aCx, JS::UndefinedValue()); // Property value.
-
-  nsCOMPtr<nsIDOMGlobalPropertyInitializer> gpi(do_QueryInterface(native));
-
-  if (gpi) {
-    if (!mWindow) {
-      return Throw(aCx, NS_ERROR_UNEXPECTED);
-    }
-
-    rv = gpi->Init(mWindow, &prop_val);
+  nsCOMPtr<nsISupports> native;
+  bool hadCachedNative = mCachedResolveResults.Get(name, getter_AddRefs(native));
+  bool okToUseNative;
+  JS::Rooted<JS::Value> prop_val(aCx);
+  if (hadCachedNative) {
+    okToUseNative = true;
+  } else {
+    native = do_CreateInstance(name_struct->mCID, &rv);
     if (NS_FAILED(rv)) {
       return Throw(aCx, rv);
     }
+
+    JS::Rooted<JS::Value> prop_val(aCx, JS::UndefinedValue()); // Property value.
+
+    nsCOMPtr<nsIDOMGlobalPropertyInitializer> gpi(do_QueryInterface(native));
+
+    if (gpi) {
+      if (!mWindow) {
+        return Throw(aCx, NS_ERROR_UNEXPECTED);
+      }
+
+      rv = gpi->Init(mWindow, &prop_val);
+      if (NS_FAILED(rv)) {
+        return Throw(aCx, rv);
+      }
+    }
+
+    okToUseNative = !prop_val.isObjectOrNull();
   }
 
-  if (JSVAL_IS_PRIMITIVE(prop_val) && !JSVAL_IS_NULL(prop_val)) {
+  if (okToUseNative) {
     rv = nsContentUtils::WrapNative(aCx, aObject, native, &prop_val);
 
     if (NS_FAILED(rv)) {
       return Throw(aCx, rv);
     }
+
+    // Now that we know we managed to wrap this thing properly, go ahead and
+    // cache it as needed.
+    if (!hadCachedNative) {
+      mCachedResolveResults.Put(name, native);
+    }
   }
 
   if (!JS_WrapValue(aCx, &prop_val)) {

dom/base/Navigator.h

@@ -14,6 +14,8 @@
 #include "nsIMozNavigatorNetwork.h"
 #include "nsAutoPtr.h"
 #include "nsWrapperCache.h"
+#include "nsHashKeys.h"
+#include "nsInterfaceHashtable.h"
 #include "nsString.h"
 #include "nsTArray.h"
 
@@ -348,6 +350,13 @@ private:
   nsTArray<nsRefPtr<nsDOMDeviceStorage> > mDeviceStorageStores;
   nsRefPtr<time::TimeManager> mTimeManager;
   nsCOMPtr<nsPIDOMWindow> mWindow;
+
+  // Hashtable for saving cached objects newresolve created, so we don't create
+  // the object twice if asked for it twice, whether due to use of "delete" or
+  // due to Xrays.  We could probably use a nsJSThingHashtable here, but then
+  // we'd need to figure out exactly how to trace that, and that seems to be
+  // rocket science.  :(
+  nsInterfaceHashtable<nsStringHashKey, nsISupports> mCachedResolveResults;
 };
 
 } // namespace dom

dom/base/test/chrome.ini

@@ -7,3 +7,4 @@ support-files =
 [test_domrequesthelper.xul]
 [test_url.xul]
 [test_console.xul]
+[test_navigator_resolve_identity_xrays.xul]

dom/base/test/mochitest.ini

@@ -40,6 +40,7 @@ support-files =
 [test_messageChannel_transferable.html]
 [test_messageChannel_unshipped.html]
 [test_named_frames.html]
+[test_navigator_resolve_identity.html]
 [test_nondomexception.html]
 [test_openDialogChromeOnly.html]
 [test_postMessage_solidus.html]

dom/base/test/test_navigator_resolve_identity.html

@@ -0,0 +1,40 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=985827
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test for Bug 985827</title>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+  <script type="application/javascript">
+
+  /** Test for Bug 985827 **/
+
+  // Test WebIDL NavigatorProperty objects
+  var x = navigator.mozContacts;
+  is(typeof x, "object", "Should have a mozContacts object");
+  delete navigator.mozContacts;
+  var y = navigator.mozContacts;
+  is(x, y, "Should have gotten the same mozContacts object again");
+
+  // Test Javascript-navigator-property objects
+  x = navigator.mozApps;
+  is(typeof x, "object", "Should have a mozApps object");
+  delete navigator.mozApps;
+  y = navigator.mozApps;
+  is(x, y, "Should have gotten the same mozApps object again");
+
+  </script>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=985827">Mozilla Bug 985827</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+
+</div>
+<pre id="test">
+</pre>
+</body>
+</html>

dom/base/test/test_navigator_resolve_identity_xrays.xul

@@ -0,0 +1,44 @@
+<?xml version="1.0"?>
+<?xml-stylesheet type="text/css" href="chrome://global/skin"?>
+<?xml-stylesheet type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css"?>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=985827
+-->
+<window title="Mozilla Bug 985827"
+        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
+  <script type="application/javascript"
+          src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"/>
+
+  <iframe id="t"></iframe>
+
+  <!-- test results are displayed in the html:body -->
+  <body xmlns="http://www.w3.org/1999/xhtml">
+  <a href="https://bugzilla.mozilla.org/show_bug.cgi?id=985827"
+     target="_blank">Mozilla Bug 985827</a>
+  </body>
+
+  <!-- test code goes here -->
+  <script type="application/javascript">
+  <![CDATA[
+  /** Test for Bug 985827 **/
+
+  SimpleTest.waitForExplicitFinish();
+  addLoadEvent(function() {
+    var nav = $("t").contentWindow.navigator;
+    ok(Components.utils.isXrayWrapper(nav), "Should have an Xray here");
+
+    // Test WebIDL NavigatorProperty objects
+    is(typeof nav.mozContacts, "object", "Should have a mozContacts object");
+    is(nav.mozContacts, nav.mozContacts,
+       "Should have gotten the same mozContacts object again");
+
+    // Test Javascript-navigator-property objects
+    is(typeof nav.mozApps, "object", "Should have a mozApps object");
+    is(nav.mozApps, nav.mozApps,
+       "Should have gotten the same mozApps object again");
+
+    SimpleTest.finish();
+  });
+  ]]>
+  </script>
+</window>