Rewrite how RemoteFrameOwners retain life.

Before out-of-process iframes, frame owners (i.e. HTMLFrameOwnerElement)
were retained by the DOM tree of the parent frame. However, remote
frames don't have a DOM tree to keep the frame owners alive. Instead,
remote frames used a map to keep the frame owners live while the
corresponding child frame was still in the frame tree.

However, this map was never updated when frames were swapped, so frame
owners would trivially leak after a frame swap. To simplify the code,
just eliminate the map and have remote frame owners keep themselves
live. With Oilpan, there's actually no need to do anything special:
Frame already has a strong reference to the FrameOwner.

Without Oilpan, it is a bit more complicated: a remote frame owner
manually refs and unrefs itself when attached to/detached from a content
frame.

BUG=none

Review URL: https://codereview.chromium.org/1807033003

Cr-Commit-Position: refs/heads/master@{#381717}

third_party/WebKit/Source/core/dom/Fullscreen.cpp

@@ -59,7 +59,7 @@ static bool fullscreenIsAllowedForAllOwners(const Document& document)
         // TODO(alexmos): The allowfullscreen attribute will need to be
         // replicated for this to work with OOPIFs.  For now, deny fullscreen
         // access inside OOPIFs until https://crbug.com/550497 is fixed.
-        if (!frame->owner()->isLocal())
+        if (frame->owner()->isRemote())
             return false;
 
         HTMLFrameOwnerElement* owner = toHTMLFrameOwnerElement(frame->owner());

third_party/WebKit/Source/core/frame/FrameOwner.h

@@ -23,6 +23,7 @@ public:
     DEFINE_INLINE_VIRTUAL_TRACE() { }
 
     virtual bool isLocal() const = 0;
+    virtual bool isRemote() const = 0;
 
     virtual void setContentFrame(Frame&) = 0;
     virtual void clearContentFrame() = 0;
@@ -51,6 +52,7 @@ public:
 
     // FrameOwner overrides:
     bool isLocal() const override { return false; }
+    bool isRemote() const override { return false; }
     void setContentFrame(Frame&) override { }
     void clearContentFrame() override { }
     SandboxFlags getSandboxFlags() const override { return SandboxNone; }

third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h

@@ -75,6 +75,7 @@ public:
 
     // FrameOwner overrides:
     bool isLocal() const override { return true; }
+    bool isRemote() const override { return false; }
     void setContentFrame(Frame&) override;
     void clearContentFrame() override;
     void dispatchLoad() override;

third_party/WebKit/Source/web/RemoteBridgeFrameOwner.cpp → third_party/WebKit/Source/web/RemoteFrameOwner.cpp

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be found
 // in the LICENSE file.
 
-#include "web/RemoteBridgeFrameOwner.h"
+#include "web/RemoteFrameOwner.h"
 
 #include "core/frame/LocalFrame.h"
 #include "public/web/WebFrameClient.h"
@@ -10,7 +10,7 @@
 
 namespace blink {
 
-RemoteBridgeFrameOwner::RemoteBridgeFrameOwner(SandboxFlags flags, const WebFrameOwnerProperties& frameOwnerProperties)
+RemoteFrameOwner::RemoteFrameOwner(SandboxFlags flags, const WebFrameOwnerProperties& frameOwnerProperties)
     : m_sandboxFlags(flags)
     , m_scrolling(static_cast<ScrollbarMode>(frameOwnerProperties.scrollingMode))
     , m_marginWidth(frameOwnerProperties.marginWidth)
@@ -18,29 +18,55 @@ RemoteBridgeFrameOwner::RemoteBridgeFrameOwner(SandboxFlags flags, const WebFram
 {
 }
 
-DEFINE_TRACE(RemoteBridgeFrameOwner)
+DEFINE_TRACE(RemoteFrameOwner)
 {
     visitor->trace(m_frame);
     FrameOwner::trace(visitor);
 }
 
-void RemoteBridgeFrameOwner::setScrollingMode(WebFrameOwnerProperties::ScrollingMode mode)
+void RemoteFrameOwner::setScrollingMode(WebFrameOwnerProperties::ScrollingMode mode)
 {
     m_scrolling = static_cast<ScrollbarMode>(mode);
 }
 
-void RemoteBridgeFrameOwner::setContentFrame(Frame& frame)
+void RemoteFrameOwner::setContentFrame(Frame& frame)
 {
+#if !ENABLE(OILPAN)
+    // Ownership of RemoteFrameOwner is complicated without Oilpan. Normally, a
+    // frame owner like HTMLFrameOwnerElement is kept alive by the DOM tree in
+    // its parent frame: the content frame of the frame owner only has a raw
+    // pointer back to its owner.
+    //
+    // However, a parent frame that is remote has no DOM tree, and thus, nothing
+    // to keep the remote frame owner alive. Instead, we manually ref() and
+    // deref() in setContentFrame() and clearContentFrame() to ensure that the
+    // remote frame owner remains live while associated with a frame.
+    //
+    // In an ideal world, Frame::m_owner would be a RefPtr, which would remove
+    // the need for this manual ref() / deref() pair. However, it's non-trivial
+    // to make that change, since HTMLFrameOwnerElement (which has FrameOwner
+    // has a base class) is already refcounted via a different base class.
+    //
+    // The other tricky part is WebFrame::swap(): during swap(), there is a
+    // period of time when a frame owner won't have an associated content frame.
+    // To prevent the refcount from going to zero, WebFrame::swap() must keep a
+    // stack reference to the frame owner if it is a remote frame owner.
+    ref();
+#endif
     m_frame = &frame;
 }
 
-void RemoteBridgeFrameOwner::clearContentFrame()
+void RemoteFrameOwner::clearContentFrame()
 {
     ASSERT(m_frame->owner() == this);
     m_frame = nullptr;
+#if !ENABLE(OILPAN)
+    // Balance the ref() in setContentFrame().
+    deref();
+#endif
 }
 
-void RemoteBridgeFrameOwner::dispatchLoad()
+void RemoteFrameOwner::dispatchLoad()
 {
     WebLocalFrameImpl* webFrame = WebLocalFrameImpl::fromFrame(toLocalFrame(*m_frame));
     webFrame->client()->dispatchLoad();

third_party/WebKit/Source/web/RemoteBridgeFrameOwner.h → third_party/WebKit/Source/web/RemoteFrameOwner.h

@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be found
 // in the LICENSE file.
 
-#ifndef RemoteBridgeFrameOwner_h
-#define RemoteBridgeFrameOwner_h
+#ifndef RemoteFrameOwner_h
+#define RemoteFrameOwner_h
 
 #include "core/frame/FrameOwner.h"
 #include "platform/scroll/ScrollTypes.h"
@@ -16,16 +16,17 @@ namespace blink {
 // 1. Allows the local frame's loader to retrieve sandbox flags associated with
 //    its owner element in another process.
 // 2. Trigger a load event on its owner element once it finishes a load.
-class RemoteBridgeFrameOwner final : public NoBaseWillBeGarbageCollectedFinalized<RemoteBridgeFrameOwner>, public FrameOwner {
-    WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(RemoteBridgeFrameOwner);
+class RemoteFrameOwner final : public RefCountedWillBeGarbageCollectedFinalized<RemoteFrameOwner>, public FrameOwner {
+    WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(RemoteFrameOwner);
 public:
-    static PassOwnPtrWillBeRawPtr<RemoteBridgeFrameOwner> create(SandboxFlags flags, const WebFrameOwnerProperties& frameOwnerProperties)
+    static PassRefPtrWillBeRawPtr<RemoteFrameOwner> create(SandboxFlags flags, const WebFrameOwnerProperties& frameOwnerProperties)
     {
-        return adoptPtrWillBeNoop(new RemoteBridgeFrameOwner(flags, frameOwnerProperties));
+        return adoptRefWillBeNoop(new RemoteFrameOwner(flags, frameOwnerProperties));
     }
 
     // FrameOwner overrides:
     bool isLocal() const override { return false; }
+    bool isRemote() const override { return true; }
     void setContentFrame(Frame&) override;
     void clearContentFrame() override;
     SandboxFlags getSandboxFlags() const override { return m_sandboxFlags; }
@@ -44,7 +45,7 @@ public:
     DECLARE_VIRTUAL_TRACE();
 
 private:
-    RemoteBridgeFrameOwner(SandboxFlags, const WebFrameOwnerProperties&);
+    RemoteFrameOwner(SandboxFlags, const WebFrameOwnerProperties&);
 
     RawPtrWillBeMember<Frame> m_frame;
     SandboxFlags m_sandboxFlags;
@@ -53,8 +54,8 @@ private:
     int m_marginHeight;
 };
 
-DEFINE_TYPE_CASTS(RemoteBridgeFrameOwner, FrameOwner, owner, !owner->isLocal(), !owner.isLocal());
+DEFINE_TYPE_CASTS(RemoteFrameOwner, FrameOwner, owner, owner->isRemote(), owner.isRemote());
 
 } // namespace blink
 
-#endif // RemoteBridgeFrameOwner_h
+#endif // RemoteFrameOwner_h

third_party/WebKit/Source/web/WebFrame.cpp

@@ -18,7 +18,7 @@
 #include "public/web/WebFrameOwnerProperties.h"
 #include "public/web/WebSandboxFlags.h"
 #include "web/OpenedFrameTracker.h"
-#include "web/RemoteBridgeFrameOwner.h"
+#include "web/RemoteFrameOwner.h"
 #include "web/WebLocalFrameImpl.h"
 #include "web/WebRemoteFrameImpl.h"
 #include <algorithm>
@@ -72,6 +72,13 @@ bool WebFrame::swap(WebFrame* frame)
     AtomicString name = oldFrame->tree().name();
     AtomicString uniqueName = oldFrame->tree().uniqueName();
     FrameOwner* owner = oldFrame->owner();
+#if !ENABLE(OILPAN)
+    // Persistence of a remote frame owner is complicated in the pre-Oilpan
+    // world. Please see RemoteFrameOwner::setContentFrame() for the details.
+    RefPtr<RemoteFrameOwner> remoteOwnerProtector;
+    if (owner && owner->isRemote())
+        remoteOwnerProtector = toRemoteFrameOwner(owner);
+#endif
 
     v8::HandleScope handleScope(v8::Isolate::GetCurrent());
     HashMap<DOMWrapperWorld*, v8::Local<v8::Object>> globals;
@@ -129,7 +136,7 @@ void WebFrame::setFrameOwnerSandboxFlags(WebSandboxFlags flags)
     // for frames with a remote owner.
     FrameOwner* owner = toImplBase()->frame()->owner();
     ASSERT(owner);
-    toRemoteBridgeFrameOwner(owner)->setSandboxFlags(static_cast<SandboxFlags>(flags));
+    toRemoteFrameOwner(owner)->setSandboxFlags(static_cast<SandboxFlags>(flags));
 }
 
 bool WebFrame::shouldEnforceStrictMixedContentChecking() const

third_party/WebKit/Source/web/WebLocalFrameImpl.cpp

@@ -227,7 +227,7 @@
 #include "web/MIDIClientProxy.h"
 #include "web/NavigatorContentUtilsClientImpl.h"
 #include "web/NotificationPermissionClientImpl.h"
-#include "web/RemoteBridgeFrameOwner.h"
+#include "web/RemoteFrameOwner.h"
 #include "web/SharedWorkerRepositoryClientImpl.h"
 #include "web/SuspendableScriptExecutor.h"
 #include "web/TextFinder.h"
@@ -1451,8 +1451,8 @@ WebLocalFrameImpl* WebLocalFrameImpl::createProvisional(WebFrameClient* client,
 
     frame->setOwner(oldFrame->owner());
 
-    if (frame->owner() && !frame->owner()->isLocal()) {
-        toRemoteBridgeFrameOwner(frame->owner())->setSandboxFlags(static_cast<SandboxFlags>(flags));
+    if (frame->owner() && frame->owner()->isRemote()) {
+        toRemoteFrameOwner(frame->owner())->setSandboxFlags(static_cast<SandboxFlags>(flags));
         // Since a remote frame doesn't get the notifications about frame owner
         // property modifications, we need to sync up those properties here.
         webFrame->setFrameOwnerProperties(frameOwnerProperties);
@@ -1870,9 +1870,9 @@ void WebLocalFrameImpl::setFrameOwnerProperties(const WebFrameOwnerProperties& f
     // for frames with a remote owner.
     FrameOwner* owner = frame()->owner();
     ASSERT(owner);
-    toRemoteBridgeFrameOwner(owner)->setScrollingMode(frameOwnerProperties.scrollingMode);
-    toRemoteBridgeFrameOwner(owner)->setMarginWidth(frameOwnerProperties.marginWidth);
-    toRemoteBridgeFrameOwner(owner)->setMarginHeight(frameOwnerProperties.marginHeight);
+    toRemoteFrameOwner(owner)->setScrollingMode(frameOwnerProperties.scrollingMode);
+    toRemoteFrameOwner(owner)->setMarginWidth(frameOwnerProperties.marginWidth);
+    toRemoteFrameOwner(owner)->setMarginHeight(frameOwnerProperties.marginHeight);
 }
 
 WebLocalFrameImpl* WebLocalFrameImpl::localRoot()

third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp

@@ -17,7 +17,7 @@
 #include "public/web/WebPerformance.h"
 #include "public/web/WebRange.h"
 #include "public/web/WebTreeScopeType.h"
-#include "web/RemoteBridgeFrameOwner.h"
+#include "web/RemoteFrameOwner.h"
 #include "web/WebLocalFrameImpl.h"
 #include "web/WebViewImpl.h"
 #include <v8/include/v8.h>
@@ -49,7 +49,6 @@ DEFINE_TRACE(WebRemoteFrameImpl)
 {
     visitor->trace(m_frameClient);
     visitor->trace(m_frame);
-    visitor->trace(m_ownersForChildren);
     visitor->template registerWeakMembers<WebFrame, &WebFrame::clearWeakFrames>(this);
     WebFrame::traceFrames(visitor, this);
     WebFrameImplBase::trace(visitor);
@@ -175,12 +174,6 @@ WebView* WebRemoteFrameImpl::view() const
     return WebViewImpl::fromPage(frame()->page());
 }
 
-void WebRemoteFrameImpl::removeChild(WebFrame* frame)
-{
-    WebFrame::removeChild(frame);
-    m_ownersForChildren.remove(frame);
-}
-
 WebDocument WebRemoteFrameImpl::document() const
 {
     // TODO(dcheng): this should also ASSERT_NOT_REACHED, but a lot of
@@ -605,14 +598,13 @@ WebString WebRemoteFrameImpl::layerTreeAsText(bool showDebugInfo) const
 WebLocalFrame* WebRemoteFrameImpl::createLocalChild(WebTreeScopeType scope, const WebString& name, const WebString& uniqueName, WebSandboxFlags sandboxFlags, WebFrameClient* client, WebFrame* previousSibling, const WebFrameOwnerProperties& frameOwnerProperties, WebFrame* opener)
 {
     WebLocalFrameImpl* child = WebLocalFrameImpl::create(scope, client, opener);
-    WillBeHeapHashMap<WebFrame*, OwnPtrWillBeMember<FrameOwner>>::AddResult result =
-        m_ownersForChildren.add(child, RemoteBridgeFrameOwner::create(static_cast<SandboxFlags>(sandboxFlags), frameOwnerProperties));
     insertAfter(child, previousSibling);
+    RefPtrWillBeRawPtr<RemoteFrameOwner> owner = RemoteFrameOwner::create(static_cast<SandboxFlags>(sandboxFlags), frameOwnerProperties);
     // FIXME: currently this calls LocalFrame::init() on the created LocalFrame, which may
     // result in the browser observing two navigations to about:blank (one from the initial
     // frame creation, and one from swapping it into the remote process). FrameLoader might
     // need a special initialization function for this case to avoid that duplicate navigation.
-    child->initializeCoreFrame(frame()->host(), result.storedValue->value.get(), name, uniqueName);
+    child->initializeCoreFrame(frame()->host(), owner.get(), name, uniqueName);
     // Partially related with the above FIXME--the init() call may trigger JS dispatch. However,
     // if the parent is remote, it should never be detached synchronously...
     ASSERT(child->frame());
@@ -629,10 +621,9 @@ void WebRemoteFrameImpl::initializeCoreFrame(FrameHost* host, FrameOwner* owner,
 WebRemoteFrame* WebRemoteFrameImpl::createRemoteChild(WebTreeScopeType scope, const WebString& name, const WebString& uniqueName, WebSandboxFlags sandboxFlags, WebRemoteFrameClient* client, WebFrame* opener)
 {
     WebRemoteFrameImpl* child = WebRemoteFrameImpl::create(scope, client, opener);
-    WillBeHeapHashMap<WebFrame*, OwnPtrWillBeMember<FrameOwner>>::AddResult result =
-        m_ownersForChildren.add(child, RemoteBridgeFrameOwner::create(static_cast<SandboxFlags>(sandboxFlags), WebFrameOwnerProperties()));
     appendChild(child);
-    child->initializeCoreFrame(frame()->host(), result.storedValue->value.get(), name, uniqueName);
+    RefPtrWillBeRawPtr<RemoteFrameOwner> owner = RemoteFrameOwner::create(static_cast<SandboxFlags>(sandboxFlags), WebFrameOwnerProperties());
+    child->initializeCoreFrame(frame()->host(), owner.get(), name, uniqueName);
     return child;
 }
 

third_party/WebKit/Source/web/WebRemoteFrameImpl.h

@@ -5,7 +5,6 @@
 #ifndef WebRemoteFrameImpl_h
 #define WebRemoteFrameImpl_h
 
-#include "core/frame/FrameOwner.h"
 #include "core/frame/RemoteFrame.h"
 #include "public/web/WebRemoteFrame.h"
 #include "public/web/WebRemoteFrameClient.h"
@@ -13,9 +12,7 @@
 #include "web/WebExport.h"
 #include "web/WebFrameImplBase.h"
 #include "wtf/Compiler.h"
-#include "wtf/HashMap.h"
 #include "wtf/OwnPtr.h"
-#include "wtf/RefCounted.h"
 
 namespace blink {
 
@@ -45,7 +42,6 @@ public:
     bool hasHorizontalScrollbar() const override;
     bool hasVerticalScrollbar() const override;
     WebView* view() const override;
-    void removeChild(WebFrame*) override;
     WebDocument document() const override;
     WebPerformance performance() const override;
     bool dispatchBeforeUnloadEvent() override;
@@ -189,8 +185,6 @@ private:
     RefPtrWillBeMember<RemoteFrame> m_frame;
     WebRemoteFrameClient* m_client;
 
-    WillBeHeapHashMap<WebFrame*, OwnPtrWillBeMember<FrameOwner>> m_ownersForChildren;
-
 #if ENABLE(OILPAN)
     // Oilpan: WebRemoteFrameImpl must remain alive until close() is called.
     // Accomplish that by keeping a self-referential Persistent<>. It is

third_party/WebKit/Source/web/web.gypi

@@ -73,10 +73,10 @@
       'PopupMenuImpl.h',
       'PrerendererClientImpl.cpp',
       'PrerendererClientImpl.h',
-      'RemoteBridgeFrameOwner.cpp',
-      'RemoteBridgeFrameOwner.h',
       'RemoteFrameClientImpl.cpp',
       'RemoteFrameClientImpl.h',
+      'RemoteFrameOwner.cpp',
+      'RemoteFrameOwner.h',
       'ResizeViewportAnchor.cpp',
       'ResizeViewportAnchor.h',
       'RotationViewportAnchor.cpp',

third_party/WebKit/public/web/WebFrame.h

@@ -217,7 +217,7 @@ public:
     BLINK_EXPORT void appendChild(WebFrame*);
 
     // Removes the given child from this frame.
-    virtual void removeChild(WebFrame*);
+    BLINK_EXPORT void removeChild(WebFrame*);
 
     // Returns the parent frame or 0 if this is a top-most frame.
     BLINK_EXPORT WebFrame* parent() const;