Bug 1521907 part 4. Start using CheckedUnwrapStatic/Dynamic in non-binding DOM code. r=peterv
☠☠ backed out by 0afc21b5734a ☠ ☠
authorBoris Zbarsky <bzbarsky@mit.edu>
Thu, 31 Jan 2019 15:51:52 +0000
changeset 456458 192152fe986a8a5fcacd1de95965bcbecd908030
parent 456457 ca65b46b0d3708a115042dd4f5484bdc7a269a6b
child 456459 2d089514890716397b1cee3f4ade5dc43218319e
push id111652
push userncsoregi@mozilla.com
push dateFri, 01 Feb 2019 22:14:41 +0000
treeherdermozilla-inbound@a36422c1abbc [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspeterv
bugs1521907
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1521907 part 4. Start using CheckedUnwrapStatic/Dynamic in non-binding DOM code. r=peterv Differential Revision: https://phabricator.services.mozilla.com/D17884
dom/base/ChromeUtils.cpp
dom/base/CustomElementRegistry.cpp
dom/base/StructuredCloneBlob.cpp
dom/base/nsContentPermissionHelper.cpp
dom/base/nsGlobalWindowOuter.cpp
dom/base/nsJSUtils.cpp
dom/media/webaudio/AudioContext.cpp
dom/media/webaudio/AudioWorkletGlobalScope.cpp
dom/plugins/base/nsJSNPRuntime.cpp
dom/promise/PromiseDebugging.cpp
dom/workers/WorkerScope.cpp
--- a/dom/base/ChromeUtils.cpp
+++ b/dom/base/ChromeUtils.cpp
@@ -216,17 +216,20 @@ namespace dom {
 
   auto cleanup = MakeScopeExit([&]() { aRv.NoteJSContextException(cx); });
 
   JS::Rooted<JS::IdVector> ids(cx, JS::IdVector(cx));
   JS::AutoValueVector values(cx);
   JS::AutoIdVector valuesIds(cx);
 
   {
-    JS::RootedObject obj(cx, js::CheckedUnwrap(aObj));
+    // cx represents our current Realm, so it makes sense to use it for the
+    // CheckedUnwrapDynamic call.  We do want CheckedUnwrapDynamic, in case
+    // someone is shallow-cloning a Window.
+    JS::RootedObject obj(cx, js::CheckedUnwrapDynamic(aObj, cx));
     if (!obj) {
       js::ReportAccessDenied(cx);
       return;
     }
 
     if (js::IsScriptedProxy(obj)) {
       JS_ReportErrorASCII(cx, "Shallow cloning a proxy object is not allowed");
       return;
@@ -253,17 +256,20 @@ namespace dom {
       values.infallibleAppend(desc.value());
     }
   }
 
   JS::RootedObject obj(cx);
   {
     Maybe<JSAutoRealm> ar;
     if (aTarget) {
-      JS::RootedObject target(cx, js::CheckedUnwrap(aTarget));
+      // Our target could be anything, so we want CheckedUnwrapDynamic here.
+      // "cx" represents the current Realm when we were called from bindings, so
+      // we can just use that.
+      JS::RootedObject target(cx, js::CheckedUnwrapDynamic(aTarget, cx));
       if (!target) {
         js::ReportAccessDenied(cx);
         return;
       }
       ar.emplace(cx, target);
     }
 
     obj = JS_NewPlainObject(cx);
--- a/dom/base/CustomElementRegistry.cpp
+++ b/dom/base/CustomElementRegistry.cpp
@@ -375,17 +375,19 @@ CustomElementDefinition* CustomElementRe
     return data;
   }
 
   return nullptr;
 }
 
 CustomElementDefinition* CustomElementRegistry::LookupCustomElementDefinition(
     JSContext* aCx, JSObject* aConstructor) const {
-  JS::Rooted<JSObject*> constructor(aCx, js::CheckedUnwrap(aConstructor));
+  // We're looking up things that tested true for JS::IsConstructor,
+  // so doing a CheckedUnwrapStatic is fine here.
+  JS::Rooted<JSObject*> constructor(aCx, js::CheckedUnwrapStatic(aConstructor));
 
   const auto& ptr = mConstructors.lookup(constructor);
   if (!ptr) {
     return nullptr;
   }
 
   CustomElementDefinition* definition =
       mCustomDefinitions.GetWeak(ptr->value());
@@ -660,18 +662,24 @@ int32_t CustomElementRegistry::InferName
 
 // https://html.spec.whatwg.org/multipage/scripting.html#element-definition
 void CustomElementRegistry::Define(JSContext* aCx, const nsAString& aName,
                                    Function& aFunctionConstructor,
                                    const ElementDefinitionOptions& aOptions,
                                    ErrorResult& aRv) {
   JS::Rooted<JSObject*> constructor(aCx, aFunctionConstructor.CallableOrNull());
 
-  JS::Rooted<JSObject*> constructorUnwrapped(aCx,
-                                             js::CheckedUnwrap(constructor));
+  // We need to do a dynamic unwrap in order to throw the right exception.  We
+  // could probably avoid that if we just threw MSG_NOT_CONSTRUCTOR if unwrap
+  // fails.
+  //
+  // In any case, aCx represents the global we want to be using for the unwrap
+  // here.
+  JS::Rooted<JSObject*> constructorUnwrapped(
+      aCx, js::CheckedUnwrapDynamic(constructor, aCx));
   if (!constructorUnwrapped) {
     // If the caller's compartment does not have permission to access the
     // unwrapped constructor then throw.
     aRv.Throw(NS_ERROR_DOM_SECURITY_ERR);
     return;
   }
 
   /**
--- a/dom/base/StructuredCloneBlob.cpp
+++ b/dom/base/StructuredCloneBlob.cpp
@@ -35,31 +35,34 @@ StructuredCloneBlob::Constructor(GlobalO
   JSContext* cx = aGlobal.Context();
 
   RefPtr<StructuredCloneBlob> holder = StructuredCloneBlob::Create();
 
   Maybe<JSAutoRealm> ar;
   JS::RootedValue value(cx, aValue);
 
   if (aTargetGlobal) {
-    JS::RootedObject targetGlobal(cx, js::CheckedUnwrap(aTargetGlobal));
+    // OK to unwrap if our caller (represented by cx's Realm) can do it.
+    JS::RootedObject targetGlobal(cx,
+                                  js::CheckedUnwrapDynamic(aTargetGlobal, cx));
     if (!targetGlobal) {
       js::ReportAccessDenied(cx);
       aRv.NoteJSContextException(cx);
       return nullptr;
     }
 
     ar.emplace(cx, targetGlobal);
 
     if (!JS_WrapValue(cx, &value)) {
       aRv.NoteJSContextException(cx);
       return nullptr;
     }
   } else if (value.isObject()) {
-    JS::RootedObject obj(cx, js::CheckedUnwrap(&value.toObject()));
+    // OK to unwrap if our caller (represented by cx's Realm) can do it.
+    JS::RootedObject obj(cx, js::CheckedUnwrapDynamic(&value.toObject(), cx));
     if (!obj) {
       js::ReportAccessDenied(cx);
       aRv.NoteJSContextException(cx);
       return nullptr;
     }
 
     ar.emplace(cx, obj);
     value = JS::ObjectValue(*obj);
@@ -73,17 +76,18 @@ StructuredCloneBlob::Constructor(GlobalO
   return holder.forget();
 }
 
 void StructuredCloneBlob::Deserialize(JSContext* aCx,
                                       JS::HandleObject aTargetScope,
                                       bool aKeepData,
                                       JS::MutableHandleValue aResult,
                                       ErrorResult& aRv) {
-  JS::RootedObject scope(aCx, js::CheckedUnwrap(aTargetScope));
+  // OK to unwrap if our caller (represented by aCx's Realm) can do it.
+  JS::RootedObject scope(aCx, js::CheckedUnwrapDynamic(aTargetScope, aCx));
   if (!scope) {
     js::ReportAccessDenied(aCx);
     aRv.NoteJSContextException(aCx);
     return;
   }
 
   if (!mHolder.isSome()) {
     aRv.Throw(NS_ERROR_NOT_INITIALIZED);
--- a/dom/base/nsContentPermissionHelper.cpp
+++ b/dom/base/nsContentPermissionHelper.cpp
@@ -665,17 +665,19 @@ nsresult TranslateChoices(
   if (aChoices.isNullOrUndefined()) {
     // No choice is specified.
   } else if (aChoices.isObject()) {
     // Iterate through all permission types.
     for (uint32_t i = 0; i < aPermissionRequests.Length(); ++i) {
       nsCString type = aPermissionRequests[i].type();
 
       JS::Rooted<JSObject*> obj(RootingCx(), &aChoices.toObject());
-      obj = CheckedUnwrap(obj);
+      // People really shouldn't be passing WindowProxy or Location
+      // objects for the choices here.
+      obj = js::CheckedUnwrapStatic(obj);
       if (!obj) {
         return NS_ERROR_FAILURE;
       }
 
       AutoJSAPI jsapi;
       jsapi.Init();
 
       JSContext* cx = jsapi.cx();
--- a/dom/base/nsGlobalWindowOuter.cpp
+++ b/dom/base/nsGlobalWindowOuter.cpp
@@ -5775,18 +5775,22 @@ nsGlobalWindowInner* nsGlobalWindowOuter
   // sandboxPrototype. This used to work incidentally for unrelated reasons, but
   // now we need to do some special handling to support it.
   if (xpc::IsSandbox(scope)) {
     JSAutoRealm ar(aCx, scope);
     JS::Rooted<JSObject*> scopeProto(aCx);
     bool ok = JS_GetPrototype(aCx, scope, &scopeProto);
     NS_ENSURE_TRUE(ok, nullptr);
     if (scopeProto && xpc::IsSandboxPrototypeProxy(scopeProto) &&
-        (scopeProto =
-             js::CheckedUnwrap(scopeProto, /* stopAtWindowProxy = */ false))) {
+        // Our current Realm on aCx is the sandbox.  Using that for the
+        // CheckedUnwrapDynamic call makes sense: if the sandbox can unwrap the
+        // window, we can use it.  And we do want CheckedUnwrapDynamic, because
+        // the whole point is to unwrap windows.
+        (scopeProto = js::CheckedUnwrapDynamic(
+             scopeProto, aCx, /* stopAtWindowProxy = */ false))) {
       global = xpc::NativeGlobal(scopeProto);
       NS_ENSURE_TRUE(global, nullptr);
     }
   }
 
   // The calling window must be holding a reference, so we can return a weak
   // pointer.
   nsCOMPtr<nsPIDOMWindowInner> win = do_QueryInterface(global);
--- a/dom/base/nsJSUtils.cpp
+++ b/dom/base/nsJSUtils.cpp
@@ -395,17 +395,18 @@ nsresult nsJSUtils::ExecutionContext::Ex
   return NS_OK;
 }
 
 static bool IsPromiseValue(JSContext* aCx, JS::Handle<JS::Value> aValue) {
   if (!aValue.isObject()) {
     return false;
   }
 
-  JS::Rooted<JSObject*> obj(aCx, js::CheckedUnwrap(&aValue.toObject()));
+  // We only care about Promise here, so CheckedUnwrapStatic is fine.
+  JS::Rooted<JSObject*> obj(aCx, js::CheckedUnwrapStatic(&aValue.toObject()));
   if (!obj) {
     return false;
   }
 
   return JS::IsPromiseObject(obj);
 }
 
 nsresult nsJSUtils::ExecutionContext::ExecScript(
--- a/dom/media/webaudio/AudioContext.cpp
+++ b/dom/media/webaudio/AudioContext.cpp
@@ -540,17 +540,18 @@ already_AddRefed<Promise> AudioContext::
     const Optional<OwningNonNull<DecodeErrorCallback> >& aFailureCallback,
     ErrorResult& aRv) {
   nsCOMPtr<nsIGlobalObject> parentObject = do_QueryInterface(GetParentObject());
   RefPtr<Promise> promise;
   AutoJSAPI jsapi;
   jsapi.Init();
   JSContext* cx = jsapi.cx();
 
-  JS::Rooted<JSObject*> obj(cx, js::CheckedUnwrap(aBuffer.Obj()));
+  // CheckedUnwrapStatic is OK, since we know we have an ArrayBuffer.
+  JS::Rooted<JSObject*> obj(cx, js::CheckedUnwrapStatic(aBuffer.Obj()));
   if (!obj) {
     aRv.Throw(NS_ERROR_DOM_SECURITY_ERR);
     return nullptr;
   }
 
   JSAutoRealm ar(cx, obj);
 
   promise = Promise::Create(parentObject, aRv);
--- a/dom/media/webaudio/AudioWorkletGlobalScope.cpp
+++ b/dom/media/webaudio/AudioWorkletGlobalScope.cpp
@@ -69,18 +69,19 @@ void AudioWorkletGlobalScope::RegisterPr
     aRv.ThrowDOMException(
         NS_ERROR_DOM_NOT_SUPPORTED_ERR,
         NS_LITERAL_CSTRING(
             "Argument 1 of AudioWorkletGlobalScope.registerProcessor "
             "is invalid: a class with the same name is already registered."));
     return;
   }
 
+  // We know processorConstructor is callable, so not a WindowProxy or Location.
   JS::Rooted<JSObject*> constructorUnwrapped(
-      aCx, js::CheckedUnwrap(processorConstructor));
+      aCx, js::CheckedUnwrapStatic(processorConstructor));
   if (!constructorUnwrapped) {
     // If the caller's compartment does not have permission to access the
     // unwrapped constructor then throw.
     aRv.Throw(NS_ERROR_DOM_SECURITY_ERR);
     return;
   }
 
   /**
--- a/dom/plugins/base/nsJSNPRuntime.cpp
+++ b/dom/plugins/base/nsJSNPRuntime.cpp
@@ -517,17 +517,19 @@ bool JSValToNPVariant(NPP npp, JSContext
   // element has since been adopted into a new document. We don't bother
   // transplanting the plugin objects, and just do a unwrap with security
   // checks if we encounter one of them as an argument. If the unwrap fails,
   // we run with the original wrapped object, since sometimes there are
   // legitimate cases where a security wrapper ends up here (for example,
   // Location objects, which are _always_ behind security wrappers).
   JS::Rooted<JSObject *> obj(cx, &val.toObject());
   JS::Rooted<JSObject *> global(cx);
-  obj = js::CheckedUnwrap(obj);
+  // CheckedUnwrapStatic is fine here; if we get a Location or WindowProxy,
+  // we'll just use the current global instead.
+  obj = js::CheckedUnwrapStatic(obj);
   if (obj) {
     global = JS::GetNonCCWObjectGlobal(obj);
   } else {
     obj = &val.toObject();
     global = JS::CurrentGlobalOrNull(cx);
   }
 
   NPObject *npobj = nsJSObjWrapper::GetNewOrUsed(npp, obj, global);
@@ -1069,17 +1071,20 @@ NPObject *nsJSObjWrapper::GetNewOrUsed(N
 //
 // Because this function unwraps, its return value must be wrapped for the cx
 // compartment for callers that plan to hold onto the result or do anything
 // substantial with it.
 static JSObject *GetNPObjectWrapper(JSContext *cx, JS::Handle<JSObject *> aObj,
                                     bool wrapResult = true) {
   JS::Rooted<JSObject *> obj(cx, aObj);
 
-  while (obj && (obj = js::CheckedUnwrap(obj))) {
+  // We can't have WindowProxy or Location objects with NP object wrapper
+  // objects on their proto chain, since they have immutable prototypes.  So
+  // CheckedUnwrapStatic is ok here.
+  while (obj && (obj = js::CheckedUnwrapStatic(obj))) {
     if (nsNPObjWrapper::IsWrapper(obj)) {
       if (wrapResult && !JS_WrapObject(cx, &obj)) {
         return nullptr;
       }
       return obj;
     }
 
     JSAutoRealm ar(cx, obj);
--- a/dom/promise/PromiseDebugging.cpp
+++ b/dom/promise/PromiseDebugging.cpp
@@ -68,17 +68,18 @@ class FlushRejections : public Cancelabl
 };
 
 /* static */ MOZ_THREAD_LOCAL(bool) FlushRejections::sDispatched;
 
 /* static */ void PromiseDebugging::GetState(
     GlobalObject& aGlobal, JS::Handle<JSObject*> aPromise,
     PromiseDebuggingStateHolder& aState, ErrorResult& aRv) {
   JSContext* cx = aGlobal.Context();
-  JS::Rooted<JSObject*> obj(cx, js::CheckedUnwrap(aPromise));
+  // CheckedUnwrapStatic is fine, since we're looking for promises only.
+  JS::Rooted<JSObject*> obj(cx, js::CheckedUnwrapStatic(aPromise));
   if (!obj || !JS::IsPromiseObject(obj)) {
     aRv.ThrowTypeError<MSG_IS_NOT_PROMISE>(
         NS_LITERAL_STRING("Argument of PromiseDebugging.getState"));
     return;
   }
   switch (JS::GetPromiseState(obj)) {
     case JS::PromiseState::Pending:
       aState.mState = PromiseDebuggingState::Pending;
@@ -94,58 +95,62 @@ class FlushRejections : public Cancelabl
   }
 }
 
 /* static */ void PromiseDebugging::GetPromiseID(GlobalObject& aGlobal,
                                                  JS::Handle<JSObject*> aPromise,
                                                  nsString& aID,
                                                  ErrorResult& aRv) {
   JSContext* cx = aGlobal.Context();
-  JS::Rooted<JSObject*> obj(cx, js::CheckedUnwrap(aPromise));
+  // CheckedUnwrapStatic is fine, since we're looking for promises only.
+  JS::Rooted<JSObject*> obj(cx, js::CheckedUnwrapStatic(aPromise));
   if (!obj || !JS::IsPromiseObject(obj)) {
     aRv.ThrowTypeError<MSG_IS_NOT_PROMISE>(
         NS_LITERAL_STRING("Argument of PromiseDebugging.getState"));
     return;
   }
   uint64_t promiseID = JS::GetPromiseID(obj);
   aID = sIDPrefix;
   aID.AppendInt(promiseID);
 }
 
 /* static */ void PromiseDebugging::GetAllocationStack(
     GlobalObject& aGlobal, JS::Handle<JSObject*> aPromise,
     JS::MutableHandle<JSObject*> aStack, ErrorResult& aRv) {
   JSContext* cx = aGlobal.Context();
-  JS::Rooted<JSObject*> obj(cx, js::CheckedUnwrap(aPromise));
+  // CheckedUnwrapStatic is fine, since we're looking for promises only.
+  JS::Rooted<JSObject*> obj(cx, js::CheckedUnwrapStatic(aPromise));
   if (!obj || !JS::IsPromiseObject(obj)) {
     aRv.ThrowTypeError<MSG_IS_NOT_PROMISE>(
         NS_LITERAL_STRING("Argument of PromiseDebugging.getAllocationStack"));
     return;
   }
   aStack.set(JS::GetPromiseAllocationSite(obj));
 }
 
 /* static */ void PromiseDebugging::GetRejectionStack(
     GlobalObject& aGlobal, JS::Handle<JSObject*> aPromise,
     JS::MutableHandle<JSObject*> aStack, ErrorResult& aRv) {
   JSContext* cx = aGlobal.Context();
-  JS::Rooted<JSObject*> obj(cx, js::CheckedUnwrap(aPromise));
+  // CheckedUnwrapStatic is fine, since we're looking for promises only.
+  JS::Rooted<JSObject*> obj(cx, js::CheckedUnwrapStatic(aPromise));
   if (!obj || !JS::IsPromiseObject(obj)) {
     aRv.ThrowTypeError<MSG_IS_NOT_PROMISE>(
         NS_LITERAL_STRING("Argument of PromiseDebugging.getRejectionStack"));
     return;
   }
   aStack.set(JS::GetPromiseResolutionSite(obj));
 }
 
 /* static */ void PromiseDebugging::GetFullfillmentStack(
     GlobalObject& aGlobal, JS::Handle<JSObject*> aPromise,
     JS::MutableHandle<JSObject*> aStack, ErrorResult& aRv) {
   JSContext* cx = aGlobal.Context();
-  JS::Rooted<JSObject*> obj(cx, js::CheckedUnwrap(aPromise));
+  // CheckedUnwrapStatic is fine, since we're looking for promises only.
+  JS::Rooted<JSObject*> obj(cx, js::CheckedUnwrapStatic(aPromise));
   if (!obj || !JS::IsPromiseObject(obj)) {
     aRv.ThrowTypeError<MSG_IS_NOT_PROMISE>(
         NS_LITERAL_STRING("Argument of PromiseDebugging.getFulfillmentStack"));
     return;
   }
   aStack.set(JS::GetPromiseResolutionSite(obj));
 }
 
--- a/dom/workers/WorkerScope.cpp
+++ b/dom/workers/WorkerScope.cpp
@@ -871,18 +871,21 @@ void WorkerDebuggerGlobalScope::CreateSa
 
 void WorkerDebuggerGlobalScope::LoadSubScript(
     JSContext* aCx, const nsAString& aURL,
     const Optional<JS::Handle<JSObject*>>& aSandbox, ErrorResult& aRv) {
   mWorkerPrivate->AssertIsOnWorkerThread();
 
   Maybe<JSAutoRealm> ar;
   if (aSandbox.WasPassed()) {
-    JS::Rooted<JSObject*> sandbox(aCx, js::CheckedUnwrap(aSandbox.Value()));
-    if (!IsWorkerDebuggerSandbox(sandbox)) {
+    // We only care about worker debugger sandbox objects here, so
+    // CheckedUnwrapStatic is fine.
+    JS::Rooted<JSObject*> sandbox(aCx,
+                                  js::CheckedUnwrapStatic(aSandbox.Value()));
+    if (!sandbox || !IsWorkerDebuggerSandbox(sandbox)) {
       aRv.Throw(NS_ERROR_INVALID_ARG);
       return;
     }
 
     ar.emplace(aCx, sandbox);
   }
 
   nsTArray<nsString> urls;