Bug 1273251: Part 3 - Allow CallbackObject to contain a null callable. r?peterv draft
authorKris Maglione <maglione.k@gmail.com>
Mon, 14 Nov 2016 21:25:37 -0800
changeset 440649 30b422627c215456cd5cbdc01d6be2edae05b183
parent 440648 ccf57291e2b56650eff94d44f6bd09ff267232b5
child 440650 a4208140ef07713fe6259b4d3e62a43ad0e39d41
push id36285
push usermaglione.k@gmail.com
push dateThu, 17 Nov 2016 23:17:43 +0000
reviewerspeterv
bugs1273251
milestone52.0a1
Bug 1273251: Part 3 - Allow CallbackObject to contain a null callable. r?peterv MozReview-Commit-ID: FCXVHouhG3I
dom/base/CustomElementRegistry.cpp
dom/bindings/BindingUtils.h
dom/bindings/CallbackFunction.h
dom/bindings/CallbackObject.cpp
dom/bindings/CallbackObject.h
dom/bindings/Codegen.py
dom/bindings/ToJSValue.h
dom/console/Console.cpp
dom/events/DOMEventTargetHelper.cpp
dom/events/EventListenerService.cpp
dom/promise/Promise.cpp
dom/promise/PromiseCallback.cpp
dom/workers/WorkerPrivate.cpp
xpcom/base/CycleCollectedJSContext.cpp
--- a/dom/base/CustomElementRegistry.cpp
+++ b/dom/base/CustomElementRegistry.cpp
@@ -567,17 +567,17 @@ CustomElementRegistry::Define(const nsAS
 
   AutoJSAPI jsapi;
   if (NS_WARN_IF(!jsapi.Init(mWindow))) {
     aRv.Throw(NS_ERROR_FAILURE);
     return;
   }
 
   JSContext *cx = jsapi.cx();
-  JS::Rooted<JSObject*> constructor(cx, aFunctionConstructor.Callable());
+  JS::Rooted<JSObject*> constructor(cx, aFunctionConstructor.CallableOrNull());
 
   /**
    * 1. If IsConstructor(constructor) is false, then throw a TypeError and abort
    *    these steps.
    */
   // For now, all wrappers are constructable if they are callable. So we need to
   // unwrap constructor to check it is really constructable.
   JS::Rooted<JSObject*> constructorUnwrapped(cx, js::CheckedUnwrap(constructor));
@@ -855,9 +855,9 @@ CustomElementDefinition::CustomElementDe
     mConstructor(aConstructor),
     mPrototype(aPrototype),
     mCallbacks(aCallbacks),
     mDocOrder(aDocOrder)
 {
 }
 
 } // namespace dom
-} // namespace mozilla
\ No newline at end of file
+} // namespace mozilla
--- a/dom/bindings/BindingUtils.h
+++ b/dom/bindings/BindingUtils.h
@@ -1761,17 +1761,17 @@ GetOrCreateDOMReflectorNoWrap(JSContext*
   return
     GetOrCreateDOMReflectorNoWrapHelper<T>::GetOrCreate(cx, value, rval);
 }
 
 template <class T>
 inline JSObject*
 GetCallbackFromCallbackObject(T* aObj)
 {
-  return aObj->Callback();
+  return aObj->CallbackOrNull();
 }
 
 // Helper for getting the callback JSObject* of a smart ptr around a
 // CallbackObject or a reference to a CallbackObject or something like
 // that.
 template <class T, bool isSmartPtr=IsSmartPtr<T>::value>
 struct GetCallbackFromCallbackObjectHelper
 {
--- a/dom/bindings/CallbackFunction.h
+++ b/dom/bindings/CallbackFunction.h
@@ -35,19 +35,19 @@ public:
   // See CallbackObject for an explanation of the arguments.
   explicit CallbackFunction(JS::Handle<JSObject*> aCallable,
                             JS::Handle<JSObject*> aAsyncStack,
                             nsIGlobalObject* aIncumbentGlobal)
     : CallbackObject(aCallable, aAsyncStack, aIncumbentGlobal)
   {
   }
 
-  JS::Handle<JSObject*> Callable() const
+  JS::Handle<JSObject*> CallableOrNull() const
   {
-    return Callback();
+    return CallbackOrNull();
   }
 
   JS::Handle<JSObject*> CallablePreserveColor() const
   {
     return CallbackPreserveColor();
   }
 
   bool HasGrayCallable() const
--- a/dom/bindings/CallbackObject.cpp
+++ b/dom/bindings/CallbackObject.cpp
@@ -85,18 +85,25 @@ CallbackObject::CallSetup::CallSetup(Cal
 
   // Compute the caller's subject principal (if necessary) early, before we
   // do anything that might perturb the relevant state.
   nsIPrincipal* webIDLCallerPrincipal = nullptr;
   if (aIsJSImplementedWebIDL) {
     webIDLCallerPrincipal = nsContentUtils::SubjectPrincipalOrSystemIfNativeCaller();
   }
 
+  JSObject* wrappedCallback = aCallback->CallbackPreserveColor();
+  if (!wrappedCallback) {
+    aRv.ThrowDOMException(NS_ERROR_DOM_NOT_SUPPORTED_ERR,
+      NS_LITERAL_CSTRING("Cannot execute callback from a nuked compartment."));
+    return;
+  }
+
   // First, find the real underlying callback.
-  JSObject* realCallback = js::UncheckedUnwrap(aCallback->CallbackPreserveColor());
+  JSObject* realCallback = js::UncheckedUnwrap(wrappedCallback);
   nsIGlobalObject* globalObject = nullptr;
 
   JSContext* cx;
   {
     // Bug 955660: we cannot do "proper" rooting here because we need the
     // global to get a context. Everything here is simple getters that cannot
     // GC, so just paper over the necessary dataflow inversion.
     JS::AutoSuppressGCAnalysis nogc;
@@ -149,23 +156,24 @@ CallbackObject::CallSetup::CallSetup(Cal
                            "incumbent global is being torn down."));
         return;
       }
       mAutoIncumbentScript.emplace(incumbent);
     }
 
     cx = mAutoEntryScript->cx();
 
-    // Unmark the callable (by invoking Callback() and not the CallbackPreserveColor()
-    // variant), and stick it in a Rooted before it can go gray again.
+    // Unmark the callable (by invoking CallbackOrNull() and not the
+    // CallbackPreserveColor() variant), and stick it in a Rooted before it can
+    // go gray again.
     // Nothing before us in this function can trigger a CC, so it's safe to wait
     // until here it do the unmark. This allows us to construct mRootedCallable
     // with the cx from mAutoEntryScript, avoiding the cost of finding another
     // JSContext. (Rooted<> does not care about requests or compartments.)
-    mRootedCallable.emplace(cx, aCallback->Callback());
+    mRootedCallable.emplace(cx, aCallback->CallbackOrNull());
   }
 
   // JS-implemented WebIDL is always OK to run, since it runs with Chrome
   // privileges anyway.
   if (mIsMainThread && !aIsJSImplementedWebIDL) {
     // Check that it's ok to run this callback at all.
     // Make sure to use realCallback to get the global of the callback object,
     // not the wrapper.
@@ -303,17 +311,17 @@ CallbackObjectHolderBase::ToXPCOMCallbac
   }
 
   // We don't init the AutoJSAPI with our callback because we don't want it
   // reporting errors to its global's onerror handlers.
   AutoJSAPI jsapi;
   jsapi.Init();
   JSContext* cx = jsapi.cx();
 
-  JS::Rooted<JSObject*> callback(cx, aCallback->Callback());
+  JS::Rooted<JSObject*> callback(cx, aCallback->CallbackOrNull());
 
   JSAutoCompartment ac(cx, callback);
   RefPtr<nsXPCWrappedJS> wrappedJS;
   nsresult rv =
     nsXPCWrappedJS::GetNewOrUsed(callback, aIID, getter_AddRefs(wrappedJS));
   if (NS_FAILED(rv) || !wrappedJS) {
     return nullptr;
   }
--- a/dom/bindings/CallbackObject.h
+++ b/dom/bindings/CallbackObject.h
@@ -74,17 +74,25 @@ public:
   // for that purpose.
   explicit CallbackObject(JS::Handle<JSObject*> aCallback,
                           JS::Handle<JSObject*> aAsyncStack,
                           nsIGlobalObject* aIncumbentGlobal)
   {
     Init(aCallback, aAsyncStack, aIncumbentGlobal);
   }
 
-  JS::Handle<JSObject*> Callback() const
+  // This is guaranteed to be non-null from the time the CallbackObject is
+  // created until JavaScript has had a chance to run. It will only return null
+  // after a JavaScript caller has called nukeSandbox on a Sandbox object, and
+  // the cycle collector has had a chance to run.
+  //
+  // This means that any native callee which receives a CallbackObject as an
+  // argument can safely rely on the callback being non-null so long as it
+  // doesn't trigger any scripts before it accesses it.
+  JS::Handle<JSObject*> CallbackOrNull() const
   {
     mCallback.exposeToActiveJS();
     return CallbackPreserveColor();
   }
 
   JSObject* GetCreationStack() const
   {
     return mCreationStack;
@@ -108,17 +116,17 @@ public:
   {
     // Calling fromMarkedLocation() is safe because we trace our mCallback, and
     // because the value of mCallback cannot change after if has been set.
     return JS::Handle<JSObject*>::fromMarkedLocation(mCallback.address());
   }
 
   /*
    * If the callback is known to be non-gray, then this method can be
-   * used instead of Callback() to avoid the overhead of
+   * used instead of CallbackOrNull() to avoid the overhead of
    * ExposeObjectToActiveJS().
    */
   JS::Handle<JSObject*> CallbackKnownNotGray() const
   {
     MOZ_ASSERT(!JS::ObjectIsMarkedGray(mCallback));
     return CallbackPreserveColor();
   }
 
@@ -155,20 +163,24 @@ protected:
   explicit CallbackObject(CallbackObject* aCallbackObject)
   {
     Init(aCallbackObject->mCallback, aCallbackObject->mCreationStack,
          aCallbackObject->mIncumbentGlobal);
   }
 
   bool operator==(const CallbackObject& aOther) const
   {
-    JSObject* thisObj =
-      js::UncheckedUnwrap(CallbackPreserveColor());
-    JSObject* otherObj =
-      js::UncheckedUnwrap(aOther.CallbackPreserveColor());
+    JSObject* wrappedThis = CallbackPreserveColor();
+    JSObject* wrappedOther = aOther.CallbackPreserveColor();
+    if (!wrappedThis || !wrappedOther) {
+      return this == &aOther;
+    }
+
+    JSObject* thisObj = js::UncheckedUnwrap(wrappedThis);
+    JSObject* otherObj = js::UncheckedUnwrap(wrappedOther);
     return thisObj == otherObj;
   }
 
 private:
   inline void InitNoHold(JSObject* aCallback, JSObject* aCreationStack,
                          nsIGlobalObject* aIncumbentGlobal)
   {
     MOZ_ASSERT(aCallback && !mCallback);
@@ -562,20 +574,20 @@ public:
 
   // But nullptr can't use the above template, because it doesn't know which S
   // to select.  So we need a special overload for nullptr.
   void operator=(decltype(nullptr) arg)
   {
     this->get().operator=(arg);
   }
 
-  // Codegen relies on being able to do Callback() on us.
-  JS::Handle<JSObject*> Callback() const
+  // Codegen relies on being able to do CallbackOrNull() on us.
+  JS::Handle<JSObject*> CallbackOrNull() const
   {
-    return this->get()->Callback();
+    return this->get()->CallbackOrNull();
   }
 
   ~RootedCallback()
   {
     // Ensure that our callback starts holding on to its own JS objects as
     // needed.  Having to null-check here when T is OwningNonNull is a bit
     // silly, but it's simpler than creating two separate RootedCallback
     // instantiations for OwningNonNull and RefPtr.
--- a/dom/bindings/Codegen.py
+++ b/dom/bindings/Codegen.py
@@ -4152,17 +4152,18 @@ class CastableObjectUnwrapper():
             self.substitution["codeOnFailure"] = fill(
                 """
                 // Be careful to not wrap random DOM objects here, even if
                 // they're wrapped in opaque security wrappers for some reason.
                 // XXXbz Wish we could check for a JS-implemented object
                 // that already has a content reflection...
                 if (!IsDOMObject(js::UncheckedUnwrap(${source}))) {
                   nsCOMPtr<nsIGlobalObject> contentGlobal;
-                  if (!GetContentGlobalForJSImplementedObject(cx, Callback(), getter_AddRefs(contentGlobal))) {
+                  JS::Handle<JSObject*> callback = CallbackOrNull();
+                  if (!(callback && GetContentGlobalForJSImplementedObject(cx, callback, getter_AddRefs(contentGlobal)))) {
                     $*{exceptionCode}
                   }
                   JS::Rooted<JSObject*> jsImplSourceObj(cx, ${source});
                   ${target} = new ${type}(jsImplSourceObj, contentGlobal);
                 } else {
                   $*{codeOnFailure}
                 }
                 """,
@@ -15013,21 +15014,21 @@ class CGJSImplClass(CGBindingImplClass):
         return fill(
             """
             JS::Rooted<JSObject*> obj(aCx, ${name}Binding::Wrap(aCx, this, aGivenProto));
             if (!obj) {
               return nullptr;
             }
 
             // Now define it on our chrome object
-            JSAutoCompartment ac(aCx, mImpl->Callback());
+            JSAutoCompartment ac(aCx, mImpl->CallbackOrNull());
             if (!JS_WrapObject(aCx, &obj)) {
               return nullptr;
             }
-            if (!JS_DefineProperty(aCx, mImpl->Callback(), "__DOM_IMPL__", obj, 0)) {
+            if (!JS_DefineProperty(aCx, mImpl->CallbackOrNull(), "__DOM_IMPL__", obj, 0)) {
               return nullptr;
             }
             return obj;
             """,
             name=self.descriptor.name)
 
     def getGetParentObjectReturnType(self):
         return "nsISupports*"
--- a/dom/bindings/ToJSValue.h
+++ b/dom/bindings/ToJSValue.h
@@ -127,17 +127,17 @@ ToJSValue(JSContext* aCx,
 MOZ_MUST_USE inline bool
 ToJSValue(JSContext* aCx,
           CallbackObject& aArgument,
           JS::MutableHandle<JS::Value> aValue)
 {
   // Make sure we're called in a compartment
   MOZ_ASSERT(JS::CurrentGlobalOrNull(aCx));
 
-  aValue.setObject(*aArgument.Callback());
+  aValue.setObjectOrNull(aArgument.CallbackOrNull());
 
   return MaybeWrapValue(aCx, aValue);
 }
 
 // Accept objects that inherit from nsWrapperCache (e.g. most
 // DOM objects).
 template <class T>
 MOZ_MUST_USE
--- a/dom/console/Console.cpp
+++ b/dom/console/Console.cpp
@@ -2277,21 +2277,26 @@ Console::NotifyHandler(JSContext* aCx, c
   MOZ_ASSERT(aCallData);
 
   if (!mConsoleEventNotifier) {
     return;
   }
 
   JS::Rooted<JS::Value> value(aCx);
 
+  JS::Rooted<JSObject*> callable(aCx, mConsoleEventNotifier->CallableOrNull());
+  if (NS_WARN_IF(!callable)) {
+    return;
+  }
+
   // aCx and aArguments are in the same compartment because this method is
   // called directly when a Console.something() runs.
   // mConsoleEventNotifier->Callable() is the scope where value will be sent to.
   if (NS_WARN_IF(!PopulateConsoleNotificationInTheTargetScope(aCx, aArguments,
-                                                              mConsoleEventNotifier->Callable(),
+                                                              callable,
                                                               &value,
                                                               aCallData))) {
     return;
   }
 
   JS::Rooted<JS::Value> ignored(aCx);
   mConsoleEventNotifier->Call(value, &ignored);
 }
--- a/dom/events/DOMEventTargetHelper.cpp
+++ b/dom/events/DOMEventTargetHelper.cpp
@@ -317,17 +317,17 @@ DOMEventTargetHelper::SetEventHandler(ns
 
 void
 DOMEventTargetHelper::GetEventHandler(nsIAtom* aType,
                                       JSContext* aCx,
                                       JS::Value* aValue)
 {
   EventHandlerNonNull* handler = GetEventHandler(aType, EmptyString());
   if (handler) {
-    *aValue = JS::ObjectValue(*handler->Callable());
+    *aValue = JS::ObjectOrNullValue(handler->CallableOrNull());
   } else {
     *aValue = JS::NullValue();
   }
 }
 
 nsresult
 DOMEventTargetHelper::PreHandleEvent(EventChainPreVisitor& aVisitor)
 {
--- a/dom/events/EventListenerService.cpp
+++ b/dom/events/EventListenerService.cpp
@@ -133,17 +133,17 @@ EventListenerInfo::GetJSVal(JSContext* a
     aAc.emplace(aCx, object);
     aJSVal.setObject(*object);
     return true;
   }
 
   nsCOMPtr<JSEventHandler> jsHandler = do_QueryInterface(mListener);
   if (jsHandler && jsHandler->GetTypedEventHandler().HasEventHandler()) {
     JS::Handle<JSObject*> handler =
-      jsHandler->GetTypedEventHandler().Ptr()->Callable();
+      jsHandler->GetTypedEventHandler().Ptr()->CallableOrNull();
     if (handler) {
       aAc.emplace(aCx, handler);
       aJSVal.setObject(*handler);
       return true;
     }
   }
   return false;
 }
--- a/dom/promise/Promise.cpp
+++ b/dom/promise/Promise.cpp
@@ -620,26 +620,26 @@ Promise::Then(JSContext* aCx,
   JS::Rooted<JSObject*> promise(aCx, PromiseObj());
   if (!JS_WrapObject(aCx, &promise)) {
     aRv.NoteJSContextException(aCx);
     return;
   }
 
   JS::Rooted<JSObject*> resolveCallback(aCx);
   if (aResolveCallback) {
-    resolveCallback = aResolveCallback->Callback();
+    resolveCallback = aResolveCallback->CallbackOrNull();
     if (!JS_WrapObject(aCx, &resolveCallback)) {
       aRv.NoteJSContextException(aCx);
       return;
     }
   }
 
   JS::Rooted<JSObject*> rejectCallback(aCx);
   if (aRejectCallback) {
-    rejectCallback = aRejectCallback->Callback();
+    rejectCallback = aRejectCallback->CallbackOrNull();
     if (!JS_WrapObject(aCx, &rejectCallback)) {
       aRv.NoteJSContextException(aCx);
       return;
     }
   }
 
   JS::Rooted<JSObject*> retval(aCx);
   retval = JS::CallOriginalPromiseThen(aCx, promise, resolveCallback,
--- a/dom/promise/PromiseCallback.cpp
+++ b/dom/promise/PromiseCallback.cpp
@@ -384,18 +384,20 @@ WrapperPromiseCallback::Call(JSContext* 
     }
     // XXXbz shouldn't this check be over in ResolveInternal anyway?
     if (valueObj == nextPromiseObj) {
       const char* fileName = nullptr;
       uint32_t lineNumber = 0;
 
       // Try to get some information about the callback to report a sane error,
       // but don't try too hard (only deals with scripted functions).
-      JS::Rooted<JSObject*> unwrapped(aCx,
-        js::CheckedUnwrap(mCallback->Callback()));
+      JS::Rooted<JSObject*> unwrapped(aCx, mCallback->CallbackOrNull());
+      if (unwrapped) {
+        js::CheckedUnwrap(&unwrapped);
+      }
 
       if (unwrapped) {
         JSAutoCompartment ac(aCx, unwrapped);
         if (JS_ObjectIsFunction(aCx, unwrapped)) {
           JS::Rooted<JS::Value> asValue(aCx, JS::ObjectValue(*unwrapped));
           JS::Rooted<JSFunction*> func(aCx, JS_ValueToFunction(aCx, asValue));
 
           MOZ_ASSERT(func);
--- a/dom/workers/WorkerPrivate.cpp
+++ b/dom/workers/WorkerPrivate.cpp
@@ -1283,17 +1283,17 @@ private:
   {
     // Silence bad assertions.
   }
 
   virtual bool
   WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override
   {
     JS::Rooted<JSObject*> global(aCx, JS::CurrentGlobalOrNull(aCx));
-    JS::Rooted<JS::Value> callable(aCx, JS::ObjectValue(*mHandler->Callable()));
+    JS::Rooted<JS::Value> callable(aCx, JS::ObjectOrNullValue(mHandler->CallableOrNull()));
     JS::HandleValueArray args = JS::HandleValueArray::empty();
     JS::Rooted<JS::Value> rval(aCx);
     if (!JS_CallFunctionValue(aCx, global, callable, args, &rval)) {
       // Just return false; WorkerRunnable::Run will report the exception.
       return false;
     }
 
     return true;
--- a/xpcom/base/CycleCollectedJSContext.cpp
+++ b/xpcom/base/CycleCollectedJSContext.cpp
@@ -933,17 +933,18 @@ public:
   virtual ~PromiseJobRunnable()
   {
   }
 
 protected:
   NS_IMETHOD
   Run() override
   {
-    nsIGlobalObject* global = xpc::NativeGlobal(mCallback->CallbackPreserveColor());
+    JSObject* callback = mCallback->CallbackPreserveColor();
+    nsIGlobalObject* global = callback ? xpc::NativeGlobal(callback) : nullptr;
     if (global && !global->IsDying()) {
       mCallback->Call("promise callback");
     }
     return NS_OK;
   }
 
 private:
   RefPtr<PromiseJobCallback> mCallback;