Always create FrameTreeNodes and RenderFrameHosts for every frame.

Always create FrameTreeNodes and RenderFrameHosts for every frame.

Patch Set 1 and 2 are import of the original CL: https://codereview.chromium.org/82033002/

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

[Charlie Reis, 2013-12-20 01:42:56]

Sorry for the delay.  I'd like to know a little more about how this will help us
track down the bug, but otherwise I'm glad to see we're pursuing it.

(I'm guessing we'll want to land this after the break, so that we can triage and
revert it once we have data.)

Albert, can you take a look at the delete operator as well?

https://codereview.chromium.org/117603002/diff/80001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/117603002/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:142: // TODO(nasko): Remove this hack once
the bug is found.
Please add a comment saying how this will help catch the bug (and which bug
we're talking about).

https://codereview.chromium.org/117603002/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:144: memset(ptr, 0xAF,
sizeof(RenderFrameImpl));
Albert, can you take a look at this part?  I'd like to have an extra set of eyes
on it, and I think Nasko mentioned you'd seen something like this before.

[awong, 2013-12-20 02:06:42]

https://codereview.chromium.org/117603002/diff/80001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/117603002/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:144: memset(ptr, 0xAF,
sizeof(RenderFrameImpl));
On 2013/12/20 01:42:57, creis wrote:
> Albert, can you take a look at this part?  I'd like to have an extra set of
eyes
> on it, and I think Nasko mentioned you'd seen something like this before.

Yep...this is the way partition alloc works.

I think this is safe, but we should make sure to only leave it in for canary and
only for a short period of time.

[nasko, 2013-12-27 21:49:47]

Added a bunch of checks.

https://codereview.chromium.org/117603002/diff/80001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/117603002/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:142: // TODO(nasko): Remove this hack once
the bug is found.
On 2013/12/20 01:42:57, Charlie Reis (OOO) wrote:
> Please add a comment saying how this will help catch the bug (and which bug
> we're talking about).

Done.

[awong, 2013-12-27 21:59:35]

Bunches of nits and requests for TODOs marking debugging code.

https://codereview.chromium.org/117603002/diff/180001/content/public/test/ren...
File content/public/test/render_view_test.cc (right):

https://codereview.chromium.org/117603002/diff/180001/content/public/test/ren...
content/public/test/render_view_test.cc:44: const int32 kNewFrameRouteId = 10;
Is there a reason for 10? I would have expected 8 given the other routing ids
above are sequential (though it's odd that it starts at 5)

https://codereview.chromium.org/117603002/diff/180001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/117603002/diff/180001/content/renderer/render...
content/renderer/render_frame_impl.cc:625: CHECK(child_render_frame);
Add TODO saying this is an over-conservative debug check.

https://codereview.chromium.org/117603002/diff/180001/content/renderer/render...
content/renderer/render_frame_impl.cc:628: CHECK(web_frame);
Add TODO saying this is an over-conservative debug check.

https://codereview.chromium.org/117603002/diff/180001/content/renderer/render...
content/renderer/render_frame_impl.cc:632: CHECK(result.second) << "Inserting a
duplicate item!";
Add TODO saying this is an over-conservative debug check.

https://codereview.chromium.org/117603002/diff/180001/content/renderer/render...
content/renderer/render_frame_impl.cc:648: base::debug::Alias(frame);
Add TODO saying to remove all the Alias() calls int he function.

https://codereview.chromium.org/117603002/diff/180001/content/renderer/render...
content/renderer/render_frame_impl.cc:656: base::debug::Alias(&parent_frame_id);
Move the alias to right after declaration?

https://codereview.chromium.org/117603002/diff/180001/content/renderer/render...
content/renderer/render_frame_impl.cc:663: // should only be notified afterwards
so they cannot call back into and
nit: back into -> back into here

https://codereview.chromium.org/117603002/diff/180001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/117603002/diff/180001/content/renderer/render...
content/renderer/render_frame_impl.h:58: void operator delete(void*);
Add comment here saying to look at the impl comment.

https://codereview.chromium.org/117603002/diff/180001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/117603002/diff/180001/content/renderer/render...
content/renderer/render_view_impl.cc:3074: // but not for the main frame, which
is kept around in a scoped_ptr.
nit: kept around in a scoped_ptr -> owned by |main_render_frame_|

[awong, 2013-12-27 21:59:37]

Bunches of nits and requests for TODOs marking debugging code.

[nasko, 2013-12-27 22:14:53]

Fixes for most comments by Albert.

John, can you review for content OWNERS?

https://codereview.chromium.org/117603002/diff/180001/content/public/test/ren...
File content/public/test/render_view_test.cc (right):

https://codereview.chromium.org/117603002/diff/180001/content/public/test/ren...
content/public/test/render_view_test.cc:44: const int32 kNewFrameRouteId = 10;
On 2013/12/27 21:59:36, awong wrote:
> Is there a reason for 10? I would have expected 8 given the other routing ids
> above are sequential (though it's odd that it starts at 5)

I'm not sure, since this came from Charlie's CL. I think it can potentially
catch issues where code expects sequential routing ids, which cannot be
guaranteed.

https://codereview.chromium.org/117603002/diff/180001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/117603002/diff/180001/content/renderer/render...
content/renderer/render_frame_impl.cc:625: CHECK(child_render_frame);
On 2013/12/27 21:59:36, awong wrote:
> Add TODO saying this is an over-conservative debug check. 

Done.

https://codereview.chromium.org/117603002/diff/180001/content/renderer/render...
content/renderer/render_frame_impl.cc:628: CHECK(web_frame);
On 2013/12/27 21:59:36, awong wrote:
> Add TODO saying this is an over-conservative debug check. 

Done.

https://codereview.chromium.org/117603002/diff/180001/content/renderer/render...
content/renderer/render_frame_impl.cc:632: CHECK(result.second) << "Inserting a
duplicate item!";
On 2013/12/27 21:59:36, awong wrote:
> Add TODO saying this is an over-conservative debug check. 

I'd like to actually keep this in the long term, we should never be adding
duplicates.

https://codereview.chromium.org/117603002/diff/180001/content/renderer/render...
content/renderer/render_frame_impl.cc:648: base::debug::Alias(frame);
On 2013/12/27 21:59:36, awong wrote:
> Add TODO saying to remove all the Alias() calls int he function.

Done.

https://codereview.chromium.org/117603002/diff/180001/content/renderer/render...
content/renderer/render_frame_impl.cc:656: base::debug::Alias(&parent_frame_id);
On 2013/12/27 21:59:36, awong wrote:
> Move the alias to right after declaration?

Done.

https://codereview.chromium.org/117603002/diff/180001/content/renderer/render...
content/renderer/render_frame_impl.cc:663: // should only be notified afterwards
so they cannot call back into and
On 2013/12/27 21:59:36, awong wrote:
> nit: back into -> back into here

Done.

https://codereview.chromium.org/117603002/diff/180001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/117603002/diff/180001/content/renderer/render...
content/renderer/render_frame_impl.h:58: void operator delete(void*);
On 2013/12/27 21:59:36, awong wrote:
> Add comment here saying to look at the impl comment.

Done.

https://codereview.chromium.org/117603002/diff/180001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/117603002/diff/180001/content/renderer/render...
content/renderer/render_view_impl.cc:3074: // but not for the main frame, which
is kept around in a scoped_ptr.
On 2013/12/27 21:59:36, awong wrote:
> nit: kept around in a scoped_ptr -> owned by |main_render_frame_|

Done.

[jam, 2013-12-27 22:24:39]

lgtm

https://codereview.chromium.org/117603002/diff/300001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/117603002/diff/300001/content/renderer/render...
content/renderer/render_frame_impl.cc:621: if (routing_id == MSG_ROUTING_NONE)
this can't happen, so no need to add code to handle that case.

[awong, 2013-12-27 22:25:47]

LGTM

https://codereview.chromium.org/117603002/diff/180001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/117603002/diff/180001/content/renderer/render...
content/renderer/render_frame_impl.cc:632: CHECK(result.second) << "Inserting a
duplicate item!";
On 2013/12/27 22:14:53, nasko wrote:
> On 2013/12/27 21:59:36, awong wrote:
> > Add TODO saying this is an over-conservative debug check. 
> 
> I'd like to actually keep this in the long term, we should never be adding
> duplicates.

Okay. Remove the exclamation then. :)

IIRC, there's a tendency to avoid exclamatory log messages.

[commit-bot: I haz the power, 2013-12-28 01:59:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/117603002/480001

[commit-bot: I haz the power, 2013-12-28 06:52:28]

Retried try job too often on win_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-12-29 00:30:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/117603002/480001

[commit-bot: I haz the power, 2013-12-29 03:45:27]

Retried try job too often on win_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[nasko, 2014-01-06 23:58:58]

jam@, awong@, can you take a look at the latest patch? I realized we aren't
adding the main frame to the map, which will cause problems in the future when
we introduce the ::FromWebFrame method, as it won't find the main frame.

Lifetime of the main frame is still managed by RenderViewImpl, as it owns it,
but we now add it to the map once it is created and no longer subject removals
from the map to if (is_subframe).

Thanks!

[jam, 2014-01-07 02:29:34]

lgtm

[nasko, 2014-01-07 19:24:25]

Adding tburkard@ for FYI prerendering change in
chrome/renderer/chrome_render_frame_observer.cc.

[jam, 2014-01-07 19:45:16]

On 2014/01/07 19:24:25, nasko wrote:
> Adding tburkard@ for FYI prerendering change in
> chrome/renderer/chrome_render_frame_observer.cc.

lgtm

[commit-bot: I haz the power, 2014-01-09 19:39:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/117603002/960001

[commit-bot: I haz the power, 2014-01-10 00:05:15]

Change committed as 244023