Make sure to root the function object we compile in

nsXBLPrototypeHandler::ExecuteHandler.  r=jst, sr=brendan

content/base/public/nsContentUtils.h

@@ -81,6 +81,8 @@ class nsIContentPolicy;
 class nsILineBreaker;
 class nsIWordBreaker;
 class nsIEventQueueService;
+class nsIJSRuntimeService;
+struct JSRuntime;
 #ifdef MOZ_XTF
 class nsIXTFService;
 #endif
@@ -581,6 +583,42 @@ public:
   {
     return sEventQueueService;
   }
+
+  /**
+   * Make sure that whatever value *aPtr contains at any given moment is
+   * protected from JS GC until we remove the GC root.  A call to this that
+   * succeeds MUST be matched by a call to RemoveJSGCRoot to avoid leaking.
+   */
+  static nsresult AddJSGCRoot(jsval* aPtr, const char* aName) {
+    return AddJSGCRoot((void*)aPtr, aName);
+  }
+
+  /**
+   * Make sure that whatever object *aPtr is pointing to at any given moment is
+   * protected from JS GC until we remove the GC root.  A call to this that
+   * succeeds MUST be matched by a call to RemoveJSGCRoot to avoid leaking.
+   */
+  static nsresult AddJSGCRoot(JSObject** aPtr, const char* aName) {
+    return AddJSGCRoot((void*)aPtr, aName);
+  }
+
+  /**
+   * Make sure that whatever object *aPtr is pointing to at any given moment is
+   * protected from JS GC until we remove the GC root.  A call to this that
+   * succeeds MUST be matched by a call to RemoveJSGCRoot to avoid leaking.
+   */
+  static nsresult AddJSGCRoot(void* aPtr, const char* aName);  
+
+  /**
+   * Remove aPtr as a JS GC root
+   */
+  static nsresult RemoveJSGCRoot(jsval* aPtr) {
+    return RemoveJSGCRoot((void*)aPtr);
+  }
+  static nsresult RemoveJSGCRoot(JSObject** aPtr) {
+    return RemoveJSGCRoot((void*)aPtr);
+  }
+  static nsresult RemoveJSGCRoot(void* aPtr);
   
 private:
   static nsresult doReparentContentWrapper(nsIContent *aChild,
@@ -591,7 +629,6 @@ private:
 
   static nsresult EnsureStringBundle(PropertiesFile aFile);
 
-
   static nsIDOMScriptObjectFactory *sDOMScriptObjectFactory;
 
   static nsIXPConnect *sXPConnect;
@@ -631,6 +668,12 @@ private:
 
   // Holds pointers to nsISupports* that should be released at shutdown
   static nsVoidArray* sPtrsToPtrsToRelease;
+
+  // For now, we don't want to automatically clean this up in Shutdown(), since
+  // consumers might unfortunately end up wanting to use it after that
+  static nsIJSRuntimeService* sJSRuntimeService;
+  static JSRuntime* sScriptRuntime;
+  static PRInt32 sScriptRootCount;
   
   static PRBool sInitialized;
 };
@@ -661,6 +704,42 @@ private:
   PRBool mScriptIsRunning;
 };
 
+class nsAutoGCRoot {
+public:
+  // aPtr should be the pointer to the jsval we want to protect
+  nsAutoGCRoot(jsval* aPtr, nsresult* aResult) :
+    mPtr(aPtr)
+  {
+    mResult = *aResult =
+      nsContentUtils::AddJSGCRoot(aPtr, "nsAutoGCRoot");
+  }
+
+  // aPtr should be the pointer to the JSObject* we want to protect
+  nsAutoGCRoot(JSObject** aPtr, nsresult* aResult) :
+    mPtr(aPtr)
+  {
+    mResult = *aResult =
+      nsContentUtils::AddJSGCRoot(aPtr, "nsAutoGCRoot");
+  }
+
+  // aPtr should be the pointer to the thing we want to protect
+  nsAutoGCRoot(void* aPtr, nsresult* aResult) :
+    mPtr(aPtr)
+  {
+    mResult = *aResult =
+      nsContentUtils::AddJSGCRoot(aPtr, "nsAutoGCRoot");
+  }
+
+  ~nsAutoGCRoot() {
+    if (NS_SUCCEEDED(mResult)) {
+      nsContentUtils::RemoveJSGCRoot(mPtr);
+    }
+  }
+
+private:
+  void* mPtr;
+  nsresult mResult;
+};
 
 #define NS_INTERFACE_MAP_ENTRY_CONTENT_CLASSINFO(_class)                      \
   if (aIID.Equals(NS_GET_IID(nsIClassInfo))) {                                \

content/base/src/nsContentUtils.cpp

@@ -114,6 +114,7 @@ static NS_DEFINE_CID(kXTFServiceCID, NS_XTFSERVICE_CID);
 #include "nsIWordBreaker.h"
 #include "nsIEventQueueService.h"
 #include "jsdbgapi.h"
+#include "nsIJSRuntimeService.h"
 
 // for ReportToConsole
 #include "nsIStringBundle.h"
@@ -145,6 +146,9 @@ nsILineBreaker *nsContentUtils::sLineBreaker;
 nsIWordBreaker *nsContentUtils::sWordBreaker;
 nsIEventQueueService *nsContentUtils::sEventQueueService;
 nsVoidArray *nsContentUtils::sPtrsToPtrsToRelease;
+nsIJSRuntimeService *nsContentUtils::sJSRuntimeService;
+JSRuntime *nsContentUtils::sScriptRuntime;
+PRInt32 nsContentUtils::sScriptRootCount = 0;
 
 
 PRBool nsContentUtils::sInitialized = PR_FALSE;
@@ -2390,3 +2394,58 @@ nsContentUtils::GetContentPolicy()
 
   return sContentPolicyService;
 }
+
+// static
+nsresult
+nsContentUtils::AddJSGCRoot(void* aPtr, const char* aName)
+{
+  if (!sScriptRuntime) {
+    nsresult rv = CallGetService("@mozilla.org/js/xpc/RuntimeService;1",
+                                 &sJSRuntimeService);
+    NS_ENSURE_TRUE(sJSRuntimeService, rv);
+
+    sJSRuntimeService->GetRuntime(&sScriptRuntime);
+    if (!sScriptRuntime) {
+      NS_RELEASE(sJSRuntimeService);
+      NS_WARNING("Unable to get JS runtime from JS runtime service");
+      return NS_ERROR_FAILURE;
+    }
+  }
+
+  PRBool ok;
+  ok = ::JS_AddNamedRootRT(sScriptRuntime, aPtr, aName);
+  if (!ok) {
+    if (sScriptRootCount == 0) {
+      // We just got the runtime... Just null things out, since no
+      // one's expecting us to have a runtime yet
+      NS_RELEASE(sJSRuntimeService);
+      sScriptRuntime = nsnull;
+    }
+    NS_WARNING("JS_AddNamedRootRT failed");
+    return NS_ERROR_OUT_OF_MEMORY;
+  }
+
+  // We now have one more root we added to the runtime
+  ++sScriptRootCount;
+
+  return NS_OK;
+}
+
+/* static */
+nsresult
+nsContentUtils::RemoveJSGCRoot(void* aPtr)
+{
+  if (!sScriptRuntime) {
+    NS_NOTREACHED("Trying to remove a JS GC root when none were added");
+    return NS_ERROR_UNEXPECTED;
+  }
+
+  ::JS_RemoveRootRT(sScriptRuntime, aPtr);
+
+  if (--sScriptRootCount == 0) {
+    NS_RELEASE(sJSRuntimeService);
+    sScriptRuntime = nsnull;
+  }
+
+  return NS_OK;
+}

content/xbl/src/Makefile.in

@@ -82,7 +82,6 @@ CPPSRCS		= \
     nsXBLResourceLoader.cpp \
     nsXBLDocumentInfo.cpp \
     nsXBLContentSink.cpp \
-    nsXBLProtoImplMember.cpp \
     nsXBLProtoImplProperty.cpp \
     nsXBLProtoImplMethod.cpp \
     nsXBLProtoImplField.cpp \

content/xbl/src/nsXBLProtoImplField.cpp

@@ -45,6 +45,7 @@
 #include "nsReadableUtils.h"
 #include "nsXBLProtoImplField.h"
 #include "nsIScriptContext.h"
+#include "nsContentUtils.h"
 
 MOZ_DECL_CTOR_COUNTER(nsXBLProtoImplField)
 

content/xbl/src/nsXBLProtoImplMember.h

@@ -96,8 +96,6 @@ struct nsXBLTextWithLineNumber
   }
 };
 
-struct nsAutoGCRoot;
-
 class nsXBLProtoImplMember
 {
 public:
@@ -122,40 +120,6 @@ protected:
   
   nsXBLProtoImplMember* mNext;  // The members of an implementation are chained.
   PRUnichar* mName;               // The name of the field, method, or property.
-  static nsIJSRuntimeService* gJSRuntimeService;
-  static JSRuntime* gScriptRuntime;
-  static PRInt32 gScriptRuntimeRefcnt;
-
-  static nsresult AddJSGCRoot(void* aScriptObjectRef, const char* aName);
-  static nsresult RemoveJSGCRoot(void* aScriptObjectRef);
-};
-
-// Struct to handle automatic rooting and unrooting
-struct nsAutoGCRoot {
-  // aPtr should be the pointer to the jsval we want to protect
-  nsAutoGCRoot(jsval* aPtr, nsresult* aResult) :
-    mPtr(aPtr)
-  {
-    mResult = *aResult =
-      nsXBLProtoImplMember::AddJSGCRoot(mPtr, "nsAutoGCRoot");
-  }
-
-  // aPtr should be the pointer to the JSObject* we want to protect
-  nsAutoGCRoot(JSObject** aPtr, nsresult* aResult) :
-    mPtr(aPtr)
-  {
-    mResult = *aResult =
-      nsXBLProtoImplMember::AddJSGCRoot(mPtr, "nsAutoGCRoot");
-  }
-
-  ~nsAutoGCRoot() {
-    if (NS_SUCCEEDED(mResult)) {
-      nsXBLProtoImplMember::RemoveJSGCRoot(mPtr);
-    }
-  }
-
-  void* mPtr;
-  nsresult mResult;
 };
 
 #endif // nsXBLProtoImplMember_h__

content/xbl/src/nsXBLProtoImplMethod.cpp

@@ -74,7 +74,7 @@ nsXBLProtoImplMethod::Destroy(PRBool aIsCompiled)
                   "Incorrect aIsCompiled in nsXBLProtoImplMethod::Destroy");
   if (aIsCompiled) {
     if (mJSMethodObject)
-      RemoveJSGCRoot(&mJSMethodObject);
+      nsContentUtils::RemoveJSGCRoot(&mJSMethodObject);
     mJSMethodObject = nsnull;
   }
   else {
@@ -258,7 +258,8 @@ nsXBLProtoImplMethod::CompileMember(nsIScriptContext* aContext, const nsCString&
     JSContext* cx = NS_REINTERPRET_CAST(JSContext*,
                                         aContext->GetNativeContext());
     rv = cx ?
-      AddJSGCRoot(&mJSMethodObject, "nsXBLProtoImplMethod::mJSMethodObject") :
+      nsContentUtils::AddJSGCRoot(&mJSMethodObject,
+                                  "nsXBLProtoImplMethod::mJSMethodObject") :
       NS_ERROR_UNEXPECTED;
     if (NS_FAILED(rv)) {
       mJSMethodObject = nsnull;

content/xbl/src/nsXBLProtoImplProperty.cpp

@@ -47,6 +47,7 @@
 #include "nsReadableUtils.h"
 #include "nsIScriptContext.h"
 #include "nsIScriptGlobalObject.h"
+#include "nsContentUtils.h"
 
 MOZ_DECL_CTOR_COUNTER(nsXBLProtoImplProperty)
 
@@ -88,14 +89,14 @@ nsXBLProtoImplProperty::Destroy(PRBool aIsCompiled)
                   "Incorrect aIsCompiled in nsXBLProtoImplProperty::Destroy");
 
   if ((mJSAttributes & JSPROP_GETTER) && mJSGetterObject) {
-    RemoveJSGCRoot(&mJSGetterObject);
+    nsContentUtils::RemoveJSGCRoot(&mJSGetterObject);
   }
   else {
     delete mGetterText;
   }
 
   if ((mJSAttributes & JSPROP_SETTER) && mJSSetterObject) {
-    RemoveJSGCRoot(&mJSSetterObject);
+    nsContentUtils::RemoveJSGCRoot(&mJSSetterObject);
   }
   else {
     delete mSetterText;
@@ -269,7 +270,8 @@ nsXBLProtoImplProperty::CompileMember(nsIScriptContext* aContext, const nsCStrin
         JSContext* cx = NS_REINTERPRET_CAST(JSContext*,
                                             aContext->GetNativeContext());
         rv = (cx)
-          ? AddJSGCRoot(&mJSGetterObject, "nsXBLProtoImplProperty::mJSGetterObject")
+          ? nsContentUtils::AddJSGCRoot(&mJSGetterObject,
+                                        "nsXBLProtoImplProperty::mJSGetterObject")
           : NS_ERROR_UNEXPECTED;
       }
       if (NS_FAILED(rv)) {
@@ -324,7 +326,8 @@ nsXBLProtoImplProperty::CompileMember(nsIScriptContext* aContext, const nsCStrin
         JSContext* cx = NS_REINTERPRET_CAST(JSContext*,
                                             aContext->GetNativeContext());
         rv = (cx)
-          ? AddJSGCRoot(&mJSSetterObject, "nsXBLProtoImplProperty::mJSSetterObject")
+          ? nsContentUtils::AddJSGCRoot(&mJSSetterObject,
+                                        "nsXBLProtoImplProperty::mJSSetterObject")
           : NS_ERROR_UNEXPECTED;
       }
       if (NS_FAILED(rv)) {

content/xbl/src/nsXBLPrototypeHandler.cpp

@@ -486,6 +486,9 @@ nsXBLPrototypeHandler::ExecuteHandler(nsIDOMEventReceiver* aReceiver,
   }
   NS_ENSURE_SUCCESS(rv, rv);
 
+  nsAutoGCRoot root(&handler, &rv);
+  NS_ENSURE_SUCCESS(rv, rv);
+
   // Temporarily bind it to the bound element
   boundContext->BindCompiledEventHandler(scriptObject, onEventAtom, handler);
 

content/xul/content/src/nsXULElement.cpp

@@ -204,57 +204,6 @@ static struct {
 } gFaults;
 #endif
 
-#include "nsIJSRuntimeService.h"
-static nsIJSRuntimeService* gJSRuntimeService = nsnull;
-static JSRuntime* gScriptRuntime = nsnull;
-static PRInt32 gScriptRuntimeRefcnt = 0;
-
-static nsresult
-AddJSGCRoot(void* aScriptObjectRef, const char* aName)
-{
-    if (++gScriptRuntimeRefcnt == 1 || !gScriptRuntime) {
-        CallGetService("@mozilla.org/js/xpc/RuntimeService;1",
-                       &gJSRuntimeService);
-        if (! gJSRuntimeService) {
-            NS_NOTREACHED("couldn't add GC root");
-            return NS_ERROR_FAILURE;
-        }
-
-        gJSRuntimeService->GetRuntime(&gScriptRuntime);
-        if (! gScriptRuntime) {
-            NS_NOTREACHED("couldn't add GC root");
-            return NS_ERROR_FAILURE;
-        }
-    }
-
-    PRBool ok;
-    ok = ::JS_AddNamedRootRT(gScriptRuntime, aScriptObjectRef, aName);
-    if (! ok) {
-        NS_NOTREACHED("couldn't add GC root");
-        return NS_ERROR_OUT_OF_MEMORY;
-    }
-
-    return NS_OK;
-}
-
-static nsresult
-RemoveJSGCRoot(void* aScriptObjectRef)
-{
-    if (! gScriptRuntime) {
-        NS_NOTREACHED("couldn't remove GC root");
-        return NS_ERROR_FAILURE;
-    }
-
-    ::JS_RemoveRootRT(gScriptRuntime, aScriptObjectRef);
-
-    if (--gScriptRuntimeRefcnt == 0) {
-        NS_RELEASE(gJSRuntimeService);
-        gScriptRuntime = nsnull;
-    }
-
-    return NS_OK;
-}
-
 //----------------------------------------------------------------------
 
 
@@ -794,9 +743,12 @@ nsXULElement::CompileEventHandler(nsIScriptContext* aContext,
             if (!cx)
                 return NS_ERROR_UNEXPECTED;
 
-            rv = AddJSGCRoot(&attr->mEventHandler,
-                             "nsXULPrototypeAttribute::mEventHandler");
-            if (NS_FAILED(rv)) return rv;
+            rv = nsContentUtils::AddJSGCRoot(&attr->mEventHandler,
+                                             "nsXULPrototypeAttribute::mEventHandler");
+            if (NS_FAILED(rv)) {
+                attr->mEventHandler = nsnull;
+                return rv;
+            }
         }
     }
 
@@ -2922,7 +2874,7 @@ nsXULPrototypeAttribute::~nsXULPrototypeAttribute()
 {
     MOZ_COUNT_DTOR(nsXULPrototypeAttribute);
     if (mEventHandler)
-        RemoveJSGCRoot(&mEventHandler);
+        nsContentUtils::RemoveJSGCRoot(&mEventHandler);
 }
 
 
@@ -3085,9 +3037,16 @@ nsXULPrototypeElement::Deserialize(nsIObjectInputStream* aStream,
                 break;
             case eType_Script: {
                 // language version/options obtained during deserialization.
-                nsXULPrototypeScript* script = new nsXULPrototypeScript(0, nsnull, PR_FALSE);
+                // Don't clobber rv here, since it might already be a failure!
+                nsresult result;
+                nsXULPrototypeScript* script =
+                    new nsXULPrototypeScript(0, nsnull, PR_FALSE, &result);
                 if (! script)
                     return NS_ERROR_OUT_OF_MEMORY;
+                if (NS_FAILED(result)) {
+                    delete script;
+                    return result;
+                }
                 child = script;
                 child->mType = childType;
 
@@ -3180,7 +3139,10 @@ nsXULPrototypeElement::SetAttrAt(PRUint32 aPos, const nsAString& aValue,
 // nsXULPrototypeScript
 //
 
-nsXULPrototypeScript::nsXULPrototypeScript(PRUint32 aLineNo, const char *aVersion, PRBool aHasE4XOption)
+nsXULPrototypeScript::nsXULPrototypeScript(PRUint32 aLineNo,
+                                           const char *aVersion,
+                                           PRBool aHasE4XOption,
+                                           nsresult* rv)
     : nsXULPrototypeNode(eType_Script),
       mLineNo(aLineNo),
       mSrcLoading(PR_FALSE),
@@ -3191,13 +3153,17 @@ nsXULPrototypeScript::nsXULPrototypeScript(PRUint32 aLineNo, const char *aVersio
       mLangVersion(aVersion)
 {
     NS_LOG_ADDREF(this, 1, ClassName(), ClassSize());
-    AddJSGCRoot(&mJSObject, "nsXULPrototypeScript::mJSObject");
+    *rv = nsContentUtils::AddJSGCRoot(&mJSObject,
+                                      "nsXULPrototypeScript::mJSObject");
+    mAddedGCRoot = NS_SUCCEEDED(*rv);
 }
 
 
 nsXULPrototypeScript::~nsXULPrototypeScript()
 {
-    RemoveJSGCRoot(&mJSObject);
+    if (mAddedGCRoot) {
+        nsContentUtils::RemoveJSGCRoot(&mJSObject);
+    }
 }
 
 

content/xul/content/src/nsXULElement.h

@@ -312,8 +312,10 @@ class nsXULDocument;
 class nsXULPrototypeScript : public nsXULPrototypeNode
 {
 public:
+    // Note: if *rv is failure after the script is constructed, delete
+    // it and return *rv.
     nsXULPrototypeScript(PRUint32 aLineNo, const char *aVersion,
-                         PRBool aHasE4XOption);
+                         PRBool aHasE4XOption, nsresult* rv);
     virtual ~nsXULPrototypeScript();
 
 #ifdef NS_BUILD_REFCNT_LOGGING
@@ -343,6 +345,7 @@ public:
     PRPackedBool             mSrcLoading;
     PRPackedBool             mOutOfLine;
     PRPackedBool             mHasE4XOption;
+    PRPackedBool             mAddedGCRoot;
     nsXULDocument*           mSrcLoadWaiters;   // [OWNER] but not COMPtr
     JSObject*                mJSObject;
     const char*              mLangVersion;

content/xul/document/src/nsXULContentSink.cpp

@@ -1256,9 +1256,14 @@ XULContentSinkImpl::OpenScript(const PRUnichar** aAttributes,
   // Don't process scripts that aren't JavaScript
   if (isJavaScript) {
       nsXULPrototypeScript* script =
-          new nsXULPrototypeScript(aLineNumber, jsVersionString, hasE4XOption);
+          new nsXULPrototypeScript(aLineNumber, jsVersionString, hasE4XOption,
+                                   &rv);
       if (! script)
           return NS_ERROR_OUT_OF_MEMORY;
+      if (NS_FAILED(rv)) {
+          delete script;
+          return rv;
+      }      
 
       // If there is a SRC attribute...
       if (! src.IsEmpty()) {