Oilpan: add [WillBeGarbageCollected] for Node.

Source/core/frame/DOMWindow.cpp

 /*
  * Copyright (C) 2006, 2007, 2008, 2010 Apple Inc. All rights reserved.
  * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies)
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 327 matching lines @@
     if (!m_document)
         return;
 
     if (m_document->isActive()) {
         // FIXME: We don't call willRemove here. Why is that OK?
         // This detach() call is also mostly redundant. Most of the calls to
         // this function come via DocumentLoader::createWriterFor, which
         // always detaches the previous Document first. Only XSLTProcessor
         // depends on this detach() call, so it seems like there's some room
         // for cleanup.
         m_document->detach();

[haraken, 2014/05/23 20:53:58]

I'm not yet fully convinced about this one. Would you mind again elaborating on
why we don't need to call detach() when the DOMWindow dies but the Document is
still alive?

If we really wants to keep the current behavior as is, I think we need to move
the detach() call into the weak processing for Document::m_domWindow.

[sof, 2014/05/23 21:00:15]

I don't have a better answer than the one given earlier today. Will have to
sleep on it.

[sof, 2014/05/24 05:51:00]

Consider the following steps:

- A DOMWindow can at the earliest be destructed once a (Local)Frame lets go of
the window.
(- A subsequent v8 GC could then finalize and destruct the DOMWindow, assuming
nothing else keeping the window alive.)
- A LocalFrame will only let go of the window in its destructor.
- The LocalFrame destructor will prior to that clear out the 'view', invoking
setView(nullptr).
- Clearing that view will invoke Document::prepareForDestruction().
- prepareForDestruction() detach()es the document.
=> By the time the DOMWindow is a candidate for destruction, the document will
already have been detached.
- When the DOMWindow's dtor does get to run, the window's detached document
(should it be accessible somehow with Oilpan enabled) will be in a Stopped
state. Hence, the conditional detach() in DOMWindow::clearDocument() can never
apply.
- As a result of that being a no-op, the entire call to clearDocument() is not
needed for Oilpan.
- Indeed, as Document::detach() already calls clearEventQueue() on its Window,
the extra call we added to clearEventQueue() in ~DOMWindow for the Oilpan case
is also redundant. Will remove.

And if that detailed set of steps doesn't convince, the detached document
condition is already asserted for in ~DOMWindow (cf. isStopped() /
isDisposed().)
As detailed above, not needed.

     }
 
-    // FIXME: This should be part of ActiveDOM Object shutdown
+    // FIXME: This should be part of ActiveDOMObject shutdown
     clearEventQueue();
 
     m_document->clearDOMWindow();
     m_document = nullptr;
 }
 
 void DOMWindow::clearEventQueue()
 {
     if (!m_eventQueue)
         return;
@@ skipping 120 matching lines @@
         enqueuePopstateEvent(stateObject);
     else
         m_pendingStateObject = stateObject;
 }
 
 DOMWindow::~DOMWindow()
 {
     ASSERT(m_hasBeenReset);
     reset();
 
-    removeAllEventListeners();
+#if ENABLE(OILPAN)
+    // Oilpan: the frame host and document objects are
+    // also garbage collected; cannot notify these
+    // when removing event listeners.
+    removeAllEventListeners(DoNotBroadcastListenerRemoval);
 
-#if ENABLE(OILPAN)
-    ASSERT(m_document->isDisposed());

[haraken, 2014/05/23 20:53:58]

Just help me understand: Why do we need to remove this ASSERT?

[sof, 2014/05/24 05:51:00]

It was safe with m_document being a RefPtr<>, no longer so with it being a
Member.

Add an assert to the DOMWindow weak processing on Document instead?

[sof, 2014/05/24 06:53:11]

Done.

+    // FIXME: This should be part of ActiveDOMObject shutdown
+    clearEventQueue();
 #else
+    removeAllEventListeners(DoBroadcastListenerRemoval);
+
     ASSERT(m_document->isStopped());
+    clearDocument();
 #endif
-
-    clearDocument();
 }
 
 const AtomicString& DOMWindow::interfaceName() const
 {
     return EventTargetNames::DOMWindow;
 }
 
 ExecutionContext* DOMWindow::executionContext() const
 {
     return m_document.get();
@@ skipping 1095 matching lines @@
     // FIXME(361045): remove InspectorInstrumentation calls once DevTools Timeline migrates to tracing.
     InspectorInstrumentationCookie cookie = InspectorInstrumentation::willDispatchEventOnWindow(frame(), *event, this);
 
     bool result = fireEventListeners(event.get());
 
     InspectorInstrumentation::didDispatchEventOnWindow(cookie);
 
     return result;
 }
 
-void DOMWindow::removeAllEventListeners()
+void DOMWindow::removeAllEventListeners(BroadcastListenerRemoval mode)
 {
     EventTarget::removeAllEventListeners();
 
     lifecycleNotifier().notifyRemoveAllEventListeners(this);
 
-    if (m_frame && m_frame->host())
-        m_frame->host()->eventHandlerRegistry().didRemoveAllEventHandlers(*this);
+    if (mode == DoBroadcastListenerRemoval) {
+        if (m_frame && m_frame->host())
+            m_frame->host()->eventHandlerRegistry().didRemoveAllEventHandlers(*this);
 
-    if (Document* document = this->document())
-        document->didClearTouchEventHandlers(document);
+        if (Document* document = this->document())
+            document->didClearTouchEventHandlers(document);
+    }
 
     removeAllUnloadEventListeners(this);
     removeAllBeforeUnloadEventListeners(this);
 }
 
+void DOMWindow::removeAllEventListeners()
+{
+    removeAllEventListeners(DoBroadcastListenerRemoval);
+}
+
 void DOMWindow::finishedLoading()
 {
     if (m_shouldPrintWhenFinishedLoading) {
         m_shouldPrintWhenFinishedLoading = false;
         print();
     }
 }
 
 void DOMWindow::setLocation(const String& urlString, DOMWindow* callingWindow, DOMWindow* enteredWindow, SetLocationLocking locking)
 {
@@ skipping 226 matching lines @@
     return static_cast<DOMWindowLifecycleNotifier&>(LifecycleContext<DOMWindow>::lifecycleNotifier());
 }
 
 PassOwnPtr<LifecycleNotifier<DOMWindow> > DOMWindow::createLifecycleNotifier()
 {
     return DOMWindowLifecycleNotifier::create(this);
 }
 
 void DOMWindow::trace(Visitor* visitor)
 {
+    visitor->trace(m_document);
     visitor->trace(m_screen);
     visitor->trace(m_history);
     visitor->trace(m_locationbar);
     visitor->trace(m_menubar);
     visitor->trace(m_personalbar);
     visitor->trace(m_scrollbars);
     visitor->trace(m_statusbar);
     visitor->trace(m_toolbar);
     visitor->trace(m_console);
     visitor->trace(m_navigator);
     visitor->trace(m_location);
     visitor->trace(m_media);
     visitor->trace(m_sessionStorage);
     visitor->trace(m_localStorage);
     visitor->trace(m_applicationCache);
     visitor->trace(m_performance);
     visitor->trace(m_css);
     WillBeHeapSupplementable<DOMWindow>::trace(visitor);
 }
 
 } // namespace WebCore