From 621e57b2b7b53e40598a7c1dc3ad7e848f45c8c6 Mon Sep 17 00:00:00 2001 From: Andrew McCreight Date: Tue, 21 Jul 2015 11:31:44 -0700 Subject: [PATCH] Bug 1176341 - De-holder nsIXPConnect::CreateSandbox. r=baku,gabor --- dom/base/Console.cpp | 31 ++++++----------- dom/base/Console.h | 6 ++-- dom/datastore/DataStoreDB.cpp | 10 ++---- dom/workers/ScriptLoader.cpp | 6 ++-- dom/workers/ServiceWorkerScriptCache.cpp | 34 ++++++++++--------- .../autoconfig/src/nsJSConfigTriggers.cpp | 8 ++--- js/xpconnect/idl/nsIXPConnect.idl | 5 ++- js/xpconnect/src/nsXPConnect.cpp | 6 ++-- 8 files changed, 44 insertions(+), 62 deletions(-) diff --git a/dom/base/Console.cpp b/dom/base/Console.cpp index 9f33d232b6e5..271b7406c48a 100644 --- a/dom/base/Console.cpp +++ b/dom/base/Console.cpp @@ -324,13 +324,7 @@ private: AutoSafeJSContext cx; - nsCOMPtr sandbox = - mConsole->GetOrCreateSandbox(cx, wp->GetPrincipal()); - if (NS_WARN_IF(!sandbox)) { - return; - } - - JS::Rooted global(cx, sandbox->GetJSObject()); + JS::Rooted 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 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 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 diff --git a/dom/base/Console.h b/dom/base/Console.h index ed1ecc309105..4ef5fdb1a116 100644 --- a/dom/base/Console.h +++ b/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 mWindow; nsCOMPtr mStorage; - nsCOMPtr mSandbox; + nsRefPtr mSandbox; #ifdef MOZ_ENABLE_PROFILER_SPS nsCOMPtr mProfiler; #endif diff --git a/dom/datastore/DataStoreDB.cpp b/dom/datastore/DataStoreDB.cpp index 19d34f37ec9b..f8374abe9d17 100644 --- a/dom/datastore/DataStoreDB.cpp +++ b/dom/datastore/DataStoreDB.cpp @@ -113,18 +113,12 @@ DataStoreDB::CreateFactoryIfNeeded() MOZ_ASSERT(xpc); AutoSafeJSContext cx; - - nsCOMPtr globalHolder; - rv = xpc->CreateSandbox(cx, principal, getter_AddRefs(globalHolder)); + JS::Rooted global(cx); + rv = xpc->CreateSandbox(cx, principal, global.address()); if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } - JS::Rooted 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); diff --git a/dom/workers/ScriptLoader.cpp b/dom/workers/ScriptLoader.cpp index 41be9458343d..f8513f42ffc5 100644 --- a/dom/workers/ScriptLoader.cpp +++ b/dom/workers/ScriptLoader.cpp @@ -1287,13 +1287,13 @@ CacheCreator::CreateCacheStorage(nsIPrincipal* aPrincipal) MOZ_ASSERT(xpc, "This should never be null!"); mozilla::AutoSafeJSContext cx; - nsCOMPtr sandbox; - nsresult rv = xpc->CreateSandbox(cx, aPrincipal, getter_AddRefs(sandbox)); + JS::Rooted 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; } diff --git a/dom/workers/ServiceWorkerScriptCache.cpp b/dom/workers/ServiceWorkerScriptCache.cpp index 48b695585e16..b19872b5ce34 100644 --- a/dom/workers/ServiceWorkerScriptCache.cpp +++ b/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 -CreateCacheStorage(nsIPrincipal* aPrincipal, ErrorResult& aRv, - nsIXPConnectJSObjectHolder** aHolder = nullptr) +CreateCacheStorage(JSContext* aCx, nsIPrincipal* aPrincipal, ErrorResult& aRv, + JS::MutableHandle aSandbox) { AssertIsOnMainThread(); MOZ_ASSERT(aPrincipal); nsIXPConnect* xpc = nsContentUtils::XPConnect(); MOZ_ASSERT(xpc, "This should never be null!"); - - AutoJSAPI jsapi; - jsapi.Init(); - nsCOMPtr 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 sandboxGlobalObject = - xpc::NativeGlobal(sandbox->GetJSObject()); + nsCOMPtr 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 mCallback; - nsCOMPtr mSandbox; + JS::PersistentRooted mSandbox; nsRefPtr mCacheStorage; nsRefPtr mCN; @@ -959,8 +958,11 @@ PurgeCache(nsIPrincipal* aPrincipal, const nsAString& aCacheName) return NS_OK; } + AutoJSAPI jsapi; + jsapi.Init(); ErrorResult rv; - nsRefPtr cacheStorage = CreateCacheStorage(aPrincipal, rv); + JS::Rooted sandboxObject(jsapi.cx()); + nsRefPtr cacheStorage = CreateCacheStorage(jsapi.cx(), aPrincipal, rv, &sandboxObject); if (NS_WARN_IF(rv.Failed())) { return rv.StealNSResult(); } diff --git a/extensions/pref/autoconfig/src/nsJSConfigTriggers.cpp b/extensions/pref/autoconfig/src/nsJSConfigTriggers.cpp index da3cbe5859bb..8f46cef7419d 100644 --- a/extensions/pref/autoconfig/src/nsJSConfigTriggers.cpp +++ b/extensions/pref/autoconfig/src/nsJSConfigTriggers.cpp @@ -46,13 +46,13 @@ nsresult CentralizedAdminPrefManagerInit() // Create a sandbox. AutoSafeJSContext cx; - nsCOMPtr sandbox; - rv = xpc->CreateSandbox(cx, principal, getter_AddRefs(sandbox)); + JS::Rooted 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; } diff --git a/js/xpconnect/idl/nsIXPConnect.idl b/js/xpconnect/idl/nsIXPConnect.idl index f7b61c29bb67..868743ecf2bd 100644 --- a/js/xpconnect/idl/nsIXPConnect.idl +++ b/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 diff --git a/js/xpconnect/src/nsXPConnect.cpp b/js/xpconnect/src/nsXPConnect.cpp index ab748ea4033b..541dbb258947 100644 --- a/js/xpconnect/src/nsXPConnect.cpp +++ b/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 rval = new XPCJSObjectHolder(obj); - rval.forget(_retval); + *_retval = rval.toObjectOrNull(); } return rv;