Expose Frame::detach() to the Blink API layer.

Expose Frame::detach() to the Blink API layer.

This is needed when the embedder needs to trigger frame detach for frame
trees that have been mirrored into other processes. This patch also adds
a frameDetached() callback for remote frames so the embedder knows when
to close() the corresponding frame.

BUG=357747
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182625

[dcheng, 2014-09-22 22:29:05]

dcheng@chromium.org changed reviewers:
+ dglazkov@chromium.org, japhet@chromium.org

[dcheng, 2014-09-22 22:29:05]

I'm not actually sure this fulfills all of OOPI's needs, or if this is actually
the interface we want. But it's a good starting point to help us fix some UaFs.
Right now, the embedder is calling close() in its destructor but we never unlink
the corresponding WebFrame objects from the frame tree.

[dcheng, 2014-09-22 22:33:31]

(Sorry sent the message too early).

Ultimately, what we want is to mirror frame detach as it works today for frames
completely in one page. Blink gets the signal to detach a frame (whether it's
from web content or driven by the embedder) and goes through the steps of
cleaning up that frame tree. In the process, it calls out to the embedder via a
WebFrame client callback (RenderFrameImpl::frameDetached for local frames) so
that the embedder can perform its cleanup and then call WebFrame::close().

[Nate Chapin, 2014-09-22 22:33:34]

This seems plausible, but I'll defer to dglazkov, since he's actually a
public/OWNER.

[dcheng, 2014-09-23 22:44:06]

Per the discussions with nasko@ and creis@ today, I've added a
WebRemoteFrameClient callback to signify when Blink has completed its portion of
the detach work. I'm not entirely sure what to write for the
comments--originally, it was called frame detached, because the frame was
detached from its view but a remote frame might not even have a view...

[nasko, 2014-09-23 22:50:16]

On 2014/09/23 22:44:06, dcheng wrote:
> Per the discussions with nasko@ and creis@ today, I've added a
> WebRemoteFrameClient callback to signify when Blink has completed its portion
of
> the detach work. I'm not entirely sure what to write for the
> comments--originally, it was called frame detached, because the frame was
> detached from its view but a remote frame might not even have a view...

I find your own message explaining the purpose to be very fitting of a comment:
"callback to signify when Blink has completed its portion of the detach work."

[dcheng, 2014-09-23 23:06:47]

On 2014/09/23 at 22:50:16, nasko wrote:
> On 2014/09/23 22:44:06, dcheng wrote:
> > Per the discussions with nasko@ and creis@ today, I've added a
> > WebRemoteFrameClient callback to signify when Blink has completed its
portion of
> > the detach work. I'm not entirely sure what to write for the
> > comments--originally, it was called frame detached, because the frame was
> > detached from its view but a remote frame might not even have a view...
> 
> I find your own message explaining the purpose to be very fitting of a
comment: "callback to signify when Blink has completed its portion of the detach
work."

I guess to me that seems kind of vague. What does it mean to 'detach' a remote
frame? I've come up with an alternate comment; let me know what you think.

[nasko, 2014-09-23 23:30:09]

On 2014/09/23 23:06:47, dcheng wrote:
> On 2014/09/23 at 22:50:16, nasko wrote:
> > On 2014/09/23 22:44:06, dcheng wrote:
> > > Per the discussions with nasko@ and creis@ today, I've added a
> > > WebRemoteFrameClient callback to signify when Blink has completed its
> portion of
> > > the detach work. I'm not entirely sure what to write for the
> > > comments--originally, it was called frame detached, because the frame was
> > > detached from its view but a remote frame might not even have a view...
> > 
> > I find your own message explaining the purpose to be very fitting of a
> comment: "callback to signify when Blink has completed its portion of the
detach
> work."
> 
> I guess to me that seems kind of vague. What does it mean to 'detach' a remote
> frame? I've come up with an alternate comment; let me know what you think.

The comment looks great.

[dglazkov, 2014-09-24 17:13:01]

lgtm

[dcheng, 2014-09-24 17:13:42]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-09-24 17:14:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/593893002/40001

[commit-bot: I haz the power, 2014-09-24 17:14:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-24 17:14:27]

Failed to apply patch for Source/web/WebFrame.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/web/WebFrame.cpp
  Hunk #1 FAILED at 87.
  1 out of 1 hunk FAILED -- saving rejects to file Source/web/WebFrame.cpp.rej

Patch:       Source/web/WebFrame.cpp
Index: Source/web/WebFrame.cpp
diff --git a/Source/web/WebFrame.cpp b/Source/web/WebFrame.cpp
index
826f69d06bb99574a13a752590594315a9d81917..2c0919c447f9ef56d5e126f7390e330cde0259af
100644
--- a/Source/web/WebFrame.cpp
+++ b/Source/web/WebFrame.cpp
@@ -87,6 +87,11 @@ bool WebFrame::swap(WebFrame* frame)
     return true;
 }
 
+void WebFrame::detach()
+{
+    toCoreFrame(this)->detach();
+}
+
 v8::Handle<v8::Value> WebFrame::executeScriptAndReturnValueForTests(const
WebScriptSource& source)
 {
     // FIXME: This fake UserGestureIndicator is required for a bunch of browser

[dcheng, 2014-09-24 17:20:47]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-09-24 17:21:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/593893002/60001

[commit-bot: I haz the power, 2014-09-24 18:24:24]

Committed patchset #4 (id:60001) as 182625