Bug 893324 - Invoke CleanUp synchronously and unconditionally for modal dialog windows. r=jst
authorBobby Holley <bobbyholley@gmail.com>
Fri, 26 Jul 2013 15:46:13 -0700
changeset 140248 a685f0150c6cce0aacd11ebedc52fdeb49a5601c
parent 140247 2287b266756501af9cedfe42082b3525969be655
child 140249 2fd67ab03768efd84974ef483a1ac85c434d5eec
push id1951
push userryanvm@gmail.com
push dateSun, 28 Jul 2013 01:55:51 +0000
treeherderfx-team@73b69c146ca6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjst
bugs893324
milestone25.0a1
Bug 893324 - Invoke CleanUp synchronously and unconditionally for modal dialog windows. r=jst Historically, we've had a bunch of complicated machinery to delay the call to CleanUp() for modal dialogs, since we needed to harvest the return value after the window closed. But in the current world we don't actually null out mReturnValue in CleanUp() at all, which presumably happened sometime around the time mReturnValue became cycle-collected. So this stuff can just go away. \o/ That simplification is righteous in itself. But it also fixes the bug here as- filed. When the user quits while a modal dialog is currently being displayed, a failure code ends up bubbling up through windowWatcher to the OpenInternal call in nsGlobalWindow::ShowModalDialog, which means that we're unable to do our delayed CleanUp() call for the outer modal window. At first it seemed like a hard problem to solve, but with the above it becomes trivial.
dom/base/nsGlobalWindow.cpp
dom/base/nsGlobalWindow.h
--- a/dom/base/nsGlobalWindow.cpp
+++ b/dom/base/nsGlobalWindow.cpp
@@ -1030,17 +1030,16 @@ nsGlobalWindow::nsGlobalWindow(nsGlobalW
     mTimeoutFiringDepth(0),
     mTimeoutsSuspendDepth(0),
     mFocusMethod(0),
     mSerial(0),
 #ifdef DEBUG
     mSetOpenerWindowCalled(false),
 #endif
     mCleanedUp(false),
-    mCallCleanUpAfterModalDialogCloses(false),
     mDialogAbuseCount(0),
     mStopAbuseDialogs(false),
     mDialogsPermanentlyDisabled(false)
 {
   nsLayoutStatics::AddRef();
 
 #ifdef MOZ_GAMEPAD
   mGamepads.Init();
@@ -1248,17 +1247,17 @@ nsGlobalWindow::~nsGlobalWindow()
   mDoc = nullptr;
 
   // Outer windows are always supposed to call CleanUp before letting themselves
   // be destroyed. And while CleanUp generally seems to be intended to clean up
   // outers, we've historically called it for both. Changing this would probably
   // involve auditing all of the references that inners and outers can have, and
   // separating the handling into CleanUp() and FreeInnerObjects.
   if (IsInnerWindow()) {
-    CleanUp(true);
+    CleanUp();
   } else {
     MOZ_ASSERT(mCleanedUp);
   }
 
   nsCOMPtr<nsIDeviceSensors> ac = do_GetService(NS_DEVICE_SENSORS_CONTRACTID);
   if (ac)
     ac->RemoveWindowAsListener(this);
 
@@ -1318,30 +1317,18 @@ nsGlobalWindow::MaybeForgiveSpamCount()
     SetPopupSpamWindow(false);
     --gOpenPopupSpamCount;
     NS_ASSERTION(gOpenPopupSpamCount >= 0,
                  "Unbalanced decrement of gOpenPopupSpamCount");
   }
 }
 
 void
-nsGlobalWindow::CleanUp(bool aIgnoreModalDialog)
-{
-  if (IsOuterWindow() && !aIgnoreModalDialog) {
-    nsGlobalWindow* inner = GetCurrentInnerWindowInternal();
-    nsCOMPtr<nsIDOMModalContentWindow> dlg(do_QueryObject(inner));
-    if (dlg) {
-      // The window we're trying to clean up is the outer window of a
-      // modal dialog.  Defer cleanup until the window closes, and let
-      // ShowModalDialog take care of calling CleanUp.
-      mCallCleanUpAfterModalDialogCloses = true;
-      return;
-    }
-  }
-
+nsGlobalWindow::CleanUp()
+{
   // Guarantee idempotence.
   if (mCleanedUp)
     return;
   mCleanedUp = true;
 
   mEventTargetObjects.EnumerateEntries(DisconnectEventTargetObjects, nullptr);
   mEventTargetObjects.Clear();
 
@@ -1399,17 +1386,17 @@ nsGlobalWindow::CleanUp(bool aIgnoreModa
     mContext = nullptr;            // Forces Release
   }
   mChromeEventHandler = nullptr; // Forces Release
   mParentTarget = nullptr;
 
   nsGlobalWindow *inner = GetCurrentInnerWindowInternal();
 
   if (inner) {
-    inner->CleanUp(aIgnoreModalDialog);
+    inner->CleanUp();
   }
 
   DisableGamepadUpdates();
   mHasGamepad = false;
 
   if (mCleanMessageManager) {
     NS_ABORT_IF_FALSE(mIsChrome, "only chrome should have msg manager cleaned");
     nsGlobalChromeWindow *asChrome = static_cast<nsGlobalChromeWindow*>(this);
@@ -2778,17 +2765,17 @@ nsGlobalWindow::DetachFromDocShell()
 
   NS_ASSERTION(!mNavigator, "Non-null mNavigator in outer window!");
 
   if (mFrames) {
     mFrames->SetDocShell(nullptr);
   }
 
   MaybeForgiveSpamCount();
-  CleanUp(false);
+  CleanUp();
 }
 
 void
 nsGlobalWindow::SetOpenerWindow(nsIDOMWindow* aOpener,
                                 bool aOriginalOpener)
 {
   FORWARD_TO_OUTER_VOID(SetOpenerWindow, (aOpener, aOriginalOpener));
 
@@ -7242,17 +7229,17 @@ nsGlobalWindow::ReallyCloseWindow()
         bool isTab = false;
         if (rootWin == this ||
             !bwin || (bwin->IsTabContentWindow(GetOuterWindowInternal(),
                                                &isTab), isTab))
           treeOwnerAsWin->Destroy();
       }
     }
 
-    CleanUp(false);
+    CleanUp();
   }
 }
 
 nsIDOMWindow *
 nsGlobalWindow::EnterModalState()
 {
   // GetScriptableTop, not GetTop, so that EnterModalState works properly with
   // <iframe mozbrowser>.
@@ -7822,21 +7809,16 @@ nsGlobalWindow::ShowModalDialog(const ns
   nsContentUtils::SetMicroTaskLevel(oldMicroTaskLevel);
   LeaveModalState(callerWin);
   NS_ENSURE_SUCCESS(rv, rv);
 
   nsCOMPtr<nsIDOMModalContentWindow> dialog = do_QueryInterface(dlgWin);
   if (dialog) {
     rv = dialog->GetReturnValue(aRetVal);
     MOZ_ASSERT(NS_SUCCEEDED(rv));
-    nsGlobalModalWindow *win = static_cast<nsGlobalModalWindow*>(dialog.get());
-    if (win->mCallCleanUpAfterModalDialogCloses) {
-      win->mCallCleanUpAfterModalDialogCloses = false;
-      win->CleanUp(true);
-    }
   }
 
   return NS_OK;
 }
 
 class CommandDispatcher : public nsRunnable
 {
 public:
--- a/dom/base/nsGlobalWindow.h
+++ b/dom/base/nsGlobalWindow.h
@@ -804,17 +804,17 @@ protected:
 
   static bool sIdleObserversAPIFuzzTimeDisabled;
 
   friend class HashchangeCallback;
   friend class mozilla::dom::BarProp;
 
   // Object Management
   virtual ~nsGlobalWindow();
-  void CleanUp(bool aIgnoreModalDialog);
+  void CleanUp();
   void ClearControllers();
   nsresult FinalClose();
 
   inline void MaybeClearInnerWindow(nsGlobalWindow* aExpectedInner)
   {
     if(mInnerWindow == aExpectedInner) {
       mInnerWindow = nullptr;
     }
@@ -1239,17 +1239,17 @@ protected:
 
   uint32_t mSerial;
 
 #ifdef DEBUG
   bool mSetOpenerWindowCalled;
   nsCOMPtr<nsIURI> mLastOpenedURI;
 #endif
 
-  bool mCleanedUp, mCallCleanUpAfterModalDialogCloses;
+  bool mCleanedUp;
 
   nsCOMPtr<nsIDOMOfflineResourceList> mApplicationCache;
 
   nsJSThingHashtable<nsPtrHashKey<nsXBLPrototypeHandler>, JSObject*> mCachedXBLPrototypeHandlers;
 
   nsCOMPtr<nsIDocument> mSuspendedDoc;
 
   nsRefPtr<mozilla::dom::indexedDB::IDBFactory> mIndexedDB;