Remove all event listeners during window's frame destruction step.

Source/core/frame/LocalDOMWindow.cpp

Index: Source/core/frame/LocalDOMWindow.cpp
diff --git a/Source/core/frame/LocalDOMWindow.cpp b/Source/core/frame/LocalDOMWindow.cpp
index 7aeba5edc9a70729d129ee13b392159ae73d1fce..3add741d21689c5877b50620794fbebf3a5fb27a 100644
--- a/Source/core/frame/LocalDOMWindow.cpp
+++ b/Source/core/frame/LocalDOMWindow.cpp
@@ -203,14 +203,11 @@ private:
     int m_asyncOperationId;
 };
 
-static void disableSuddenTermination()
+static void updateSuddenTerminationStatus(LocalDOMWindow* domWindow, bool addedListener, FrameLoaderClient::SuddenTerminationDisablerType disablerType)
 {
-    blink::Platform::current()->suddenTerminationChanged(false);
-}
-
-static void enableSuddenTermination()
-{
-    blink::Platform::current()->suddenTerminationChanged(true);
+    blink::Platform::current()->suddenTerminationChanged(!addedListener);
+    if (domWindow->frame() && domWindow->frame()->loader().client())
+        domWindow->frame()->loader().client()->suddenTerminationDisablerChanged(addedListener, disablerType);
 }
 
 typedef HashCountedSet<LocalDOMWindow*> DOMWindowSet;
@@ -230,13 +227,9 @@ static DOMWindowSet& windowsWithBeforeUnloadEventListeners()
 static void addUnloadEventListener(LocalDOMWindow* domWindow)
 {
     DOMWindowSet& set = windowsWithUnloadEventListeners();
-    if (set.isEmpty()) {
-        disableSuddenTermination();
-        if (domWindow->frame()) {
-            domWindow->frame()->loader().client()->suddenTerminationDisablerChanged(
-                true, FrameLoaderClient::UnloadHandler);
-        }
-    }
+    if (set.isEmpty())
+        updateSuddenTerminationStatus(domWindow, true, FrameLoaderClient::UnloadHandler);
+
     set.add(domWindow);
 }
 
@@ -247,13 +240,8 @@ static void removeUnloadEventListener(LocalDOMWindow* domWindow)
     if (it == set.end())
         return;
     set.remove(it);
-    if (set.isEmpty()) {
-        enableSuddenTermination();
-        if (domWindow->frame()) {
-            domWindow->frame()->loader().client()->suddenTerminationDisablerChanged(
-                false, FrameLoaderClient::UnloadHandler);
-        }
-    }
+    if (set.isEmpty())
+        updateSuddenTerminationStatus(domWindow, false, FrameLoaderClient::UnloadHandler);
 }
 
 static void removeAllUnloadEventListeners(LocalDOMWindow* domWindow)
@@ -263,25 +251,16 @@ static void removeAllUnloadEventListeners(LocalDOMWindow* domWindow)
     if (it == set.end())
         return;
     set.removeAll(it);
-    if (set.isEmpty()) {
-        enableSuddenTermination();
-        if (domWindow->frame()) {
-            domWindow->frame()->loader().client()->suddenTerminationDisablerChanged(
-                false, FrameLoaderClient::UnloadHandler);
-        }
-    }
+    if (set.isEmpty())
+        updateSuddenTerminationStatus(domWindow, false, FrameLoaderClient::UnloadHandler);
 }
 
 static void addBeforeUnloadEventListener(LocalDOMWindow* domWindow)
 {
     DOMWindowSet& set = windowsWithBeforeUnloadEventListeners();
-    if (set.isEmpty()) {
-        disableSuddenTermination();
-        if (domWindow->frame()) {
-            domWindow->frame()->loader().client()->suddenTerminationDisablerChanged(
-                true, FrameLoaderClient::BeforeUnloadHandler);
-        }
-    }
+    if (set.isEmpty())
+        updateSuddenTerminationStatus(domWindow, true, FrameLoaderClient::BeforeUnloadHandler);
+
     set.add(domWindow);
 }
 
@@ -292,13 +271,8 @@ static void removeBeforeUnloadEventListener(LocalDOMWindow* domWindow)
     if (it == set.end())
         return;
     set.remove(it);
-    if (set.isEmpty()) {
-        enableSuddenTermination();
-        if (domWindow->frame()) {
-            domWindow->frame()->loader().client()->suddenTerminationDisablerChanged(
-                false, FrameLoaderClient::BeforeUnloadHandler);
-        }
-    }
+    if (set.isEmpty())
+        updateSuddenTerminationStatus(domWindow, false, FrameLoaderClient::BeforeUnloadHandler);
 }
 
 static void removeAllBeforeUnloadEventListeners(LocalDOMWindow* domWindow)
@@ -308,13 +282,8 @@ static void removeAllBeforeUnloadEventListeners(LocalDOMWindow* domWindow)
     if (it == set.end())
         return;
     set.removeAll(it);
-    if (set.isEmpty()) {
-        enableSuddenTermination();
-        if (domWindow->frame()) {
-            domWindow->frame()->loader().client()->suddenTerminationDisablerChanged(
-                false, FrameLoaderClient::BeforeUnloadHandler);
-        }
-    }
+    if (set.isEmpty())
+        updateSuddenTerminationStatus(domWindow, false, FrameLoaderClient::BeforeUnloadHandler);
 }
 
 static bool allowsBeforeUnloadListeners(LocalDOMWindow* window)
@@ -379,6 +348,9 @@ LocalDOMWindow::LocalDOMWindow(LocalFrame& frame)
     , m_hasBeenReset(false)
 #endif
 {
+#if ENABLE(OILPAN)
+    ThreadState::current()->registerPreFinalizer(*this);
+#endif
 }
 
 void LocalDOMWindow::clearDocument()
@@ -532,24 +504,28 @@ void LocalDOMWindow::statePopped(PassRefPtr<SerializedScriptValue> stateObject)
 LocalDOMWindow::~LocalDOMWindow()
 {
 #if ENABLE(OILPAN)

[haraken, 2015/03/18 23:38:45]

Not related to your CL, I'm wondering if we could remove this ENABLE(OILPAN)
branch. Now that we have a pre-finalizer in LocalDOMWindow, I guess it would be
possible to refactor the code so that oilpan and non-oilpan use the same code.

Ignore this comment if I'm not making sense. I don't quite remember why we have
different ASSERTs between oilpan and non-oilpan here. Either way, a separate CL
is fine.

[sof, 2015/03/19 06:27:36]

It would be nice to empty this dtor entirely of anything but asserts, but I
haven't investigated what parts of clearDocument() can be moved up/out.

-    // Oilpan: the frame host and document objects are
-    // also garbage collected; cannot notify these
-    // when removing event listeners.
-    removeAllEventListenersInternal(DoNotBroadcastListenerRemoval);
-
     // Cleared when detaching document.
     ASSERT(!m_eventQueue);
 #else
     ASSERT(m_hasBeenReset);
-    reset();
-
-    removeAllEventListenersInternal(DoBroadcastListenerRemoval);
-
     ASSERT(m_document->isStopped());
     clearDocument();
 #endif
 }
 
+void LocalDOMWindow::dispose()
+{
+    // Oilpan: should the LocalDOMWindow be GCed along with its LocalFrame, the frame
+    // may not have explicitly detached itself first. In that case, handle the clearing
+    // of event listeners in a prefinalizing action.
+    //
+    // (Non-Oilpan, LocalDOMWindow::reset() will always be invoked which takes care of this step.)

[haraken, 2015/03/18 23:38:45]

Help me understand: The LocalDOMWindow::reset() is called by
LocalFrame::setDOMWindow(), and it seems there is no difference in timing when
the setDOMWindow() is called in oilpan and non-oilpan. If that's the case, why
do you argue that reset() is always called in non-oilpan but not in oilpan?

[sof, 2015/03/19 06:27:36]

Yes, if the frame is detached, reset() will be called at the same time. Should
that not happen & when end up finalizing LocalFrame, ~LocalFrame() will only
call setDOMWindow() with Oilpan disabled. That's where the difference comes
from.

(A comment with a quiet word of caution about not adding too much code to this
dispose() method might be worth it here. We don't want to be doing too much work
in these actions.)

[haraken, 2015/03/19 06:54:45]

Thanks for the clarification; makes sense!

+    if (!frame())
+        return;
+
+    removeAllEventListeners();
+}
+
 const AtomicString& LocalDOMWindow::interfaceName() const
 {
     return EventTargetNames::LocalDOMWindow;
@@ -587,6 +563,7 @@ void LocalDOMWindow::frameDestroyed()
     willDestroyDocumentInFrame();
     resetLocation();
     m_properties.clear();
+    removeAllEventListeners();
 }
 
 void LocalDOMWindow::willDestroyDocumentInFrame()
@@ -1577,25 +1554,18 @@ bool LocalDOMWindow::dispatchEvent(PassRefPtrWillBeRawPtr<Event> prpEvent, PassR
     return fireEventListeners(event.get());
 }
 
-void LocalDOMWindow::removeAllEventListenersInternal(BroadcastListenerRemoval mode)
+void LocalDOMWindow::removeAllEventListeners()
 {
     EventTarget::removeAllEventListeners();
 
-    if (mode == DoBroadcastListenerRemoval) {
-        notifyRemoveAllEventListeners(this);
-        if (frame() && frame()->host())
-            frame()->host()->eventHandlerRegistry().didRemoveAllEventHandlers(*this);
-    }
+    notifyRemoveAllEventListeners(this);
+    if (frame() && frame()->host())
+        frame()->host()->eventHandlerRegistry().didRemoveAllEventHandlers(*this);
 
     removeAllUnloadEventListeners(this);
     removeAllBeforeUnloadEventListeners(this);
 }
 
-void LocalDOMWindow::removeAllEventListeners()
-{
-    removeAllEventListenersInternal(DoBroadcastListenerRemoval);
-}
-
 void LocalDOMWindow::finishedLoading()
 {
     if (m_shouldPrintWhenFinishedLoading) {