Prepare RenderFrameProxy for mirroring the frame tree

Changes to RenderFrameProxy:

- Add accessors: web_frame(), routing_id(), render_view().
- Remove accessor: render_frame(). Where we do need to touch the
  RenderFrame, we'll look it up by its routing ID.
- Small change to the CompositingHelper to use the new getters.
- Add a map to allow finding a RenderFrameProxy by its associated
  blink::WebFrame.
- Introduce a second factory function and differentiate the two
  factory functions according to the two ways RenderFrameProxies will
  be created. The first is for when an extant local RenderFrame is
  being swapped out and replaced with a new RenderFrameProxy. The
  second is for when a RenderFrameProxy needs to be created without
  displacing an existing RenderFrame, as shall occur once we mirror
  the frame tree.
- This second factory function, which is uncalled at the moment, will
  create WebRemoteFrames. Also there is stubbed out code in the first
  factory function to create WebRemoteFrames. This code is in
  preparation for eliminating the RenderFrame (and its attendant
  WebLocalFrame) and having instead just a RenderFrameProxy.
- Add some defensive checks to prepare for when the render frame may
  not exist, as will happen once the second factory function
  enters use.
- Add an Init function so that code can be shared between the two
  factory functions.

As an adminstrative note, this patch is a chunk of nasko's 
larger "use RenderFrameProxyHost" effor (issue 241223002)

BUG=357747
TEST=browsertests, http://csreis.github.io/tests/cross-site-iframe.html renders after going cross-site under --site-per-process

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283572

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283965

[ncarter (slow), 2014-06-30 23:51:55]

Avi: please review
Nasko: this is your code, with some clean-up and stubbing out.

[Avi (use Gerrit), 2014-07-01 01:34:04]

I have insufficient knowledge of this area to review things properly :(

[ncarter (slow), 2014-07-01 01:48:40]

OK, I'll hunt for a different reviewer.


On Mon, Jun 30, 2014 at 6:34 PM, <avi@chromium.org> wrote:

> I have insufficient knowledge of this area to review things properly :(
>
> https://codereview.chromium.org/357043006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dcheng, 2014-07-03 05:58:54]

https://codereview.chromium.org/357043006/diff/100001/content/renderer/child_...
File content/renderer/child_frame_compositing_helper.h (right):

https://codereview.chromium.org/357043006/diff/100001/content/renderer/child_...
content/renderer/child_frame_compositing_helper.h:61: static
ChildFrameCompositingHelper* CreateCompositingHelperForRenderFrame(
It seems kind of weird that this function ends with "ForRenderFrame" but we pass
in a RenderFrameProxy. Is there a reason for the current naming scheme?

https://codereview.chromium.org/357043006/diff/100001/content/renderer/render...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/357043006/diff/100001/content/renderer/render...
content/renderer/render_frame_proxy.cc:124:
RenderFrameProxy::~RenderFrameProxy() {
It appears that we're leaking web_frame_ because we never call close() on it. We
should have a TODO to fix that at least =)

https://codereview.chromium.org/357043006/diff/100001/content/renderer/render...
content/renderer/render_frame_proxy.cc:188: if (render_frame)
I think this could use a comment explaining why we might fail to find the
RenderFrameImpl.

https://codereview.chromium.org/357043006/diff/100001/content/renderer/render...
File content/renderer/render_frame_proxy.h (right):

https://codereview.chromium.org/357043006/diff/100001/content/renderer/render...
content/renderer/render_frame_proxy.h:59: // This method should be used to
create a RenderFrameProxy, when there isn't
I don't think this comment is completely accurate. This is used to drive
mirroring, right? So it's not /only/ applicable when another frame is doing a
cross-process navigation; it's needed any time a page is hosted in multiple
processes and another process creates a new subframe.

https://codereview.chromium.org/357043006/diff/100001/content/renderer/render...
content/renderer/render_frame_proxy.h:64: // RenderFrameProxy to which the new
frame is parented.
To a reader, I don't think it's immediately obvious why the parent must always
be a RenderFrameProxy--do we have a comment blob somewhere that explains why
(e.g. all subframes begin their life as a RenderFrame)?

https://codereview.chromium.org/357043006/diff/100001/content/renderer/render...
content/renderer/render_frame_proxy.h:64: // RenderFrameProxy to which the new
frame is parented.
To a reader, I don't think it's immediately obvious why the parent must always
be a RenderFrameProxy--do we have a comment blob somewhere that explains why
(e.g. all subframes begin their life as a RenderFrame)?

https://codereview.chromium.org/357043006/diff/100001/content/renderer/render...
content/renderer/render_frame_proxy.h:86: int routing_id() { return routing_id_;
}
Out of curiosity, is there a particular reason we omit the const here and
elsewhere?

https://codereview.chromium.org/357043006/diff/100001/content/renderer/render...
content/renderer/render_frame_proxy.h:106: blink::WebFrame* web_frame_;
I think these fields could use some comments. Also, theoretically, we're only
going to deal with WebRemoteFrames in RemoteFrameProxy right?

[ncarter (slow), 2014-07-14 17:35:07]

PTAL

https://codereview.chromium.org/357043006/diff/100001/content/renderer/child_...
File content/renderer/child_frame_compositing_helper.h (right):

https://codereview.chromium.org/357043006/diff/100001/content/renderer/child_...
content/renderer/child_frame_compositing_helper.h:61: static
ChildFrameCompositingHelper* CreateCompositingHelperForRenderFrame(
On 2014/07/03 05:58:54, dcheng (OOO) wrote:
> It seems kind of weird that this function ends with "ForRenderFrame" but we
pass
> in a RenderFrameProxy. Is there a reason for the current naming scheme?

Done.

https://codereview.chromium.org/357043006/diff/100001/content/renderer/render...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/357043006/diff/100001/content/renderer/render...
content/renderer/render_frame_proxy.cc:124:
RenderFrameProxy::~RenderFrameProxy() {
On 2014/07/03 05:58:54, dcheng (OOO) wrote:
> It appears that we're leaking web_frame_ because we never call close() on it.
We
> should have a TODO to fix that at least =)

I fixed it and added a TODO.

https://codereview.chromium.org/357043006/diff/100001/content/renderer/render...
content/renderer/render_frame_proxy.cc:188: if (render_frame)
On 2014/07/03 05:58:54, dcheng (OOO) wrote:
> I think this could use a comment explaining why we might fail to find the
> RenderFrameImpl.

I wouldn't know what to write.

https://codereview.chromium.org/357043006/diff/100001/content/renderer/render...
File content/renderer/render_frame_proxy.h (right):

https://codereview.chromium.org/357043006/diff/100001/content/renderer/render...
content/renderer/render_frame_proxy.h:59: // This method should be used to
create a RenderFrameProxy, when there isn't
On 2014/07/03 05:58:54, dcheng (OOO) wrote:
> I don't think this comment is completely accurate. This is used to drive
> mirroring, right? So it's not /only/ applicable when another frame is doing a
> cross-process navigation; it's needed any time a page is hosted in multiple
> processes and another process creates a new subframe.

Done.

https://codereview.chromium.org/357043006/diff/100001/content/renderer/render...
content/renderer/render_frame_proxy.h:64: // RenderFrameProxy to which the new
frame is parented.
On 2014/07/03 05:58:54, dcheng (OOO) wrote:
> To a reader, I don't think it's immediately obvious why the parent must always
> be a RenderFrameProxy--do we have a comment blob somewhere that explains why
> (e.g. all subframes begin their life as a RenderFrame)?

Done.

https://codereview.chromium.org/357043006/diff/100001/content/renderer/render...
content/renderer/render_frame_proxy.h:86: int routing_id() { return routing_id_;
}
On 2014/07/03 05:58:54, dcheng (OOO) wrote:
> Out of curiosity, is there a particular reason we omit the const here and
> elsewhere?

Done.

https://codereview.chromium.org/357043006/diff/100001/content/renderer/render...
content/renderer/render_frame_proxy.h:106: blink::WebFrame* web_frame_;
On 2014/07/03 05:58:54, dcheng (OOO) wrote:
> I think these fields could use some comments. Also, theoretically, we're only
> going to deal with WebRemoteFrames in RemoteFrameProxy right?

Yes, this will become a WebRemoteFrame in a later patch, but it can't until we
switch over to the #if 0'ed code in the .cc file.

[dcheng, 2014-07-15 18:44:15]

https://codereview.chromium.org/357043006/diff/100001/content/renderer/render...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/357043006/diff/100001/content/renderer/render...
content/renderer/render_frame_proxy.cc:188: if (render_frame)
On 2014/07/14 17:35:07, ncarter wrote:
> On 2014/07/03 05:58:54, dcheng (OOO) wrote:
> > I think this could use a comment explaining why we might fail to find the
> > RenderFrameImpl.
> 
> I wouldn't know what to write.

Does anything explode if we don't do this null check?

https://codereview.chromium.org/357043006/diff/140001/content/renderer/render...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/357043006/diff/140001/content/renderer/render...
content/renderer/render_frame_proxy.cc:36: int frame_routing_id) {
Is there any particular reason to use a routing ID for the render frame that's
being replaced? It looks like in render_frame_impl.cc:943 we pass in the routing
ID for 'this', but then on line 43 we reconstitute the RenderFrameImpl from the
routing ID.

https://codereview.chromium.org/357043006/diff/140001/content/renderer/render...
content/renderer/render_frame_proxy.cc:52: web_frame =
parent_web_frame->createRemoteChild("", proxy.get());
Could we pre-emptively plumb the frame name through here?

https://codereview.chromium.org/357043006/diff/140001/content/renderer/render...
content/renderer/render_frame_proxy.cc:66: int render_view_routing_id) {
Similar question: can/should we pass in a RemoteFrameProxy/RenderView directly
here rather than using the ::FromRoutingID() functions to reconstitute it below?

[ncarter (slow), 2014-07-16 01:13:53]

PTAL

https://codereview.chromium.org/357043006/diff/140001/content/renderer/render...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/357043006/diff/140001/content/renderer/render...
content/renderer/render_frame_proxy.cc:36: int frame_routing_id) {
On 2014/07/15 18:44:14, dcheng (OOO) wrote:
> Is there any particular reason to use a routing ID for the render frame that's
> being replaced? It looks like in render_frame_impl.cc:943 we pass in the
routing
> ID for 'this', but then on line 43 we reconstitute the RenderFrameImpl from
the
> routing ID.

Good suggestion. Done.

https://codereview.chromium.org/357043006/diff/140001/content/renderer/render...
content/renderer/render_frame_proxy.cc:52: web_frame =
parent_web_frame->createRemoteChild("", proxy.get());
On 2014/07/15 18:44:14, dcheng (OOO) wrote:
> Could we pre-emptively plumb the frame name through here?

Since this is #if 0'ed code, I have no way to test it. let's try this in a
future patch.

https://codereview.chromium.org/357043006/diff/140001/content/renderer/render...
content/renderer/render_frame_proxy.cc:66: int render_view_routing_id) {
On 2014/07/15 18:44:14, dcheng (OOO) wrote:
> Similar question: can/should we pass in a RemoteFrameProxy/RenderView directly
> here rather than using the ::FromRoutingID() functions to reconstitute it
below?

The code that calls this just has raw IDs from an IPC, so I think it's fine to
pass these as ints. However, I fixed the other case though.

[dcheng, 2014-07-16 01:22:23]

lgtm

[ncarter (slow), 2014-07-16 01:24:41]

+jam -- please have a look

[jam, 2014-07-16 06:03:17]

lgtm

[ncarter (slow), 2014-07-16 17:49:15]

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2014-07-16 17:55:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nick@chromium.org/357043006/160001

[commit-bot: I haz the power, 2014-07-16 19:02:58]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[commit-bot: I haz the power, 2014-07-16 23:23:12]

Change committed as 283572

[viettrungluu, 2014-07-17 01:33:03]

On 2014/07/16 23:23:12, I haz the power (commit-bot) wrote:
> Change committed as 283572

ASan is reporting a use-after-free that looks like it's due to you:

http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...

E.g.:

@@@STEP_LOG_LINE@ReloadEmbedder@SUMMARY: AddressSanitizer: heap-use-after-free
content/renderer/render_frame_proxy.cc:168
content::RenderFrameProxy::OnMessageReceived(IPC::Message const&)@@@

[viettrungluu, 2014-07-17 01:38:53]

On 2014/07/17 01:33:03, viettrungluu wrote:
> On 2014/07/16 23:23:12, I haz the power (commit-bot) wrote:
> > Change committed as 283572
> 
> ASan is reporting a use-after-free that looks like it's due to you:
> 
>
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...
> 
> E.g.:
> 
> @@@STEP_LOG_LINE@ReloadEmbedder@SUMMARY: AddressSanitizer: heap-use-after-free
> content/renderer/render_frame_proxy.cc:168
> content::RenderFrameProxy::OnMessageReceived(IPC::Message const&)@@@

Reverted your change at r283608. Sorry.

[ncarter (slow), 2014-07-17 18:39:33]

Daniel, please take another look -- most recent patch set has the fix we
discussed.

[dcheng, 2014-07-17 18:42:12]

lgtm

May be worth adding a short comment that if the message was handled, |this| may
already be destroyed.

[ncarter (slow), 2014-07-17 18:47:32]

Done.

[ncarter (slow), 2014-07-17 18:47:42]

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2014-07-17 18:49:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nick@chromium.org/357043006/220001

[commit-bot: I haz the power, 2014-07-17 23:37:01]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-18 01:34:42]

Change committed as 283965