Bug 622326. If we still have script running when we want to clear our window scope, use a termination function, not a runnable, to wait for it to finish. r=bzbarsky,jst a=blocker
authorPaolo Amadini <paolo.mozmail@amadzone.org>
Thu, 27 Jan 2011 14:42:19 -0800
changeset 61409 5e0bba79c052f89caa54df38210fa0a7a1028bdf
parent 61408 ecb1a0b69e0bc9b7c49570cbbd1654ada670d63a
child 61410 4215d5d9bc906b8a6f4438a2f9e73b6029bc8136
push id18350
push userjst@mozilla.com
push dateThu, 27 Jan 2011 22:43:19 +0000
treeherdermozilla-central@5e0bba79c052 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbzbarsky, jst, blocker
bugs622326
milestone2.0b11pre
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 622326. If we still have script running when we want to clear our window scope, use a termination function, not a runnable, to wait for it to finish. r=bzbarsky,jst a=blocker
dom/base/nsGlobalWindow.cpp
dom/base/nsGlobalWindow.h
dom/base/nsJSEnvironment.cpp
--- a/dom/base/nsGlobalWindow.cpp
+++ b/dom/base/nsGlobalWindow.cpp
@@ -1155,52 +1155,46 @@ nsGlobalWindow::ClearControllers()
       if (context)
         context->SetCommandContext(nsnull);
     }
 
     mControllers = nsnull;
   }
 }
 
-class ClearScopeEvent : public nsRunnable
-{
-public:
-  ClearScopeEvent(nsGlobalWindow *innerWindow)
-    : mInnerWindow(innerWindow) {
-  }
-
-  NS_IMETHOD Run()
-  {
-    mInnerWindow->ReallyClearScope(this);
-    return NS_OK;
-  }
-
-private:
-  nsRefPtr<nsGlobalWindow> mInnerWindow;
-};
+// static
+void
+nsGlobalWindow::TryClearWindowScope(nsISupports *aWindow)
+{
+  nsGlobalWindow *window =
+          static_cast<nsGlobalWindow *>(static_cast<nsIDOMWindow*>(aWindow));
+
+  // This termination function might be called when any script evaluation in our
+  // context terminated, even if there are other scripts in the stack. Thus, we
+  // have to check again if a script is executing and post a new termination
+  // function if necessary.
+  window->ClearScopeWhenAllScriptsStop();
+}
 
 void
-nsGlobalWindow::ReallyClearScope(nsRunnable *aRunnable)
+nsGlobalWindow::ClearScopeWhenAllScriptsStop()
 {
   NS_ASSERTION(IsInnerWindow(), "Must be an inner window");
 
+  // We cannot clear scope safely until all the scripts in our script context
+  // stopped. This might be a long wait, for example if one script is busy
+  // because it started a nested event loop for a modal dialog.
   nsIScriptContext *jsscx = GetContextInternal();
   if (jsscx && jsscx->GetExecutingScript()) {
-    if (!aRunnable) {
-      aRunnable = new ClearScopeEvent(this);
-      if (!aRunnable) {
-        // The only reason that we clear scope here is to try to prevent
-        // leaks. Failing to clear scope might mean that we'll leak more
-        // but if we don't have enough memory to allocate a ClearScopeEvent
-        // we probably don't have to worry about this anyway.
-        return;
-      }
-    }
-
-    NS_DispatchToMainThread(aRunnable);
+    // We ignore the return value because the only reason that we clear scope
+    // here is to try to prevent leaks. Failing to clear scope might mean that
+    // we'll leak more but if we don't have enough memory to allocate a
+    // termination function we probably don't have to worry about this anyway.
+    jsscx->SetTerminationFunction(TryClearWindowScope,
+                                  static_cast<nsIDOMWindow *>(this));
     return;
   }
 
   NotifyWindowIDDestroyed("inner-window-destroyed");
   nsIScriptContext *scx = GetContextInternal();
   if (scx) {
     scx->ClearScope(mJSObject, PR_TRUE);
   }
@@ -1265,19 +1259,17 @@ nsGlobalWindow::FreeInnerObjects(PRBool 
   if (mApplicationCache) {
     static_cast<nsDOMOfflineResourceList*>(mApplicationCache.get())->Disconnect();
     mApplicationCache = nsnull;
   }
 
   mIndexedDB = nsnull;
 
   if (aClearScope) {
-    // NB: This might not clear our scope, but fire an event to do so
-    // instead.
-    ReallyClearScope(nsnull);
+    ClearScopeWhenAllScriptsStop();
   }
 
   if (mDummyJavaPluginOwner) {
     // Tear down the dummy java plugin.
 
     // XXXjst: On a general note, should windows with java stuff in
     // them ever even make it into the fast-back cache?
 
--- a/dom/base/nsGlobalWindow.h
+++ b/dom/base/nsGlobalWindow.h
@@ -294,17 +294,16 @@ public:
 
   typedef mozilla::TimeStamp TimeStamp;
   typedef mozilla::TimeDuration TimeDuration;
 
   // public methods
   nsPIDOMWindow* GetPrivateParent();
   // callback for close event
   void ReallyCloseWindow();
-  void ReallyClearScope(nsRunnable *aRunnable);
 
   // nsISupports
   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
 
   // nsIScriptGlobalObject
   virtual nsIScriptContext *GetContext();
   virtual JSObject *GetGlobalJSObject();
   JSObject *FastGetGlobalJSObject()
@@ -588,16 +587,18 @@ private:
   // Disables updates for the accelerometer.
   void DisableAccelerationUpdates();
 
 protected:
   // Object Management
   virtual ~nsGlobalWindow();
   void CleanUp(PRBool aIgnoreModalDialog);
   void ClearControllers();
+  static void TryClearWindowScope(nsISupports* aWindow);
+  void ClearScopeWhenAllScriptsStop();
   nsresult FinalClose();
 
   void FreeInnerObjects(PRBool aClearScope);
   nsGlobalWindow *CallerInnerWindow();
 
   nsresult InnerSetNewDocument(nsIDocument* aDocument);
 
   nsresult DefineArgumentsProperty(nsIArray *aArguments);
--- a/dom/base/nsJSEnvironment.cpp
+++ b/dom/base/nsJSEnvironment.cpp
@@ -1151,17 +1151,21 @@ nsJSContext::nsJSContext(JSRuntime *aRun
   mProcessingScriptTag = PR_FALSE;
 }
 
 nsJSContext::~nsJSContext()
 {
 #ifdef DEBUG
   nsCycleCollector_DEBUG_wasFreed(static_cast<nsIScriptContext*>(this));
 #endif
-  NS_PRECONDITION(!mTerminations, "Shouldn't have termination funcs by now");
+
+  // We may still have pending termination functions if the context is destroyed
+  // before they could be executed. In this case, free the references to their
+  // parameters, but don't execute the functions (see bug 622326).
+  delete mTerminations;
 
   mGlobalObjectRef = nsnull;
 
   DestroyJSContext();
 
   --sContextCount;
 
   if (!sContextCount && sDidShutdown) {
@@ -3355,17 +3359,17 @@ nsJSContext::ScriptEvaluated(PRBool aTer
     mModalStateTime = 0;
   }
 }
 
 nsresult
 nsJSContext::SetTerminationFunction(nsScriptTerminationFunc aFunc,
                                     nsISupports* aRef)
 {
-  NS_PRECONDITION(JS_IsRunning(mContext), "should be executing script");
+  NS_PRECONDITION(GetExecutingScript(), "should be executing script");
 
   nsJSContext::TerminationFuncClosure* newClosure =
     new nsJSContext::TerminationFuncClosure(aFunc, aRef, mTerminations);
   if (!newClosure) {
     return NS_ERROR_OUT_OF_MEMORY;
   }
 
   mTerminations = newClosure;