Bug 1022773 - Return value rooting for dom/, r=bz, a=abillings
☠☠ backed out by ee727fe51f06 ☠ ☠
authorSteve Fink <sfink@mozilla.com>
Wed, 25 Jun 2014 15:35:36 -0700
changeset 207540 127429f7dd1961f41b26a223ed6fe1badfe38227
parent 207539 60e8b99055e9da53abf469ece518f6e61af48008
child 207541 2505eb1fd336cb4232e5602788970953facc56e4
push id3741
push userasasaki@mozilla.com
push dateMon, 21 Jul 2014 20:25:18 +0000
treeherdermozilla-beta@4d6f46f5af68 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbz, abillings
bugs1022773
milestone32.0a2
Bug 1022773 - Return value rooting for dom/, r=bz, a=abillings
dom/base/nsJSEnvironment.cpp
dom/bindings/Codegen.py
dom/devicestorage/DeviceStorageRequestChild.cpp
dom/devicestorage/nsDeviceStorage.cpp
dom/devicestorage/nsDeviceStorage.h
--- a/dom/base/nsJSEnvironment.cpp
+++ b/dom/base/nsJSEnvironment.cpp
@@ -2812,33 +2812,43 @@ NS_DOMReadStructuredClone(JSContext* cx,
     uint32_t width, height;
     JS::Rooted<JS::Value> dataArray(cx);
     if (!JS_ReadUint32Pair(reader, &width, &height) ||
         !JS_ReadTypedArray(reader, &dataArray)) {
       return nullptr;
     }
     MOZ_ASSERT(dataArray.isObject());
 
-    // Construct the ImageData.
-    nsRefPtr<ImageData> imageData = new ImageData(width, height,
-                                                  dataArray.toObject());
-    // Wrap it in a JS::Value.
-    return imageData->WrapObject(cx);
+    // Protect the result from a moving GC in ~nsRefPtr.
+    JS::Rooted<JSObject*> result(cx);
+    {
+      // Construct the ImageData.
+      nsRefPtr<ImageData> imageData = new ImageData(width, height,
+                                                    dataArray.toObject());
+      // Wrap it in a JS::Value.
+      result = imageData->WrapObject(cx);
+    }
+    return result;
   } else if (tag == SCTAG_DOM_WEBCRYPTO_KEY) {
     nsIGlobalObject *global = xpc::GetNativeForGlobal(JS::CurrentGlobalOrNull(cx));
     if (!global) {
       return nullptr;
     }
 
-    nsRefPtr<Key> key = new Key(global);
-    if (!key->ReadStructuredClone(reader)) {
-      return nullptr;
+    // Prevent the return value from being trashed by a GC during ~nsRefPtr.
+    JS::Rooted<JSObject*> result(cx);
+    {
+      nsRefPtr<Key> key = new Key(global);
+      if (!key->ReadStructuredClone(reader)) {
+        result = nullptr;
+      } else {
+        result = key->WrapObject(cx);
+      }
     }
-
-    return key->WrapObject(cx);
+    return result;
   }
 
   // Don't know what this is. Bail.
   xpc::Throw(cx, NS_ERROR_DOM_DATA_CLONE_ERR);
   return nullptr;
 }
 
 bool
--- a/dom/bindings/Codegen.py
+++ b/dom/bindings/Codegen.py
@@ -1595,27 +1595,29 @@ class CGConstructNavigatorObject(CGAbstr
                             "on navigator. See bug 856820.")
         return fill(
             """
             GlobalObject global(aCx, aObj);
             if (global.Failed()) {
               return nullptr;
             }
             ErrorResult rv;
-            nsRefPtr<mozilla::dom::${descriptorName}> result = ConstructNavigatorObjectHelper(aCx, global, rv);
-            rv.WouldReportJSException();
-            if (rv.Failed()) {
-              ThrowMethodFailedWithDetails(aCx, rv, "${descriptorName}", "navigatorConstructor");
-              return nullptr;
-            }
             JS::Rooted<JS::Value> v(aCx);
-            if (!WrapNewBindingObject(aCx, result, &v)) {
-              //XXX Assertion disabled for now, see bug 991271.
-              MOZ_ASSERT(true || JS_IsExceptionPending(aCx));
-              return nullptr;
+            {  // Scope to make sure |result| goes out of scope while |v| is rooted
+              nsRefPtr<mozilla::dom::${descriptorName}> result = ConstructNavigatorObjectHelper(aCx, global, rv);
+              rv.WouldReportJSException();
+              if (rv.Failed()) {
+                ThrowMethodFailedWithDetails(aCx, rv, "${descriptorName}", "navigatorConstructor");
+                return nullptr;
+              }
+              if (!WrapNewBindingObject(aCx, result, &v)) {
+                //XXX Assertion disabled for now, see bug 991271.
+                MOZ_ASSERT(true || JS_IsExceptionPending(aCx));
+                return nullptr;
+              }
             }
             return &v.toObject();
             """,
             descriptorName=self.descriptor.name)
 
 
 class CGClassConstructHookHolder(CGGeneric):
     def __init__(self, descriptor):
--- a/dom/devicestorage/DeviceStorageRequestChild.cpp
+++ b/dom/devicestorage/DeviceStorageRequestChild.cpp
@@ -67,31 +67,31 @@ DeviceStorageRequestChild::
       break;
     }
 
     case DeviceStorageResponseValue::TSuccessResponse:
     {
       nsString fullPath;
       mDSFile->GetFullPath(fullPath);
       AutoJSContext cx;
-      JS::Rooted<JS::Value> result(cx,
-        StringToJsval(mRequest->GetOwner(), fullPath));
+      JS::Rooted<JS::Value> result(cx);
+      StringToJsval(mRequest->GetOwner(), fullPath, &result);
       mRequest->FireSuccess(result);
       break;
     }
 
     case DeviceStorageResponseValue::TFileDescriptorResponse:
     {
       FileDescriptorResponse r = aValue;
 
       nsString fullPath;
       mDSFile->GetFullPath(fullPath);
       AutoJSContext cx;
-      JS::Rooted<JS::Value> result(cx,
-        StringToJsval(mRequest->GetOwner(), fullPath));
+      JS::Rooted<JS::Value> result(cx);
+      StringToJsval(mRequest->GetOwner(), fullPath, &result);
 
       mDSFileDescriptor->mDSFile = mDSFile;
       mDSFileDescriptor->mFileDescriptor = r.fileDescriptor();
       mRequest->FireSuccess(result);
       break;
     }
 
     case DeviceStorageResponseValue::TBlobResponse:
@@ -125,58 +125,58 @@ DeviceStorageRequestChild::
       mRequest->FireSuccess(result);
       break;
     }
 
     case DeviceStorageResponseValue::TAvailableStorageResponse:
     {
       AvailableStorageResponse r = aValue;
       AutoJSContext cx;
-      JS::Rooted<JS::Value> result(
-        cx, StringToJsval(mRequest->GetOwner(), r.mountState()));
+      JS::Rooted<JS::Value> result(cx);
+      StringToJsval(mRequest->GetOwner(), r.mountState(), &result);
       mRequest->FireSuccess(result);
       break;
     }
 
     case DeviceStorageResponseValue::TStorageStatusResponse:
     {
       StorageStatusResponse r = aValue;
       AutoJSContext cx;
-      JS::Rooted<JS::Value> result(
-        cx, StringToJsval(mRequest->GetOwner(), r.storageStatus()));
+      JS::Rooted<JS::Value> result(cx);
+      StringToJsval(mRequest->GetOwner(), r.storageStatus(), &result);
       mRequest->FireSuccess(result);
       break;
     }
 
     case DeviceStorageResponseValue::TFormatStorageResponse:
     {
       FormatStorageResponse r = aValue;
       AutoJSContext cx;
-      JS::Rooted<JS::Value> result(
-        cx, StringToJsval(mRequest->GetOwner(), r.mountState()));
+      JS::Rooted<JS::Value> result(cx);
+      StringToJsval(mRequest->GetOwner(), r.mountState(), &result);
       mRequest->FireSuccess(result);
       break;
     }
 
     case DeviceStorageResponseValue::TMountStorageResponse:
     {
       MountStorageResponse r = aValue;
       AutoJSContext cx;
-      JS::Rooted<JS::Value> result(
-        cx, StringToJsval(mRequest->GetOwner(), r.storageStatus()));
+      JS::Rooted<JS::Value> result(cx);
+      StringToJsval(mRequest->GetOwner(), r.storageStatus(), &result);
       mRequest->FireSuccess(result);
       break;
     }
 
     case DeviceStorageResponseValue::TUnmountStorageResponse:
     {
       UnmountStorageResponse r = aValue;
       AutoJSContext cx;
-      JS::Rooted<JS::Value> result(
-        cx, StringToJsval(mRequest->GetOwner(), r.storageStatus()));
+      JS::Rooted<JS::Value> result(cx);
+      StringToJsval(mRequest->GetOwner(), r.storageStatus(), &result);
       mRequest->FireSuccess(result);
       break;
     }
 
     case DeviceStorageResponseValue::TEnumerationResponse:
     {
       EnumerationResponse r = aValue;
       nsDOMDeviceStorageCursor* cursor
--- a/dom/devicestorage/nsDeviceStorage.cpp
+++ b/dom/devicestorage/nsDeviceStorage.cpp
@@ -1635,31 +1635,37 @@ nsDOMDeviceStorage::SetRootDirectoryForT
   mStorageName = aStorageName;
 }
 
 JS::Value
 InterfaceToJsval(nsPIDOMWindow* aWindow,
                  nsISupports* aObject,
                  const nsIID* aIID)
 {
-  AutoJSContext cx;
   nsCOMPtr<nsIScriptGlobalObject> sgo = do_QueryInterface(aWindow);
   if (!sgo) {
-    return JSVAL_NULL;
-  }
-
-  JS::Rooted<JSObject*> scopeObj(cx, sgo->GetGlobalJSObject());
-  NS_ENSURE_TRUE(scopeObj, JSVAL_NULL);
-  JSAutoCompartment ac(cx, scopeObj);
-
-
-  JS::Rooted<JS::Value> someJsVal(cx);
-  nsresult rv = nsContentUtils::WrapNative(cx, aObject, aIID, &someJsVal);
+    return JS::NullValue();
+  }
+
+  JSObject *unrootedScopeObj = sgo->GetGlobalJSObject();
+  NS_ENSURE_TRUE(unrootedScopeObj, JS::NullValue());
+  JSRuntime *runtime = JS_GetObjectRuntime(unrootedScopeObj);
+  JS::Rooted<JS::Value> someJsVal(runtime);
+  nsresult rv;
+
+  { // Protect someJsVal from moving GC in ~JSAutoCompartment
+    AutoJSContext cx;
+
+    JS::Rooted<JSObject*> scopeObj(cx, unrootedScopeObj);
+    JSAutoCompartment ac(cx, scopeObj);
+
+    rv = nsContentUtils::WrapNative(cx, aObject, aIID, &someJsVal);
+  }
   if (NS_FAILED(rv)) {
-    return JSVAL_NULL;
+    return JS::NullValue();
   }
 
   return someJsVal;
 }
 
 JS::Value
 nsIFileToJsval(nsPIDOMWindow* aWindow, DeviceStorageFile* aFile)
 {
@@ -1685,42 +1691,43 @@ nsIFileToJsval(nsPIDOMWindow* aWindow, D
   MOZ_ASSERT(aFile->mLastModifiedDate != UINT64_MAX);
 
   nsCOMPtr<nsIDOMBlob> blob = new nsDOMFileFile(fullPath, aFile->mMimeType,
                                                 aFile->mLength, aFile->mFile,
                                                 aFile->mLastModifiedDate);
   return InterfaceToJsval(aWindow, blob, &NS_GET_IID(nsIDOMBlob));
 }
 
-JS::Value StringToJsval(nsPIDOMWindow* aWindow, nsAString& aString)
+bool
+StringToJsval(nsPIDOMWindow* aWindow, nsAString& aString,
+              JS::MutableHandle<JS::Value> result)
 {
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(aWindow);
 
   nsCOMPtr<nsIScriptGlobalObject> sgo = do_QueryInterface(aWindow);
   if (!sgo) {
-    return JSVAL_NULL;
+    return false;
   }
 
   nsIScriptContext *scriptContext = sgo->GetScriptContext();
   if (!scriptContext) {
     return JSVAL_NULL;
   }
 
   AutoPushJSContext cx(scriptContext->GetNativeContext());
   if (!cx) {
     return JSVAL_NULL;
   }
 
-  JS::Rooted<JS::Value> result(cx);
-  if (!xpc::StringToJsval(cx, aString, &result)) {
-    return JSVAL_NULL;
-  }
-
-  return result;
+  if (!xpc::StringToJsval(cx, aString, result)) {
+    return false;
+  }
+
+  return true;
 }
 
 class DeviceStorageCursorRequest MOZ_FINAL
   : public nsIContentPermissionRequest
   , public PCOMContentPermissionRequestChild
 {
 public:
   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
@@ -2109,18 +2116,18 @@ public:
     MOZ_ASSERT(NS_IsMainThread());
 
     nsString state = NS_LITERAL_STRING("unavailable");
     if (mFile) {
       mFile->GetStatus(state);
     }
 
     AutoJSContext cx;
-    JS::Rooted<JS::Value> result(cx,
-                                 StringToJsval(mRequest->GetOwner(), state));
+    JS::Rooted<JS::Value> result(cx);
+    StringToJsval(mRequest->GetOwner(), state, &result);
     mRequest->FireSuccess(result);
     mRequest = nullptr;
     return NS_OK;
   }
 
 private:
   nsRefPtr<DeviceStorageFile> mFile;
   nsRefPtr<DOMRequest> mRequest;
@@ -2143,18 +2150,18 @@ public:
     MOZ_ASSERT(NS_IsMainThread());
 
     nsString state = NS_LITERAL_STRING("undefined");
     if (mFile) {
       mFile->GetStorageStatus(state);
     }
 
     AutoJSContext cx;
-    JS::Rooted<JS::Value> result(cx,
-                                 StringToJsval(mRequest->GetOwner(), state));
+    JS::Rooted<JS::Value> result(cx);
+    StringToJsval(mRequest->GetOwner(), state, &result);
     mRequest->FireSuccess(result);
     mRequest = nullptr;
     return NS_OK;
   }
 
 private:
   nsRefPtr<DeviceStorageFile> mFile;
   nsRefPtr<DOMRequest> mRequest;
@@ -2177,18 +2184,18 @@ public:
     MOZ_ASSERT(NS_IsMainThread());
 
     nsString state = NS_LITERAL_STRING("unavailable");
     if (mFile) {
       mFile->DoFormat(state);
     }
 
     AutoJSContext cx;
-    JS::Rooted<JS::Value> result(cx,
-                                 StringToJsval(mRequest->GetOwner(), state));
+    JS::Rooted<JS::Value> result(cx);
+    StringToJsval(mRequest->GetOwner(), state, &result);
     mRequest->FireSuccess(result);
     mRequest = nullptr;
     return NS_OK;
   }
 
 private:
   nsRefPtr<DeviceStorageFile> mFile;
   nsRefPtr<DOMRequest> mRequest;
@@ -2211,18 +2218,18 @@ public:
     MOZ_ASSERT(NS_IsMainThread());
 
     nsString state = NS_LITERAL_STRING("unavailable");
     if (mFile) {
       mFile->DoMount(state);
     }
 
     AutoJSContext cx;
-    JS::Rooted<JS::Value> result(cx,
-                                 StringToJsval(mRequest->GetOwner(), state));
+    JS::Rooted<JS::Value> result(cx);
+    StringToJsval(mRequest->GetOwner(), state, &result);
     mRequest->FireSuccess(result);
     mRequest = nullptr;
     return NS_OK;
   }
 
 private:
   nsRefPtr<DeviceStorageFile> mFile;
   nsRefPtr<DOMRequest> mRequest;
@@ -2245,18 +2252,18 @@ public:
     MOZ_ASSERT(NS_IsMainThread());
 
     nsString state = NS_LITERAL_STRING("unavailable");
     if (mFile) {
       mFile->DoUnmount(state);
     }
 
     AutoJSContext cx;
-    JS::Rooted<JS::Value> result(cx,
-                                 StringToJsval(mRequest->GetOwner(), state));
+    JS::Rooted<JS::Value> result(cx);
+    StringToJsval(mRequest->GetOwner(), state, &result);
     mRequest->FireSuccess(result);
     mRequest = nullptr;
     return NS_OK;
   }
 
 private:
   nsRefPtr<DeviceStorageFile> mFile;
   nsRefPtr<DOMRequest> mRequest;
@@ -2297,17 +2304,17 @@ public:
 
     AutoJSContext cx;
     JS::Rooted<JS::Value> result(cx, JSVAL_NULL);
     nsPIDOMWindow* window = mRequest->GetOwner();
 
     if (mFile) {
       result = nsIFileToJsval(window, mFile);
     } else if (mPath.Length()) {
-      result = StringToJsval(window, mPath);
+      StringToJsval(window, mPath, &result);
     }
     else {
       result = JS_NumberValue(double(mValue));
     }
 
     mRequest->FireSuccess(result);
     mRequest = nullptr;
     return NS_OK;
--- a/dom/devicestorage/nsDeviceStorage.h
+++ b/dom/devicestorage/nsDeviceStorage.h
@@ -219,18 +219,19 @@ public:
 private:
   ~nsDOMDeviceStorageCursor();
 
   nsRefPtr<DeviceStorageFile> mFile;
   nsCOMPtr<nsIPrincipal> mPrincipal;
 };
 
 //helpers
-JS::Value
-StringToJsval(nsPIDOMWindow* aWindow, nsAString& aString);
+bool
+StringToJsval(nsPIDOMWindow* aWindow, nsAString& aString,
+              JS::MutableHandle<JS::Value> result);
 
 JS::Value
 nsIFileToJsval(nsPIDOMWindow* aWindow, DeviceStorageFile* aFile);
 
 JS::Value
 InterfaceToJsval(nsPIDOMWindow* aWindow, nsISupports* aObject, const nsIID* aIID);
 
 #endif