Bug 1308919 - Don't make Handles to Heap<T> as it avoids the read barrier r=bz
authorJon Coppeard <jcoppeard@mozilla.com>
Fri, 14 Oct 2016 09:45:28 +0100
changeset 360845 22feffdddcd0d6bade3e267820db292e470f2d1e
parent 360844 62817c443347073468fe18e5bde744b9108f8285
child 360846 25ec37510c7fbd5ea0d66339e970aed89068dc1b
push id6795
push userjlund@mozilla.com
push dateMon, 23 Jan 2017 14:19:46 +0000
treeherdermozilla-beta@76101b503191 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbz
bugs1308919
milestone52.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 1308919 - Don't make Handles to Heap<T> as it avoids the read barrier r=bz
dom/bindings/Codegen.py
dom/xbl/nsXBLProtoImplMethod.cpp
dom/xbl/nsXBLProtoImplProperty.cpp
dom/xul/XULDocument.cpp
dom/xul/nsXULContentSink.cpp
dom/xul/nsXULElement.cpp
dom/xul/nsXULElement.h
js/xpconnect/src/xpcpublic.h
--- a/dom/bindings/Codegen.py
+++ b/dom/bindings/Codegen.py
@@ -3106,18 +3106,24 @@ class CGGetPerInterfaceObject(CGAbstract
             }
 
             /*
              * The object might _still_ be null, but that's OK.
              *
              * Calling fromMarkedLocation() is safe because protoAndIfaceCache is
              * traced by TraceProtoAndIfaceCache() and its contents are never
              * changed after they have been set.
+             *
+             * Calling address() avoids the read read barrier that does grey
+             * unmarking, but it's not possible for the object to be gray here.
              */
-            return JS::Handle<JSObject*>::fromMarkedLocation(protoAndIfaceCache.EntrySlotMustExist(${id}).address());
+
+            const JS::Heap<JSObject*>& entrySlot = protoAndIfaceCache.EntrySlotMustExist(${id});
+            MOZ_ASSERT_IF(entrySlot, !JS::ObjectIsMarkedGray(entrySlot));
+            return JS::Handle<JSObject*>::fromMarkedLocation(entrySlot.address());
             """,
             id=self.id)
 
 
 class CGGetProtoObjectHandleMethod(CGGetPerInterfaceObject):
     """
     A method for getting the interface prototype object.
     """
--- a/dom/xbl/nsXBLProtoImplMethod.cpp
+++ b/dom/xbl/nsXBLProtoImplMethod.cpp
@@ -248,21 +248,17 @@ nsXBLProtoImplMethod::Write(nsIObjectOut
   MOZ_ASSERT(IsCompiled());
   if (GetCompiledMethodPreserveColor()) {
     nsresult rv = aStream->Write8(XBLBinding_Serialize_Method);
     NS_ENSURE_SUCCESS(rv, rv);
 
     rv = aStream->WriteWStringZ(mName);
     NS_ENSURE_SUCCESS(rv, rv);
 
-    // Calling fromMarkedLocation() is safe because mMethod is traced by the
-    // Trace() method above, and because its value is never changed after it has
-    // been set to a compiled method.
-    JS::Handle<JSObject*> method =
-      JS::Handle<JSObject*>::fromMarkedLocation(mMethod.AsHeapObject().address());
+    JS::Rooted<JSObject*> method(RootingCx(), GetCompiledMethod());
     return XBL_SerializeFunction(aStream, method);
   }
 
   return NS_OK;
 }
 
 nsresult
 nsXBLProtoImplAnonymousMethod::Execute(nsIContent* aBoundElement, JSAddonId* aAddonId)
@@ -337,19 +333,15 @@ nsXBLProtoImplAnonymousMethod::Write(nsI
   MOZ_ASSERT(IsCompiled());
   if (GetCompiledMethodPreserveColor()) {
     nsresult rv = aStream->Write8(aType);
     NS_ENSURE_SUCCESS(rv, rv);
 
     rv = aStream->WriteWStringZ(mName);
     NS_ENSURE_SUCCESS(rv, rv);
 
-    // Calling fromMarkedLocation() is safe because mMethod is traced by the
-    // Trace() method above, and because its value is never changed after it has
-    // been set to a compiled method.
-    JS::Handle<JSObject*> method =
-      JS::Handle<JSObject*>::fromMarkedLocation(mMethod.AsHeapObject().address());
+    JS::Rooted<JSObject*> method(RootingCx(), GetCompiledMethod());
     rv = XBL_SerializeFunction(aStream, method);
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
   return NS_OK;
 }
--- a/dom/xbl/nsXBLProtoImplProperty.cpp
+++ b/dom/xbl/nsXBLProtoImplProperty.cpp
@@ -15,21 +15,21 @@
 #include "nsXBLPrototypeBinding.h"
 #include "nsXBLSerialize.h"
 #include "xpcpublic.h"
 
 using namespace mozilla;
 using namespace mozilla::dom;
 
 nsXBLProtoImplProperty::nsXBLProtoImplProperty(const char16_t* aName,
-                                               const char16_t* aGetter, 
+                                               const char16_t* aGetter,
                                                const char16_t* aSetter,
                                                const char16_t* aReadOnly,
                                                uint32_t aLineNumber) :
-  nsXBLProtoImplMember(aName), 
+  nsXBLProtoImplMember(aName),
   mJSAttributes(JSPROP_ENUMERATE)
 #ifdef DEBUG
   , mIsCompiled(false)
 #endif
 {
   MOZ_COUNT_CTOR(nsXBLProtoImplProperty);
 
   if (aReadOnly) {
@@ -305,17 +305,17 @@ nsXBLProtoImplProperty::Read(nsIObjectIn
   if (aType == XBLBinding_Serialize_GetterProperty ||
       aType == XBLBinding_Serialize_GetterSetterProperty) {
     nsresult rv = XBL_DeserializeFunction(aStream, &getterObject);
     NS_ENSURE_SUCCESS(rv, rv);
 
     mJSAttributes |= JSPROP_GETTER | JSPROP_SHARED;
   }
   mGetter.SetJSFunction(getterObject);
-  
+
   JS::Rooted<JSObject*> setterObject(cx);
   if (aType == XBLBinding_Serialize_SetterProperty ||
       aType == XBLBinding_Serialize_GetterSetterProperty) {
     nsresult rv = XBL_DeserializeFunction(aStream, &setterObject);
     NS_ENSURE_SUCCESS(rv, rv);
 
     mJSAttributes |= JSPROP_SETTER | JSPROP_SHARED;
   }
@@ -347,29 +347,24 @@ nsXBLProtoImplProperty::Write(nsIObjectO
     type |= XBLBinding_Serialize_ReadOnly;
   }
 
   nsresult rv = aStream->Write8(type);
   NS_ENSURE_SUCCESS(rv, rv);
   rv = aStream->WriteWStringZ(mName);
   NS_ENSURE_SUCCESS(rv, rv);
 
-  // The calls to fromMarkedLocation() below are safe because mSetter and
-  // mGetter are traced by the Trace() method above, and because their values
-  // are never changed after they have been set to a compiled function.
   MOZ_ASSERT_IF(mJSAttributes & (JSPROP_GETTER | JSPROP_SETTER), mIsCompiled);
 
   if (mJSAttributes & JSPROP_GETTER) {
-    JS::Handle<JSObject*> function =
-      JS::Handle<JSObject*>::fromMarkedLocation(mGetter.AsHeapObject().address());
+    JS::Rooted<JSObject*> function(RootingCx(), mGetter.GetJSFunction());
     rv = XBL_SerializeFunction(aStream, function);
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
   if (mJSAttributes & JSPROP_SETTER) {
-     JS::Handle<JSObject*> function =
-      JS::Handle<JSObject*>::fromMarkedLocation(mSetter.AsHeapObject().address());
+    JS::Rooted<JSObject*> function(RootingCx(), mSetter.GetJSFunction());
     rv = XBL_SerializeFunction(aStream, function);
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
   return NS_OK;
 }
--- a/dom/xul/XULDocument.cpp
+++ b/dom/xul/XULDocument.cpp
@@ -2839,17 +2839,17 @@ XULDocument::ResumeWalk()
                     // stale and must be reloaded.
                     bool blocked;
                     rv = LoadScript(scriptproto, &blocked);
                     // If the script cannot be loaded, just keep going!
 
                     if (NS_SUCCEEDED(rv) && blocked)
                         return NS_OK;
                 }
-                else if (scriptproto->GetScriptObject()) {
+                else if (scriptproto->HasScriptObject()) {
                     // An inline script
                     rv = ExecuteScript(scriptproto);
                     if (NS_FAILED(rv)) return rv;
                 }
             }
             break;
 
             case nsXULPrototypeNode::eType_Text: {
@@ -3205,17 +3205,17 @@ XULDocument::ReportMissingOverlay(nsIURI
 nsresult
 XULDocument::LoadScript(nsXULPrototypeScript* aScriptProto, bool* aBlock)
 {
     // Load a transcluded script
     nsresult rv;
 
     bool isChromeDoc = IsChromeURI(mDocumentURI);
 
-    if (isChromeDoc && aScriptProto->GetScriptObject()) {
+    if (isChromeDoc && aScriptProto->HasScriptObject()) {
         rv = ExecuteScript(aScriptProto);
 
         // Ignore return value from execution, and don't block
         *aBlock = false;
         return NS_OK;
     }
 
     // Try the XUL script cache, in case two XUL documents source the same
@@ -3228,17 +3228,17 @@ XULDocument::LoadScript(nsXULPrototypeSc
             nsXULPrototypeCache::GetInstance()->GetScript(
                                    aScriptProto->mSrcURI);
         if (newScriptObject) {
             // The script language for a proto must remain constant - we
             // can't just change it for this unexpected language.
             aScriptProto->Set(newScriptObject);
         }
 
-        if (aScriptProto->GetScriptObject()) {
+        if (aScriptProto->HasScriptObject()) {
             rv = ExecuteScript(aScriptProto);
 
             // Ignore return value from execution, and don't block
             *aBlock = false;
             return NS_OK;
         }
     }
 
@@ -3348,17 +3348,17 @@ XULDocument::OnStreamComplete(nsIStreamL
             // completes.
             JS::SourceBufferHolder srcBuf(mOffThreadCompileStringBuf,
                                           mOffThreadCompileStringLength,
                                           JS::SourceBufferHolder::GiveOwnership);
             mOffThreadCompileStringBuf = nullptr;
             mOffThreadCompileStringLength = 0;
 
             rv = mCurrentScriptProto->Compile(srcBuf, uri, 1, this, this);
-            if (NS_SUCCEEDED(rv) && !mCurrentScriptProto->GetScriptObject()) {
+            if (NS_SUCCEEDED(rv) && !mCurrentScriptProto->HasScriptObject()) {
                 // We will be notified via OnOffThreadCompileComplete when the
                 // compile finishes. Keep the contents of the compiled script
                 // alive until the compilation finishes.
                 mOffThreadCompiling = true;
                 // If the JS engine did not take the source buffer, then take
                 // it back here to ensure it remains alive.
                 mOffThreadCompileStringBuf = srcBuf.take();
                 if (mOffThreadCompileStringBuf) {
@@ -3373,17 +3373,17 @@ XULDocument::OnStreamComplete(nsIStreamL
     return OnScriptCompileComplete(mCurrentScriptProto->GetScriptObject(), rv);
 }
 
 NS_IMETHODIMP
 XULDocument::OnScriptCompileComplete(JSScript* aScript, nsresult aStatus)
 {
     // When compiling off thread the script will not have been attached to the
     // script proto yet.
-    if (aScript && !mCurrentScriptProto->GetScriptObject())
+    if (aScript && !mCurrentScriptProto->HasScriptObject())
         mCurrentScriptProto->Set(aScript);
 
     // Allow load events to be fired once off thread compilation finishes.
     if (mOffThreadCompiling) {
         mOffThreadCompiling = false;
         UnblockOnload(false);
     }
 
@@ -3426,21 +3426,21 @@ XULDocument::OnScriptCompileComplete(JSS
         // containing a companion nsXULPrototypeScript node that owns a
         // GC root protecting the script object.  Otherwise, the script
         // cache entry will dangle once the uncached prototype document
         // is released when its owning XULDocument is unloaded.
         //
         // (See http://bugzilla.mozilla.org/show_bug.cgi?id=98207 for
         // the true crime story.)
         bool useXULCache = nsXULPrototypeCache::GetInstance()->IsEnabled();
-  
-        if (useXULCache && IsChromeURI(mDocumentURI) && scriptProto->GetScriptObject()) {
+
+        if (useXULCache && IsChromeURI(mDocumentURI) && scriptProto->HasScriptObject()) {
+            JS::Rooted<JSScript*> script(RootingCx(), scriptProto->GetScriptObject());
             nsXULPrototypeCache::GetInstance()->PutScript(
-                               scriptProto->mSrcURI,
-                               scriptProto->GetScriptObject());
+                               scriptProto->mSrcURI, script);
         }
 
         if (mIsWritingFastLoad && mCurrentPrototype != mMasterPrototype) {
             // If we are loading an overlay script, try to serialize
             // it to the FastLoad file here.  Master scripts will be
             // serialized when the master prototype document gets
             // written, at the bottom of ResumeWalk.  That way, master
             // out-of-line scripts are serialized in the same order that
@@ -3469,17 +3469,17 @@ XULDocument::OnScriptCompileComplete(JSS
                      "waiting for wrong script to load?");
         doc->mCurrentScriptProto = nullptr;
 
         // Unlink doc from scriptProto's list before executing and resuming
         *docp = doc->mNextSrcLoadWaiter;
         doc->mNextSrcLoadWaiter = nullptr;
 
         // Execute only if we loaded and compiled successfully, then resume
-        if (NS_SUCCEEDED(aStatus) && scriptProto->GetScriptObject()) {
+        if (NS_SUCCEEDED(aStatus) && scriptProto->HasScriptObject()) {
             doc->ExecuteScript(scriptProto);
         }
         doc->ResumeWalk();
         NS_RELEASE(doc);
     }
 
     return rv;
 }
@@ -3490,35 +3490,35 @@ XULDocument::ExecuteScript(nsXULPrototyp
     NS_PRECONDITION(aScript != nullptr, "null ptr");
     NS_ENSURE_TRUE(aScript, NS_ERROR_NULL_POINTER);
     NS_ENSURE_TRUE(mScriptGlobalObject, NS_ERROR_NOT_INITIALIZED);
 
     nsresult rv;
     rv = mScriptGlobalObject->EnsureScriptEnvironment();
     NS_ENSURE_SUCCESS(rv, rv);
 
-    JS::HandleScript scriptObject = aScript->GetScriptObject();
-    NS_ENSURE_TRUE(scriptObject, NS_ERROR_UNEXPECTED);
-
     // Execute the precompiled script with the given version
     nsAutoMicroTask mt;
 
     // We're about to run script via JS::CloneAndExecuteScript, so we need an
     // AutoEntryScript. This is Gecko specific and not in any spec.
     AutoEntryScript aes(mScriptGlobalObject, "precompiled XUL <script> element");
     JSContext* cx = aes.cx();
+
+    JS::Rooted<JSScript*> scriptObject(cx, aScript->GetScriptObject());
+    NS_ENSURE_TRUE(scriptObject, NS_ERROR_UNEXPECTED);
+
     JS::Rooted<JSObject*> baseGlobal(cx, JS::CurrentGlobalOrNull(cx));
     NS_ENSURE_TRUE(xpc::Scriptability::Get(baseGlobal).Allowed(), NS_OK);
 
     JSAddonId* addonId = mCurrentPrototype ? MapURIToAddonID(mCurrentPrototype->GetURI()) : nullptr;
     JS::Rooted<JSObject*> global(cx, xpc::GetAddonScope(cx, baseGlobal, addonId));
     NS_ENSURE_TRUE(global, NS_ERROR_FAILURE);
 
     JS::ExposeObjectToActiveJS(global);
-    xpc_UnmarkGrayScript(scriptObject);
     JSAutoCompartment ac(cx, global);
 
     // The script is in the compilation scope. Clone it into the target scope
     // and execute it. On failure, ~AutoScriptEntry will handle exceptions, so
     // there is no need to manually check the return value.
     JS::RootedValue rval(cx);
     JS::CloneAndExecuteScript(cx, scriptObject, &rval);
 
--- a/dom/xul/nsXULContentSink.cpp
+++ b/dom/xul/nsXULContentSink.cpp
@@ -524,17 +524,17 @@ XULContentSinkImpl::HandleEndElement(con
     }
     break;
 
     case nsXULPrototypeNode::eType_Script: {
         nsXULPrototypeScript* script =
             static_cast<nsXULPrototypeScript*>(node.get());
 
         // If given a src= attribute, we must ignore script tag content.
-        if (!script->mSrcURI && !script->GetScriptObject()) {
+        if (!script->mSrcURI && !script->HasScriptObject()) {
             nsCOMPtr<nsIDocument> doc = do_QueryReferent(mDocument);
 
             script->mOutOfLine = false;
             if (doc)
                 script->Compile(mText, mTextLength, mDocumentURL,
                                 script->mLineNo, doc);
         }
 
--- a/dom/xul/nsXULElement.cpp
+++ b/dom/xul/nsXULElement.cpp
@@ -2249,17 +2249,17 @@ nsXULPrototypeElement::Serialize(nsIObje
             } else {
                 tmp = aStream->WriteCompoundObject(script->mSrcURI,
                                                    NS_GET_IID(nsIURI),
                                                    true);
                 if (NS_FAILED(tmp)) {
                   rv = tmp;
                 }
 
-                if (script->GetScriptObject()) {
+                if (script->HasScriptObject()) {
                     // This may return NS_OK without muxing script->mSrcURI's
                     // data into the cache file, in the case where that
                     // muxed document is already there (written by a prior
                     // session, or by an earlier cache episode during this
                     // session).
                     tmp = script->SerializeOutOfLine(aStream, aProtoDoc);
                     if (NS_FAILED(tmp)) {
                       rv = tmp;
@@ -2526,25 +2526,20 @@ nsXULPrototypeScript::Serialize(nsIObjec
 
     // Write basic prototype data
     nsresult rv;
     rv = aStream->Write32(mLineNo);
     if (NS_FAILED(rv)) return rv;
     rv = aStream->Write32(mLangVersion);
     if (NS_FAILED(rv)) return rv;
 
-    // Calling fromMarkedLocation() is safe because we trace mScriptObject in
-    // TraceScriptObject() and because its value is never changed after it has
-    // been set.
-    JS::Handle<JSScript*> script =
-        JS::Handle<JSScript*>::fromMarkedLocation(mScriptObject.address());
     JSContext* cx = jsapi.cx();
+    JS::Rooted<JSScript*> script(cx, mScriptObject);
     MOZ_ASSERT(xpc::CompilationScope() == JS::CurrentGlobalOrNull(cx));
-    return nsContentUtils::XPConnect()->WriteScript(aStream, cx,
-                                                    xpc_UnmarkGrayScript(script));
+    return nsContentUtils::XPConnect()->WriteScript(aStream, cx, script);
 }
 
 nsresult
 nsXULPrototypeScript::SerializeOutOfLine(nsIObjectOutputStream* aStream,
                                          nsXULPrototypeDocument* aProtoDoc)
 {
     nsresult rv = NS_ERROR_NOT_IMPLEMENTED;
 
@@ -2668,18 +2663,20 @@ nsXULPrototypeScript::DeserializeOutOfLi
             // error.
             if (NS_SUCCEEDED(rv))
                 rv = Deserialize(objectInput, aProtoDoc, nullptr, nullptr);
 
             if (NS_SUCCEEDED(rv)) {
                 if (useXULCache && mSrcURI) {
                     bool isChrome = false;
                     mSrcURI->SchemeIs("chrome", &isChrome);
-                    if (isChrome)
-                        cache->PutScript(mSrcURI, GetScriptObject());
+                    if (isChrome) {
+                        JS::Rooted<JSScript*> script(RootingCx(), GetScriptObject());
+                        cache->PutScript(mSrcURI, script);
+                    }
                 }
                 cache->FinishInputStream(mSrcURI);
             } else {
                 // If mSrcURI is not in the cache,
                 // rv will be NS_ERROR_NOT_AVAILABLE and we'll try to
                 // update the cache file to hold a serialization of
                 // this script, once it has finished loading.
                 if (rv != NS_ERROR_NOT_AVAILABLE)
--- a/dom/xul/nsXULElement.h
+++ b/dom/xul/nsXULElement.h
@@ -230,26 +230,25 @@ public:
                      nsIURI* aURI, uint32_t aLineNo,
                      nsIDocument* aDocument,
                      nsIOffThreadScriptReceiver *aOffThreadReceiver = nullptr);
 
     void UnlinkJSObjects();
 
     void Set(JSScript* aObject);
 
-    // It's safe to return a handle because we trace mScriptObject, no one ever
-    // uses the handle (or the script object) past the point at which the
-    // nsXULPrototypeScript dies, and we can't get memmoved so the
-    // &mScriptObject pointer can't go stale.
-    JS::Handle<JSScript*> GetScriptObject()
+    bool HasScriptObject()
     {
-        // Calling fromMarkedLocation() is safe because we trace mScriptObject in
-        // TraceScriptObject() and because its value is never changed after it has
-        // been set.
-        return JS::Handle<JSScript*>::fromMarkedLocation(mScriptObject.address());
+        // Conversion to bool doesn't trigger mScriptObject's read barrier.
+        return mScriptObject;
+    }
+
+    JSScript* GetScriptObject()
+    {
+        return mScriptObject;
     }
 
     void TraceScriptObject(JSTracer* aTrc)
     {
         JS::TraceEdge(aTrc, &mScriptObject, "active window XUL prototype script");
     }
 
     void Trace(const TraceCallbacks& aCallbacks, void* aClosure)
--- a/js/xpconnect/src/xpcpublic.h
+++ b/js/xpconnect/src/xpcpublic.h
@@ -182,25 +182,16 @@ xpc_FastGetCachedWrapper(JSContext* cx, 
             vp.setObject(*wrapper);
             return wrapper;
         }
     }
 
     return nullptr;
 }
 
-inline JSScript*
-xpc_UnmarkGrayScript(JSScript* script)
-{
-    if (script)
-        JS::ExposeScriptToActiveJS(script);
-
-    return script;
-}
-
 // If aVariant is an XPCVariant, this marks the object to be in aGeneration.
 // This also unmarks the gray JSObject.
 extern void
 xpc_MarkInCCGeneration(nsISupports* aVariant, uint32_t aGeneration);
 
 // If aWrappedJS is a JS wrapper, unmark its JSObject.
 extern void
 xpc_TryUnmarkWrappedGrayObject(nsISupports* aWrappedJS);