Bug 1176341 - De-holder nsIXPConnect::CreateSandbox. r=baku,gabor

dom/base/Console.cpp

@@ -324,13 +324,7 @@ private:
 
     AutoSafeJSContext cx;
 
-    nsCOMPtr<nsIXPConnectJSObjectHolder> sandbox =
-      mConsole->GetOrCreateSandbox(cx, wp->GetPrincipal());
-    if (NS_WARN_IF(!sandbox)) {
-       return;
-    }
-
-    JS::Rooted<JSObject*> global(cx, sandbox->GetJSObject());
+    JS::Rooted<JSObject*> global(cx, mConsole->GetOrCreateSandbox(cx, wp->GetPrincipal()));
     if (NS_WARN_IF(!global)) {
       return;
     }
@@ -661,7 +655,7 @@ private:
 
 NS_IMPL_CYCLE_COLLECTION_CLASS(Console)
 
-// We don't need to traverse/unlink mStorage and mSanbox because they are not
+// We don't need to traverse/unlink mStorage and mSandbox because they are not
 // CCed objects and they are only used on the main thread, even when this
 // Console object is used on workers.
 
@@ -715,19 +709,12 @@ Console::Console(nsPIDOMWindow* aWindow)
 Console::~Console()
 {
   if (!NS_IsMainThread()) {
-    nsCOMPtr<nsIThread> mainThread;
-    NS_GetMainThread(getter_AddRefs(mainThread));
-
     if (mStorage) {
-      nsIConsoleAPIStorage* storage;
-      mStorage.forget(&storage);
-      NS_ProxyRelease(mainThread, storage, false);
+      NS_ReleaseOnMainThread(mStorage);
     }
 
     if (mSandbox) {
-      nsIXPConnectJSObjectHolder* sandbox;
-      mSandbox.forget(&sandbox);
-      NS_ProxyRelease(mainThread, sandbox, false);
+      NS_ReleaseOnMainThread(mSandbox);
     }
   }
 
@@ -1876,7 +1863,7 @@ Console::ShouldIncludeStackTrace(MethodName aMethodName)
   }
 }
 
-nsIXPConnectJSObjectHolder*
+JSObject*
 Console::GetOrCreateSandbox(JSContext* aCx, nsIPrincipal* aPrincipal)
 {
   MOZ_ASSERT(NS_IsMainThread());
@@ -1885,14 +1872,16 @@ Console::GetOrCreateSandbox(JSContext* aCx, nsIPrincipal* aPrincipal)
     nsIXPConnect* xpc = nsContentUtils::XPConnect();
     MOZ_ASSERT(xpc, "This should never be null!");
 
-    nsresult rv = xpc->CreateSandbox(aCx, aPrincipal,
-                                     getter_AddRefs(mSandbox));
+    JS::Rooted<JSObject*> sandbox(aCx);
+    nsresult rv = xpc->CreateSandbox(aCx, aPrincipal, sandbox.address());
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return nullptr;
     }
+
+    mSandbox = new JSObjectHolder(aCx, sandbox);
   }
 
-  return mSandbox;
+  return mSandbox->GetJSObject();
 }
 
 } // namespace dom

dom/base/Console.h

@@ -9,6 +9,7 @@
 
 #include "mozilla/dom/BindingDeclarations.h"
 #include "mozilla/ErrorResult.h"
+#include "mozilla/JSObjectHolder.h"
 #include "nsCycleCollectionParticipant.h"
 #include "nsDataHashtable.h"
 #include "nsHashKeys.h"
@@ -20,7 +21,6 @@
 class nsIConsoleAPIStorage;
 class nsIPrincipal;
 class nsIProfiler;
-class nsIXPConnectJSObjectHolder;
 
 namespace mozilla {
 namespace dom {
@@ -199,12 +199,12 @@ private:
   bool
   ShouldIncludeStackTrace(MethodName aMethodName);
 
-  nsIXPConnectJSObjectHolder*
+  JSObject*
   GetOrCreateSandbox(JSContext* aCx, nsIPrincipal* aPrincipal);
 
   nsCOMPtr<nsPIDOMWindow> mWindow;
   nsCOMPtr<nsIConsoleAPIStorage> mStorage;
-  nsCOMPtr<nsIXPConnectJSObjectHolder> mSandbox;
+  nsRefPtr<JSObjectHolder> mSandbox;
 #ifdef MOZ_ENABLE_PROFILER_SPS
   nsCOMPtr<nsIProfiler> mProfiler;
 #endif

dom/datastore/DataStoreDB.cpp

@@ -113,18 +113,12 @@ DataStoreDB::CreateFactoryIfNeeded()
     MOZ_ASSERT(xpc);
 
     AutoSafeJSContext cx;
-
-    nsCOMPtr<nsIXPConnectJSObjectHolder> globalHolder;
-    rv = xpc->CreateSandbox(cx, principal, getter_AddRefs(globalHolder));
+    JS::Rooted<JSObject*> global(cx);
+    rv = xpc->CreateSandbox(cx, principal, global.address());
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return rv;
     }
 
-    JS::Rooted<JSObject*> global(cx, globalHolder->GetJSObject());
-    if (NS_WARN_IF(NS_FAILED(rv))) {
-      return NS_ERROR_UNEXPECTED;
-    }
-
     // The CreateSandbox call returns a proxy to the actual sandbox object. We
     // don't need a proxy here.
     global = js::UncheckedUnwrap(global);

dom/workers/ScriptLoader.cpp

@@ -1287,13 +1287,13 @@ CacheCreator::CreateCacheStorage(nsIPrincipal* aPrincipal)
   MOZ_ASSERT(xpc, "This should never be null!");
 
   mozilla::AutoSafeJSContext cx;
-  nsCOMPtr<nsIXPConnectJSObjectHolder> sandbox;
-  nsresult rv = xpc->CreateSandbox(cx, aPrincipal, getter_AddRefs(sandbox));
+  JS::Rooted<JSObject*> sandbox(cx);
+  nsresult rv = xpc->CreateSandbox(cx, aPrincipal, sandbox.address());
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return rv;
   }
 
-  mSandboxGlobalObject = xpc::NativeGlobal(sandbox->GetJSObject());
+  mSandboxGlobalObject = xpc::NativeGlobal(sandbox);
   if (NS_WARN_IF(!mSandboxGlobalObject)) {
     return NS_ERROR_FAILURE;
   }

dom/workers/ServiceWorkerScriptCache.cpp

@@ -32,35 +32,31 @@ namespace serviceWorkerScriptCache {
 
 namespace {
 
+// XXX A sandbox nsIGlobalObject does not preserve its reflector, so |aSandbox|
+// must be kept alive as long as the CacheStorage if you want to ensure that
+// the CacheStorage will continue to work. Failures will manifest as errors
+// like "JavaScript error: , line 0: TypeError: The expression cannot be
+// converted to return the specified type."
 already_AddRefed<CacheStorage>
-CreateCacheStorage(nsIPrincipal* aPrincipal, ErrorResult& aRv,
-                   nsIXPConnectJSObjectHolder** aHolder = nullptr)
+CreateCacheStorage(JSContext* aCx, nsIPrincipal* aPrincipal, ErrorResult& aRv,
+                   JS::MutableHandle<JSObject*> aSandbox)
 {
   AssertIsOnMainThread();
   MOZ_ASSERT(aPrincipal);
 
   nsIXPConnect* xpc = nsContentUtils::XPConnect();
   MOZ_ASSERT(xpc, "This should never be null!");
-
-  AutoJSAPI jsapi;
-  jsapi.Init();
-  nsCOMPtr<nsIXPConnectJSObjectHolder> sandbox;
-  aRv = xpc->CreateSandbox(jsapi.cx(), aPrincipal, getter_AddRefs(sandbox));
+  aRv = xpc->CreateSandbox(aCx, aPrincipal, aSandbox.address());
   if (NS_WARN_IF(aRv.Failed())) {
     return nullptr;
   }
 
-  nsCOMPtr<nsIGlobalObject> sandboxGlobalObject =
-    xpc::NativeGlobal(sandbox->GetJSObject());
+  nsCOMPtr<nsIGlobalObject> sandboxGlobalObject = xpc::NativeGlobal(aSandbox);
   if (!sandboxGlobalObject) {
     aRv.Throw(NS_ERROR_FAILURE);
     return nullptr;
   }
 
-  if (aHolder) {
-    sandbox.forget(aHolder);
-  }
-
   // We assume private browsing is not enabled here.  The ScriptLoader
   // explicitly fails for private browsing so there should never be
   // a service worker running in private browsing mode.  Therefore if
@@ -321,8 +317,11 @@ public:
 
     // Always create a CacheStorage since we want to write the network entry to
     // the cache even if there isn't an existing one.
+    AutoJSAPI jsapi;
+    jsapi.Init();
     ErrorResult result;
-    mCacheStorage = CreateCacheStorage(aPrincipal, result, getter_AddRefs(mSandbox));
+    mSandbox.init(jsapi.cx());
+    mCacheStorage = CreateCacheStorage(jsapi.cx(), aPrincipal, result, &mSandbox);
     if (NS_WARN_IF(result.Failed())) {
       MOZ_ASSERT(!result.IsErrorWithMessage());
       return result.StealNSResult();
@@ -623,7 +622,7 @@ private:
   }
 
   nsRefPtr<CompareCallback> mCallback;
-  nsCOMPtr<nsIXPConnectJSObjectHolder> mSandbox;
+  JS::PersistentRooted<JSObject*> mSandbox;
   nsRefPtr<CacheStorage> mCacheStorage;
 
   nsRefPtr<CompareNetwork> mCN;
@@ -959,8 +958,11 @@ PurgeCache(nsIPrincipal* aPrincipal, const nsAString& aCacheName)
     return NS_OK;
   }
 
+  AutoJSAPI jsapi;
+  jsapi.Init();
   ErrorResult rv;
-  nsRefPtr<CacheStorage> cacheStorage = CreateCacheStorage(aPrincipal, rv);
+  JS::Rooted<JSObject*> sandboxObject(jsapi.cx());
+  nsRefPtr<CacheStorage> cacheStorage = CreateCacheStorage(jsapi.cx(), aPrincipal, rv, &sandboxObject);
   if (NS_WARN_IF(rv.Failed())) {
     return rv.StealNSResult();
   }

extensions/pref/autoconfig/src/nsJSConfigTriggers.cpp

@@ -46,13 +46,13 @@ nsresult CentralizedAdminPrefManagerInit()
 
     // Create a sandbox.
     AutoSafeJSContext cx;
-    nsCOMPtr<nsIXPConnectJSObjectHolder> sandbox;
-    rv = xpc->CreateSandbox(cx, principal, getter_AddRefs(sandbox));
+    JS::Rooted<JSObject*> sandbox(cx);
+    rv = xpc->CreateSandbox(cx, principal, sandbox.address());
     NS_ENSURE_SUCCESS(rv, rv);
 
     // Unwrap, store and root the sandbox.
-    NS_ENSURE_STATE(sandbox->GetJSObject());
-    autoconfigSb.init(cx, js::UncheckedUnwrap(sandbox->GetJSObject()));
+    NS_ENSURE_STATE(sandbox);
+    autoconfigSb.init(cx, js::UncheckedUnwrap(sandbox));
 
     return NS_OK;
 }

js/xpconnect/idl/nsIXPConnect.idl

@@ -266,7 +266,7 @@ interface nsIXPCFunctionThisTranslator : nsISupports
     { 0xbd, 0xd6, 0x0, 0x0, 0x64, 0x65, 0x73, 0x74 } }
 %}
 
-[noscript, uuid(b91f1eeb-2fe4-44cc-9983-abcc06d69a94)]
+[noscript, uuid(db83b3af-ac22-4dd2-99cf-7f79270ed4cd)]
 interface nsIXPConnect : nsISupports
 {
 %{ C++
@@ -474,8 +474,7 @@ interface nsIXPConnect : nsISupports
      * @param principal The principal (or NULL to use the null principal)
      *                  to use when evaluating code in this sandbox.
      */
-    [noscript] nsIXPConnectJSObjectHolder createSandbox(in JSContextPtr cx,
-                                                        in nsIPrincipal principal);
+    [noscript] JSObjectPtr createSandbox(in JSContextPtr cx, in nsIPrincipal principal);
 
     /**
      * Evaluate script in a sandbox, completely isolated from all

js/xpconnect/src/nsXPConnect.cpp

@@ -751,7 +751,7 @@ nsXPConnect::SetFunctionThisTranslator(const nsIID & aIID,
 
 NS_IMETHODIMP
 nsXPConnect::CreateSandbox(JSContext* cx, nsIPrincipal* principal,
-                           nsIXPConnectJSObjectHolder** _retval)
+                           JSObject** _retval)
 {
     *_retval = nullptr;
 
@@ -762,9 +762,7 @@ nsXPConnect::CreateSandbox(JSContext* cx, nsIPrincipal* principal,
                "Bad return value from xpc_CreateSandboxObject()!");
 
     if (NS_SUCCEEDED(rv) && !rval.isPrimitive()) {
-        JSObject* obj = rval.toObjectOrNull();
-        nsRefPtr<XPCJSObjectHolder> rval = new XPCJSObjectHolder(obj);
-        rval.forget(_retval);
+        *_retval = rval.toObjectOrNull();
     }
 
     return rv;