Move Frame to the Oilpan heap.

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 6c930d737ed8ddeac2084ccfee6205259667d83b..3e26658be94b022158e9423d3e6b0b61224ea65b 100644
--- a/Source/core/frame/LocalDOMWindow.cpp
+++ b/Source/core/frame/LocalDOMWindow.cpp
@@ -483,7 +483,7 @@ void LocalDOMWindow::enqueuePopstateEvent(PassRefPtr<SerializedScriptValue> stat
 
 void LocalDOMWindow::statePopped(PassRefPtr<SerializedScriptValue> stateObject)
 {
-    if (!frame())
+    if (!m_frame)
         return;
 
     // Per step 11 of section 6.5.9 (history traversal) of the HTML5 spec, we
@@ -496,8 +496,10 @@ void LocalDOMWindow::statePopped(PassRefPtr<SerializedScriptValue> stateObject)
 
 LocalDOMWindow::~LocalDOMWindow()
 {
+#if !ENABLE(OILPAN)
     ASSERT(m_hasBeenReset);
     reset();

[haraken, 2014/09/08 07:25:58]

Why is it OK to just remove the reset() call from oilpan builds? I guess someone
needs to call the reset() before the LocalDOMWindow gets destructed.

[sof, 2014/09/08 21:17:45]

Notifying the registered DOMWindowProperty instances isn't needed, they're being
swept at the same time. The property clearing in resetDOMWindowProperties() is
also not of value during finalization.

If not clear, the reason for having to disable these two lines is that when the
LocalFrame is swept, its strong reference to the LocalDOMWindow goes. i.e., the
will no longer trigger an explicit reset() when being destructed. Hence the
assert will no longer apply. And due to the fact that m_properties (holding the
DOMWindowProperty instances) is now on the heap, we cannot call reset() from
here & unsafely access m_properties.

+#endif
 
 #if ENABLE(OILPAN)
     // Oilpan: the frame host and document objects are
@@ -540,11 +542,13 @@ Page* LocalDOMWindow::page()
     return frame() ? frame()->page() : 0;
 }
 
+#if !ENABLE(OILPAN)
 void LocalDOMWindow::frameDestroyed()
 {
     FrameDestructionObserver::frameDestroyed();
     reset();

[haraken, 2014/09/08 07:25:58]

Ditto.

[sof, 2014/09/08 21:17:45]

The additional frame clearing that that reset() effectively forwards to the
DOMWindowProperty instances is now handled by keeping WeakMember<LocalFrame>s
instead.

 }
+#endif
 
 void LocalDOMWindow::willDetachFrameHost()
 {
@@ -557,7 +561,7 @@ void LocalDOMWindow::willDestroyDocumentInFrame()
 {
     // It is necessary to copy m_properties to a separate vector because the DOMWindowProperties may
     // unregister themselves from the LocalDOMWindow as a result of the call to willDestroyGlobalObjectInFrame.
-    Vector<DOMWindowProperty*> properties;
+    WillBeHeapVector<RawPtrWillBeMember<DOMWindowProperty> > properties;
     copyToVector(m_properties, properties);
     for (size_t i = 0; i < properties.size(); ++i)
         properties[i]->willDestroyGlobalObjectInFrame();
@@ -567,7 +571,7 @@ void LocalDOMWindow::willDetachDocumentFromFrame()
 {
     // It is necessary to copy m_properties to a separate vector because the DOMWindowProperties may
     // unregister themselves from the LocalDOMWindow as a result of the call to willDetachGlobalObjectFromFrame.
-    Vector<DOMWindowProperty*> properties;
+    WillBeHeapVector<RawPtrWillBeMember<DOMWindowProperty> > properties;
     copyToVector(m_properties, properties);
     for (size_t i = 0; i < properties.size(); ++i)
         properties[i]->willDetachGlobalObjectFromFrame();
@@ -913,7 +917,7 @@ void LocalDOMWindow::dispatchMessageEventWithOriginCheck(SecurityOrigin* intende
 
 DOMSelection* LocalDOMWindow::getSelection()
 {
-    if (!isCurrentlyDisplayedInFrame() || !m_frame)
+    if (!isCurrentlyDisplayedInFrame())
         return 0;
 
     return m_frame->document()->getSelection();
@@ -1071,8 +1075,8 @@ bool LocalDOMWindow::find(const String& string, bool caseSensitive, bool backwar
         return false;
 
     // |m_frame| can be destructed during |Editor::findString()| via
-    // |Document::updateLayou()|, e.g. event handler removes a frame.
-    RefPtr<LocalFrame> protectFrame(m_frame);
+    // |Document::updateLayout()|, e.g. event handler removes a frame.
+    RefPtrWillBeRawPtr<LocalFrame> protectFrame(m_frame.get());
 
     // FIXME (13016): Support wholeWord, searchInFrames and showDialog
     return m_frame->editor().findString(string, !backwards, caseSensitive, wrap, false);
@@ -1205,7 +1209,24 @@ int LocalDOMWindow::scrollY() const
 
 bool LocalDOMWindow::closed() const
 {
+#if ENABLE(OILPAN)
+    // FIXME: Oilpan: the timing of when m_frame is GCed is clearly
+    // different to what happens non-Oilpan. m_frame->hasBeenClosed()
+    // is considered instead/additionally to mirror extant behavior as
+    // closely as possible.
+    //
+    // However, the observed behavior isn't identical. Non-Oilpan, for
+    // an iframe removed from the DOM, window.closed will only turn
+    // 'true' once all references to it have gone and the object is
+    // finalized at the next v8 GC. This will naturally happen at some
+    // point after close() has been called on the underlying
+    // LocalFrame (as a side-effect of detaching and removing the
+    // frame from the DOM), hence with Oilpan 'window.closed' will
+    // transition to 'true' slightly earlier.

[haraken, 2014/09/08 07:25:58]

If possible, we should make this behavior change before landing this CL. It
would be better if we could use the explicit close() call in non-oilpan builds
as well and avoid introducing different call paths.

I believe the new behavior is a correct one.

[sof, 2014/09/08 21:17:45]

It wouldn't be a problem to separate it out and land it first, making non-Oilpan
and Oilpan behave the same.

I'm slightly nervous that it might break some old&ingrained assumptions in code
using window.closed though. (no, I don't know what code that might be! :) )

[sof, 2014/09/16 12:23:22]

Handling the change to window.closed via http://crbug.com/414658 (
https://codereview.chromium.org/538323003/ )

+    return !m_frame || m_frame->hasBeenClosed();
+#else
     return !m_frame;
+#endif
 }
 
 unsigned LocalDOMWindow::length() const
@@ -1874,11 +1895,10 @@ void LocalDOMWindow::showModalDialog(const String& urlString, const String& dial
 
 LocalDOMWindow* LocalDOMWindow::anonymousIndexedGetter(uint32_t index)
 {
-    LocalFrame* frame = this->frame();
-    if (!frame)
+    if (!m_frame)
         return 0;
 
-    Frame* child = frame->tree().scopedChild(index);
+    Frame* child = m_frame->tree().scopedChild(index);
     if (child)
         return child->domWindow();
 
@@ -1898,6 +1918,9 @@ PassOwnPtr<LifecycleNotifier<LocalDOMWindow> > LocalDOMWindow::createLifecycleNo
 void LocalDOMWindow::trace(Visitor* visitor)
 {
     visitor->trace(m_document);
+#if ENABLE(OILPAN)
+    visitor->trace(m_properties);
+#endif
     visitor->trace(m_screen);
     visitor->trace(m_history);
     visitor->trace(m_locationbar);
@@ -1919,6 +1942,7 @@ void LocalDOMWindow::trace(Visitor* visitor)
     WillBeHeapSupplementable<LocalDOMWindow>::trace(visitor);
     EventTargetWithInlineData::trace(visitor);
     LifecycleContext<LocalDOMWindow>::trace(visitor);
+    FrameDestructionObserver::trace(visitor);
 }
 
 } // namespace blink