Bug 1246091 - patch 7/7 - Correct use of JSCompartment in Console.cpp, r=bz
authorAndrea Marchesini <amarchesini@mozilla.com>
Wed, 23 Mar 2016 22:55:07 +0100
changeset 290140 cb19fc3357e279a886abeaee475dfdb966fcd11c
parent 290139 94c7a77724d36b22b64afb24f50ff3e719de2b23
child 290141 edd188f125c5e8d7f1b2ff2a03b0e57d249afbc6
push id30114
push usercbook@mozilla.com
push dateThu, 24 Mar 2016 15:15:54 +0000
treeherdermozilla-central@24c5fbde4488 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbz
bugs1246091
milestone48.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 1246091 - patch 7/7 - Correct use of JSCompartment in Console.cpp, r=bz
dom/base/Console.cpp
dom/base/Console.h
--- a/dom/base/Console.cpp
+++ b/dom/base/Console.cpp
@@ -112,16 +112,18 @@ public:
 
     // We must be registered before doing any JS operation otherwise it can
     // happen that mCopiedArguments are not correctly traced.
     aConsole->StoreCallData(this);
 
     mMethodName = aName;
     mMethodString = aString;
 
+    mGlobal = JS::CurrentGlobalOrNull(aCx);
+
     for (uint32_t i = 0; i < aArguments.Length(); ++i) {
       if (NS_WARN_IF(!mCopiedArguments.AppendElement(aArguments[i]))) {
         aConsole->UnstoreCallData(this);
         return false;
       }
     }
 
     return true;
@@ -166,25 +168,29 @@ public:
   Trace(const TraceCallbacks& aCallbacks, void* aClosure)
   {
     AssertIsOnOwningThread();
 
     ConsoleCallData* tmp = this;
     for (uint32_t i = 0; i < mCopiedArguments.Length(); ++i) {
       NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK(mCopiedArguments[i])
     }
+
+    NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK(mGlobal);
   }
 
   void
   AssertIsOnOwningThread() const
   {
     MOZ_ASSERT(mOwningThread);
     MOZ_ASSERT(PR_GetCurrentThread() == mOwningThread);
   }
 
+  JS::Heap<JSObject*> mGlobal;
+
   // This is a copy of the arguments we received from the DOM bindings. Console
   // object traces them because this ConsoleCallData calls
   // RegisterConsoleCallData() in the Initialize().
   nsTArray<JS::Heap<JS::Value>> mCopiedArguments;
 
   Console::MethodName mMethodName;
   bool mPrivate;
   int64_t mTimeStamp;
@@ -312,19 +318,19 @@ public:
   virtual
   ~ConsoleRunnable()
   {
     // Clear the StructuredCloneHolderBase class.
     Clear();
   }
 
   bool
-  Dispatch(JS::Handle<JSObject*> aGlobal)
+  Dispatch(JSContext* aCx)
   {
-    if (!DispatchInternal(aGlobal)) {
+    if (!DispatchInternal(aCx)) {
       ReleaseData();
       return false;
     }
 
     return true;
   }
 
   virtual bool Notify(JSContext* aCx, workers::Status aStatus) override
@@ -353,23 +359,21 @@ private:
       RunWithWindow(window);
     }
 
     PostDispatch();
     return NS_OK;
   }
 
   bool
-  DispatchInternal(JS::Handle<JSObject*> aGlobal)
+  DispatchInternal(JSContext* aCx)
   {
     mWorkerPrivate->AssertIsOnWorkerThread();
 
-    JSContext* cx = mWorkerPrivate->GetJSContext();
-
-    if (NS_WARN_IF(!PreDispatch(cx, aGlobal))) {
+    if (NS_WARN_IF(!PreDispatch(aCx))) {
       return false;
     }
 
     if (NS_WARN_IF(!mWorkerPrivate->AddFeature(this))) {
       return false;
     }
 
     if (NS_WARN_IF(NS_FAILED(NS_DispatchToMainThread(this)))) {
@@ -466,17 +470,17 @@ private:
     JSAutoCompartment ac(cx, global);
 
     RunConsole(cx, nullptr, nullptr);
   }
 
 protected:
   // This method is called in the owning thread of the Console object.
   virtual bool
-  PreDispatch(JSContext* aCx, JS::Handle<JSObject*> aGlobal) = 0;
+  PreDispatch(JSContext* aCx) = 0;
 
   // This method is called in the main-thread.
   virtual void
   RunConsole(JSContext* aCx, nsPIDOMWindowOuter* aOuterWindow,
              nsPIDOMWindowInner* aInnerWindow) = 0;
 
   // This method is called in the owning thread of the Console object.
   virtual void
@@ -562,23 +566,22 @@ public:
 
 private:
   ~ConsoleCallDataRunnable()
   {
     MOZ_ASSERT(!mCallData);
   }
 
   bool
-  PreDispatch(JSContext* aCx, JS::Handle<JSObject*> aGlobal) override
+  PreDispatch(JSContext* aCx) override
   {
     mWorkerPrivate->AssertIsOnWorkerThread();
     mCallData->AssertIsOnOwningThread();
 
     ClearException ce(aCx);
-    JSAutoCompartment ac(aCx, aGlobal);
 
     JS::Rooted<JSObject*> arguments(aCx,
       JS_NewArrayObject(aCx, mCallData->mCopiedArguments.Length()));
     if (NS_WARN_IF(!arguments)) {
       return false;
     }
 
     JS::Rooted<JS::Value> arg(aCx);
@@ -689,19 +692,17 @@ private:
 
       if (!values.AppendElement(value, fallible)) {
         return;
       }
     }
 
     MOZ_ASSERT(values.Length() == length);
 
-    JS::Rooted<JSObject*> global(aCx, JS::CurrentGlobalOrNull(aCx));
-
-    mConsole->ProcessCallData(mCallData, global, values);
+    mConsole->ProcessCallData(aCx, mCallData, values);
   }
 
   RefPtr<ConsoleCallData> mCallData;
 };
 
 // This runnable calls ProfileMethod() on the console on the main-thread.
 class ConsoleProfileRunnable final : public ConsoleRunnable
 {
@@ -712,27 +713,20 @@ public:
     , mAction(aAction)
     , mArguments(aArguments)
   {
     MOZ_ASSERT(aConsole);
   }
 
 private:
   bool
-  PreDispatch(JSContext* aCx, JS::Handle<JSObject*> aGlobal) override
+  PreDispatch(JSContext* aCx) override
   {
     ClearException ce(aCx);
 
-    JS::Rooted<JSObject*> global(aCx, aGlobal);
-    if (NS_WARN_IF(!global)) {
-      return false;
-    }
-
-    JSAutoCompartment ac(aCx, global);
-
     JS::Rooted<JSObject*> arguments(aCx,
       JS_NewArrayObject(aCx, mArguments.Length()));
     if (NS_WARN_IF(!arguments)) {
       return false;
     }
 
     JS::Rooted<JS::Value> arg(aCx);
     for (uint32_t i = 0; i < mArguments.Length(); ++i) {
@@ -1096,17 +1090,17 @@ Console::ProfileMethod(JSContext* aCx, c
   MOZ_ASSERT(mStatus == eInitialized);
 
   if (!NS_IsMainThread()) {
     // Here we are in a worker thread.
     RefPtr<ConsoleProfileRunnable> runnable =
       new ConsoleProfileRunnable(this, aAction, aData);
 
     JS::Rooted<JSObject*> global(aCx, JS::CurrentGlobalOrNull(aCx));
-    runnable->Dispatch(global);
+    runnable->Dispatch(aCx);
     return;
   }
 
   ClearException ce(aCx);
 
   RootedDictionary<ConsoleProfileEvent> event(aCx);
   event.mAction = aAction;
 
@@ -1368,35 +1362,33 @@ Console::Method(JSContext* aCx, MethodNa
     if (callData->mTopStackFrame) {
       frame = *callData->mTopStackFrame;
     }
 
     callData->mCountValue = IncreaseCounter(aCx, frame, aData,
                                             callData->mCountLabel);
   }
 
-  JS::Rooted<JSObject*> global(aCx, JS::CurrentGlobalOrNull(aCx));
-
   if (NS_IsMainThread()) {
     callData->SetIDs(mOuterID, mInnerID);
-    ProcessCallData(callData, global, aData);
+    ProcessCallData(aCx, callData, aData);
 
     // Just because we don't want to expose
     // retrieveConsoleEvents/setConsoleEventHandler to main-thread, we can
     // cleanup the mCallDataStorage:
     UnstoreCallData(callData);
     return;
   }
 
   // We do this only in workers for now.
-  NotifyHandler(aCx, global, aData, callData);
+  NotifyHandler(aCx, aData, callData);
 
   RefPtr<ConsoleCallDataRunnable> runnable =
     new ConsoleCallDataRunnable(this, callData);
-  NS_WARN_IF(!runnable->Dispatch(global));
+  NS_WARN_IF(!runnable->Dispatch(aCx));
 }
 
 // We store information to lazily compute the stack in the reserved slots of
 // LazyStackGetter.  The first slot always stores a JS object: it's either the
 // JS wrapper of the nsIStackFrame or the actual reified stack representation.
 // The second slot is a PrivateValue() holding an nsIStackFrame* when we haven't
 // reified the stack yet, or an UndefinedValue() otherwise.
 enum {
@@ -1435,30 +1427,39 @@ LazyStackGetter(JSContext* aCx, unsigned
   js::SetFunctionNativeReserved(callee, SLOT_STACKOBJ, stackVal);
   js::SetFunctionNativeReserved(callee, SLOT_RAW_STACK, JS::UndefinedValue());
 
   args.rval().set(stackVal);
   return true;
 }
 
 void
-Console::ProcessCallData(ConsoleCallData* aData, JS::Handle<JSObject*> aGlobal,
+Console::ProcessCallData(JSContext* aCx, ConsoleCallData* aData,
                          const Sequence<JS::Value>& aArguments)
 {
   AssertIsOnMainThread();
   MOZ_ASSERT(aData);
 
-  AutoJSAPI jsapi;
-  if (!jsapi.Init(aGlobal)) {
-    return;
-  }
-  JSContext* cx = jsapi.cx();
-
-  JS::Rooted<JS::Value> eventValue(cx);
-  if (NS_WARN_IF(!PopulateEvent(cx, aGlobal, aArguments, &eventValue, aData))) {
+  JS::Rooted<JS::Value> eventValue(aCx);
+
+  // We want to create a console event object and pass it to our
+  // nsIConsoleAPIStorage implementation.  We want to define some accessor
+  // properties on this object, and those will need to keep an nsIStackFrame
+  // alive.  But nsIStackFrame cannot be wrapped in an untrusted scope.  And
+  // further, passing untrusted objects to system code is likely to run afoul of
+  // Object Xrays.  So we want to wrap in a system-principal scope here.  But
+  // which one?  We could cheat and try to get the underlying JSObject* of
+  // mStorage, but that's a bit fragile.  Instead, we just use the junk scope,
+  // with explicit permission from the XPConnect module owner.  If you're
+  // tempted to do that anywhere else, talk to said module owner first.
+
+  // aCx and aArguments are in the same compartment.
+  if (NS_WARN_IF(!PopulateConsoleObjectInTheTargetScope(aCx, aArguments,
+                                                        xpc::PrivilegedJunkScope(),
+                                                        &eventValue, aData))) {
     return;
   }
 
   if (!mStorage) {
     mStorage = do_GetService("@mozilla.org/consoleAPI-storage;1");
   }
 
   if (!mStorage) {
@@ -1479,35 +1480,34 @@ Console::ProcessCallData(ConsoleCallData
   }
 
   if (NS_FAILED(mStorage->RecordEvent(innerID, outerID, eventValue))) {
     NS_WARNING("Failed to record a console event.");
   }
 }
 
 bool
-Console::PopulateEvent(JSContext* aCx,
-                       JS::Handle<JSObject*> aGlobal,
-                       const Sequence<JS::Value>& aArguments,
-                       JS::MutableHandle<JS::Value> aEventValue,
-                       ConsoleCallData* aData) const
+Console::PopulateConsoleObjectInTheTargetScope(JSContext* aCx,
+                                               const Sequence<JS::Value>& aArguments,
+                                               JSObject* aTargetScope,
+                                               JS::MutableHandle<JS::Value> aEventValue,
+                                               ConsoleCallData* aData) const
 {
   MOZ_ASSERT(aCx);
   MOZ_ASSERT(aData);
+  MOZ_ASSERT(aTargetScope);
 
   ConsoleStackEntry frame;
   if (aData->mTopStackFrame) {
     frame = *aData->mTopStackFrame;
   }
 
   ClearException ce(aCx);
   RootedDictionary<ConsoleEvent> event(aCx);
 
-  JSAutoCompartment ac(aCx, aGlobal);
-
   event.mID.Construct();
   event.mInnerID.Construct();
 
   if (aData->mIDType == ConsoleCallData::eString) {
     event.mID.Value().SetAsString() = aData->mOuterIDString;
     event.mInnerID.Value().SetAsString() = aData->mInnerIDString;
   } else if (aData->mIDType == ConsoleCallData::eNumber) {
     event.mID.Value().SetAsUnsignedLongLong() = aData->mOuterIDNumber;
@@ -1585,27 +1585,17 @@ Console::PopulateEvent(JSContext* aCx,
                                         aData->mStopTimerStatus);
   }
 
   else if (aData->mMethodName == MethodCount) {
     event.mCounter = CreateCounterValue(aCx, aData->mCountLabel,
                                         aData->mCountValue);
   }
 
-  // We want to create a console event object and pass it to our
-  // nsIConsoleAPIStorage implementation.  We want to define some accessor
-  // properties on this object, and those will need to keep an nsIStackFrame
-  // alive.  But nsIStackFrame cannot be wrapped in an untrusted scope.  And
-  // further, passing untrusted objects to system code is likely to run afoul of
-  // Object Xrays.  So we want to wrap in a system-principal scope here.  But
-  // which one?  We could cheat and try to get the underlying JSObject* of
-  // mStorage, but that's a bit fragile.  Instead, we just use the junk scope,
-  // with explicit permission from the XPConnect module owner.  If you're
-  // tempted to do that anywhere else, talk to said module owner first.
-  JSAutoCompartment ac2(aCx, xpc::PrivilegedJunkScope());
+  JSAutoCompartment ac2(aCx, aTargetScope);
 
   if (NS_WARN_IF(!ToJSValue(aCx, event, aEventValue))) {
     return false;
   }
 
   JS::Rooted<JSObject*> eventObj(aCx, &aEventValue.toObject());
   if (NS_WARN_IF(!JS_DefineProperty(aCx, eventObj, "wrappedJSObject", eventObj,
                                     JSPROP_ENUMERATE))) {
@@ -2257,64 +2247,73 @@ Console::ReleaseCallData(ConsoleCallData
   MOZ_ASSERT(aCallData);
   MOZ_ASSERT(aCallData->mStatus == ConsoleCallData::eToBeDeleted);
   MOZ_ASSERT(mCallDataStoragePending.Contains(aCallData));
 
   mCallDataStoragePending.RemoveElement(aCallData);
 }
 
 void
-Console::NotifyHandler(JSContext* aCx, JS::Handle<JSObject*> aGlobal,
-                       const Sequence<JS::Value>& aArguments,
+Console::NotifyHandler(JSContext* aCx, const Sequence<JS::Value>& aArguments,
                        ConsoleCallData* aCallData) const
 {
   AssertIsOnOwningThread();
   MOZ_ASSERT(!NS_IsMainThread());
   MOZ_ASSERT(aCallData);
 
   if (!mConsoleEventNotifier) {
     return;
   }
 
-  JSAutoCompartment ac(aCx, mConsoleEventNotifier->Callable());
-
   JS::Rooted<JS::Value> value(aCx);
-  if (NS_WARN_IF(!PopulateEvent(aCx, mConsoleEventNotifier->Callable(),
-                                aArguments, &value, aCallData))) {
+
+  // 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(!PopulateConsoleObjectInTheTargetScope(aCx, aArguments,
+                                                        mConsoleEventNotifier->Callable(),
+                                                        &value, aCallData))) {
     return;
   }
 
   JS::Rooted<JS::Value> ignored(aCx);
   mConsoleEventNotifier->Call(value, &ignored);
 }
 
 void
 Console::RetrieveConsoleEvents(JSContext* aCx, nsTArray<JS::Value>& aEvents,
                                ErrorResult& aRv)
 {
   AssertIsOnOwningThread();
 
   // We don't want to expose this functionality to main-thread yet.
   MOZ_ASSERT(!NS_IsMainThread());
 
-  JS::Rooted<JSObject*> global(aCx, JS::CurrentGlobalOrNull(aCx));
+  JS::Rooted<JSObject*> targetScope(aCx, JS::CurrentGlobalOrNull(aCx));
 
   for (uint32_t i = 0; i < mCallDataStorage.Length(); ++i) {
     JS::Rooted<JS::Value> value(aCx);
 
+    JS::Rooted<JSObject*> sequenceScope(aCx, mCallDataStorage[i]->mGlobal);
+    JSAutoCompartment ac(aCx, sequenceScope);
+
     Sequence<JS::Value> sequence;
     SequenceRooter<JS::Value> arguments(aCx, &sequence);
 
     if (!mCallDataStorage[i]->PopulateArgumentsSequence(sequence)) {
       aRv.Throw(NS_ERROR_OUT_OF_MEMORY);
       return;
     }
 
-    if (NS_WARN_IF(!PopulateEvent(aCx, global, sequence, &value,
-                                  mCallDataStorage[i]))) {
+    // Here we have aCx and sequence in the same compartment.
+    // targetScope is the destination scope and value will be populated in its
+    // compartment.
+    if (NS_WARN_IF(!PopulateConsoleObjectInTheTargetScope(aCx, sequence,
+                                                          targetScope, &value,
+                                                          mCallDataStorage[i]))) {
       aRv.Throw(NS_ERROR_FAILURE);
       return;
     }
 
     aEvents.AppendElement(value);
   }
 }
 
--- a/dom/base/Console.h
+++ b/dom/base/Console.h
@@ -158,43 +158,55 @@ private:
     MethodAssert,
     MethodCount
   };
 
   void
   Method(JSContext* aCx, MethodName aName, const nsAString& aString,
          const Sequence<JS::Value>& aData);
 
+  // This method must receive aCx and aArguments in the same JSCompartment.
   void
-  ProcessCallData(ConsoleCallData* aData,
-                  JS::Handle<JSObject*> aGlobal,
+  ProcessCallData(JSContext* aCx,
+                  ConsoleCallData* aData,
                   const Sequence<JS::Value>& aArguments);
 
   void
   StoreCallData(ConsoleCallData* aData);
 
   void
   UnstoreCallData(ConsoleCallData* aData);
 
   // Read in Console.cpp how this method is used.
   void
   ReleaseCallData(ConsoleCallData* aCallData);
 
+  // aCx and aArguments must be in the same JS compartment.
   void
   NotifyHandler(JSContext* aCx,
-                JS::Handle<JSObject*> aGlobal,
                 const Sequence<JS::Value>& aArguments,
                 ConsoleCallData* aData) const;
 
+  // PopulateConsoleObjectInTheTargetScope receives aCx and aArguments in the
+  // same JS compartment and populates the ConsoleEvent object (aValue) in the
+  // aTargetScope.
+  // aTargetScope can be:
+  // - the system-principal scope when we want to dispatch the ConsoleEvent to
+  //   nsIConsoleAPIStorage (See the comment in Console.cpp about the use of
+  //   xpc::PrivilegedJunkScope()
+  // - the mConsoleEventNotifier->Callable() scope when we want to notify this
+  //   handler about a new ConsoleEvent.
+  // - It can be the global from the JSContext when RetrieveConsoleEvents is
+  //   called.
   bool
-  PopulateEvent(JSContext* aCx,
-                JS::Handle<JSObject*> aGlobal,
-                const Sequence<JS::Value>& aArguments,
-                JS::MutableHandle<JS::Value> aValue,
-                ConsoleCallData* aData) const;
+  PopulateConsoleObjectInTheTargetScope(JSContext* aCx,
+                                        const Sequence<JS::Value>& aArguments,
+                                        JSObject* aTargetScope,
+                                        JS::MutableHandle<JS::Value> aValue,
+                                        ConsoleCallData* aData) const;
 
   // If the first JS::Value of the array is a string, this method uses it to
   // format a string. The supported sequences are:
   //   %s    - string
   //   %d,%i - integer
   //   %f    - double
   //   %o,%O - a JS object.
   //   %c    - style string.