Fixed the reporting of paint times in window.chrome.loadTimes()

Fixed the reporting of paint times in window.chrome.loadTimes()

The paint events were updating the document_state of whatever the last
frame in the frame list was. In a case of "how did this ever work" it
must have been sheer luck that the last local frame in the list was the
main frame.

The update just checks to make sure that the local frame is actually
the main frame (no parent) when selecting the frame to update the
document_state of.

Could be that this broke in 38 when the proxy frames were added and
maybe they were only tested on pages with a single frame.

BUG=422913
Committed: https://crrev.com/dcc725bedfff11ab3265fa38fb36e15ea2261070
Cr-Commit-Position: refs/heads/master@{#300703}

[Pat Meenan, 2014-10-21 20:20:09]

pmeenan@chromium.org changed reviewers:
+ jam@chromium.org

[Pat Meenan, 2014-10-21 20:20:11]

jam@ PTAL. Hopefully trivial 2-line fix.

It looks like this was broken by the addition of RenderFrameProxyHost objects so
I'm a bit concerned that there may have been other fallout as well.

nick@, would you mind taking a look back at the proxy addition to make sure
there aren't other cases where pages with multiple frames might not be grabbing
the main frame correctly?

[Pat Meenan, 2014-10-21 20:26:16]

pmeenan@chromium.org changed reviewers:
+ creis@chromium.org

[Pat Meenan, 2014-10-21 20:26:16]

cries@ - looks like you reviewed the original CL for RenderFrameProxyHosts that
this is fixing: https://codereview.chromium.org/444503002

[Charlie Reis, 2014-10-21 23:40:19]

creis@chromium.org changed reviewers:
+ nasko@chromium.org, nick@chromium.org

[Charlie Reis, 2014-10-21 23:40:20]

[+nasko,nick for FYI]

Thanks for catching this.  LGTM, and hopefully we can clean up the hack in a
follow-up CL.

https://codereview.chromium.org/670513003/diff/1/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/670513003/diff/1/content/renderer/render_view...
content/renderer/render_view_impl.cc:3329: for (WebFrame* frame = main_frame;
frame;
Yikes.  The premise behind this whole loop seems broken to me.  Sorry to have
missed it.

I think this is intended to be the same short-term hack to avoid an OOPIF crash
as in GetScrollOffset below.  Can you copy Nasko's TODO from line 3360 here as
well, so that we update both places in a follow-up CL?

[Charlie Reis, 2014-10-21 23:40:48]

[+site-isolation-reviews]

[Pat Meenan, 2014-10-22 00:02:17]

Looking at the scroll offset loop it looks like it has a very similar issue.  At
least it breaks out of the loop on the first frame it finds that is local
instead of the last but it doesn't check to see if it is a top-level frame.  If
the main frame will always be the first in the list it should be ok but should I
also add the !frame->parent() check to the scroll offset loop?

[Charlie Reis, 2014-10-22 00:07:01]

On 2014/10/22 00:02:17, Pat Meenan wrote:
> Looking at the scroll offset loop it looks like it has a very similar issue. 
At
> least it breaks out of the loop on the first frame it finds that is local
> instead of the last but it doesn't check to see if it is a top-level frame. 
If
> the main frame will always be the first in the list it should be ok but should
I
> also add the !frame->parent() check to the scroll offset loop?

Wait, your fix isn't right.  The !frame->parent() check defeats the purpose of
that code, which is to allow the code to avoid crashing in --site-per-process
when the main frame isn't local.

The loop to find the first local frame is an awkward approach and does need a
"break," but it shouldn't affect anything outside --site-per-process mode.

So, not LGTM due to the !frame->parent() check.  (Nasko, perhaps there's a more
graceful way to handle this case now in subframe processes?)

[nasko, 2014-10-22 00:09:43]

On 2014/10/22 00:07:01, Charlie Reis wrote:
> On 2014/10/22 00:02:17, Pat Meenan wrote:
> > Looking at the scroll offset loop it looks like it has a very similar issue.

> At
> > least it breaks out of the loop on the first frame it finds that is local
> > instead of the last but it doesn't check to see if it is a top-level frame. 
> If
> > the main frame will always be the first in the list it should be ok but
should
> I
> > also add the !frame->parent() check to the scroll offset loop?
> 
> Wait, your fix isn't right.  The !frame->parent() check defeats the purpose of
> that code, which is to allow the code to avoid crashing in --site-per-process
> when the main frame isn't local.
> 
> The loop to find the first local frame is an awkward approach and does need a
> "break," but it shouldn't affect anything outside --site-per-process mode.
> 
> So, not LGTM due to the !frame->parent() check.  (Nasko, perhaps there's a
more
> graceful way to handle this case now in subframe processes?)

I do agree that this is a nasty workaround and we should remove it when we can.
I think the !frame->parent() is actually wrong to add since the whole idea is
that the frame will have a parent, but it will be in a different process. If we
just keep the break, it will break out of the loop immediately, as we start from
the main frame, which is always local without --site-per-process.

[Pat Meenan, 2014-10-22 00:15:49]

nasko@ it looks like I either need to remove the parent check from the loop I
added and just leave the break on first match or add a parent check to the
scroll offset depending on what the hack is trying to do.

If the main frame is out of process then is the hack just finding an arbitrary
first in-process frame to operate on or will there always be a proxy in-process
that will not have any parents?

If it's the latter then the break will be enough to fix the common case and
still let it find an arbitrary in-process frame for the exception case.  If it's
the former then the parent check will never work and it will still crash.

Either way I think I need to either remove the parent check from the change I
made and just keep the break on first match or add the parent check to the
scroll offset case.

[Pat Meenan, 2014-10-22 00:20:01]

ok, sorry about the overlapping mails. I removed the parent check and added the
TODO so it matches the scroll offset hack.

[Pat Meenan, 2014-10-22 12:40:03]

https://codereview.chromium.org/670513003/diff/1/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/670513003/diff/1/content/renderer/render_view...
content/renderer/render_view_impl.cc:3329: for (WebFrame* frame = main_frame;
frame;
On 2014/10/21 23:40:20, Charlie Reis wrote:
> Yikes.  The premise behind this whole loop seems broken to me.  Sorry to have
> missed it.
> 
> I think this is intended to be the same short-term hack to avoid an OOPIF
crash
> as in GetScrollOffset below.  Can you copy Nasko's TODO from line 3360 here as
> well, so that we update both places in a follow-up CL?

Done.

[nasko, 2014-10-22 13:38:15]

LGTM

[Pat Meenan, 2014-10-22 13:39:26]

The CQ bit was checked by pmeenan@chromium.org

[commit-bot: I haz the power, 2014-10-22 13:40:12]

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

[commit-bot: I haz the power, 2014-10-22 13:40:15]

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

[commit-bot: I haz the power, 2014-10-22 13:40:16]

A disapproval has been posted by following reviewers: creis@chromium.org.
Please make sure to get positive review before another CQ attempt.

[Charlie Reis, 2014-10-22 16:07:52]

Sorry to hold things up.  LGTM.

[Charlie Reis, 2014-10-22 16:08:01]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2014-10-22 16:08:39]

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

[commit-bot: I haz the power, 2014-10-22 18:04:24]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-10-22 18:05:06]

Patchset 2 (id:??) landed as
https://crrev.com/dcc725bedfff11ab3265fa38fb36e15ea2261070
Cr-Commit-Position: refs/heads/master@{#300703}