Move Frame to the Oilpan heap.

Source/core/frame/LocalDOMWindow.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 465 matching lines @@
 {
     if (!ContextFeatures::pushStateEnabled(document()))
         return;
 
     // FIXME: https://bugs.webkit.org/show_bug.cgi?id=36202 Popstate event needs to fire asynchronously
     dispatchEvent(PopStateEvent::create(stateObject, &history()));
 }
 
 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
     // defer firing of popstate until we're in the complete state.
     if (document()->isLoadCompleted())
         enqueuePopstateEvent(stateObject);
     else
         m_pendingStateObject = 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
     // also garbage collected; cannot notify these
     // when removing event listeners.
     removeAllEventListenersInternal(DoNotBroadcastListenerRemoval);
 
     // Cleared when detaching document.
     ASSERT(!m_eventQueue);
 #else
@@ skipping 22 matching lines @@
 PassRefPtrWillBeRawPtr<MediaQueryList> LocalDOMWindow::matchMedia(const String& media)
 {
     return document() ? document()->mediaQueryMatcher().matchMedia(media) : nullptr;
 }
 
 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()
 {
     m_frame->host()->eventHandlerRegistry().didRemoveAllEventHandlers(*this);
     m_frame->console().messageStorage()->frameWindowDiscarded(this);
     InspectorInstrumentation::frameWindowDiscarded(m_frame, this);
 }
 
 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();
 }
 
 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();
 }
 
 void LocalDOMWindow::registerProperty(DOMWindowProperty* property)
 {
     m_properties.add(property);
 }
 
@@ skipping 325 matching lines @@
             frameConsole()->addMessage(consoleMessage.release());
             return;
         }
     }
 
     dispatchEvent(event);
 }
 
 DOMSelection* LocalDOMWindow::getSelection()
 {
-    if (!isCurrentlyDisplayedInFrame() || !m_frame)
+    if (!isCurrentlyDisplayedInFrame())
         return 0;
 
     return m_frame->document()->getSelection();
 }
 
 Element* LocalDOMWindow::frameElement() const
 {
     if (!m_frame)
         return 0;
 
@@ skipping 137 matching lines @@
 
     return String();
 }
 
 bool LocalDOMWindow::find(const String& string, bool caseSensitive, bool backwards, bool wrap, bool /*wholeWord*/, bool /*searchInFrames*/, bool /*showDialog*/) const
 {
     if (!isCurrentlyDisplayedInFrame())
         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);
 }
 
 bool LocalDOMWindow::offscreenBuffering() const
 {
     return true;
 }
 
@@ skipping 112 matching lines @@
     if (!view)
         return 0;
 
     m_frame->document()->updateLayoutIgnorePendingStylesheets();
 
     return adjustForAbsoluteZoom(view->scrollY(), m_frame->pageZoomFactor());
 }
 
 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
 {
     if (!isCurrentlyDisplayedInFrame())
         return 0;
 
     return m_frame->tree().scopedChildCount();
 }
 
@@ skipping 648 matching lines @@
     LocalFrame* dialogFrame = createWindow(urlString, emptyAtom, windowFeatures,
         *callingWindow, *firstFrame, *m_frame, function, functionContext);
     if (!dialogFrame)
         return;
     UserGestureIndicatorDisabler disabler;
     dialogFrame->host()->chrome().runModal();
 }
 
 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();
 
     return 0;
 }
 
 DOMWindowLifecycleNotifier& LocalDOMWindow::lifecycleNotifier()
 {
     return static_cast<DOMWindowLifecycleNotifier&>(LifecycleContext<LocalDOMWindow>::lifecycleNotifier());
 }
 
 PassOwnPtr<LifecycleNotifier<LocalDOMWindow> > LocalDOMWindow::createLifecycleNotifier()
 {
     return DOMWindowLifecycleNotifier::create(this);
 }
 
 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);
     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);
     visitor->trace(m_eventQueue);
     WillBeHeapSupplementable<LocalDOMWindow>::trace(visitor);
     EventTargetWithInlineData::trace(visitor);
     LifecycleContext<LocalDOMWindow>::trace(visitor);
+    FrameDestructionObserver::trace(visitor);
 }
 
 } // namespace blink