Keep a copy of page id in RenderViewHost.

Keep a copy of page id in RenderViewHost.

This is an instrumented version of the patch that will be reverted in a few days.
This is meant to catch crashes in edge cases and log enough for us to repro
them.

BUG=99379, 369661
TEST=this is gonna crash for a few people

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

[Avi (use Gerrit), 2014-08-20 19:46:07]

Robert: crash keys; this is how you do it, right?
Charlie: are these the urls we should be logging?

[Robert Sesek, 2014-08-20 20:20:30]

crash_keys.cc LGTM

[Charlie Reis, 2014-08-20 20:22:30]

One question about whether logging the URL will be useful.

https://codereview.chromium.org/493853002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/493853002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:563:
base::debug::SetCrashKeyValue("url1",
Once we have the data, what will the URL tell us?  I ask because it seems
unlikely to lead to repro steps (from past experience on bugs like this) and is
data that probably shouldn't be logged without a reason.

I'm guessing that the page ID values will be more useful.  Do we have any
hypotheses so far that we could test with additional state (e.g., whether
there's a last committed entry)?

https://codereview.chromium.org/493853002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:924: "url1",
GetLastCommittedURL().possibly_invalid_spec());
It's possible for GetLastCommittedURL to return null in a new tab, and I bet we
could construct a case that we would even get an UpdateTitle message in that
situation (e.g., w=window.open(slow_url);w.document.title=foo).

[Avi (use Gerrit), 2014-08-20 20:29:20]

https://codereview.chromium.org/493853002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/493853002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:563:
base::debug::SetCrashKeyValue("url1",
On 2014/08/20 20:22:30, Charlie Reis wrote:
> I ask because it seems
> unlikely to lead to repro steps (from past experience on bugs like this)

I have no experience in debugging situations like this, so if you have
experience and suggestions as to what might be useful in this scenario I'm all
ears.

I was logging the URLs in the attempt to make a repro case out of them. I don't
have hypotheses right now. The scenario that I can imagine is like last time,
where we were re-creating a RenderView, and it sent out a message before
navigation (5 != -1 or the like), but right now I have no idea what is going on
here, or even if all of these crashes are even related.

https://codereview.chromium.org/493853002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:924: "url1",
GetLastCommittedURL().possibly_invalid_spec());
On 2014/08/20 20:22:30, Charlie Reis wrote:
> It's possible for GetLastCommittedURL to return null in a new tab, and I bet
we
> could construct a case that we would even get an UpdateTitle message in that
> situation (e.g., w=window.open(slow_url);w.document.title=foo).

Would that be a failure, though? Before the commit, the page ids would match and
we wouldn't fail. And GetLastCommittedURL can't return null; it returns GURL.

[Charlie Reis, 2014-08-20 20:44:47]

LGTM

https://codereview.chromium.org/493853002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/493853002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:563:
base::debug::SetCrashKeyValue("url1",
On 2014/08/20 20:29:20, Avi wrote:
> On 2014/08/20 20:22:30, Charlie Reis wrote:
> > I ask because it seems
> > unlikely to lead to repro steps (from past experience on bugs like this)
> 
> I have no experience in debugging situations like this, so if you have
> experience and suggestions as to what might be useful in this scenario I'm all
> ears.
> 
> I was logging the URLs in the attempt to make a repro case out of them. I
don't
> have hypotheses right now. The scenario that I can imagine is like last time,
> where we were re-creating a RenderView, and it sent out a message before
> navigation (5 != -1 or the like), but right now I have no idea what is going
on
> here, or even if all of these crashes are even related.

Well, I could be wrong-- the URL might lead to a repro in this case, if it's a
deterministic page behavior that's to blame.  Since we don't expect it to be
live for more than one Canary, it seems ok to give it a try.  Let's be sure to
revert it once it's out there for one build.

https://codereview.chromium.org/493853002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:924: "url1",
GetLastCommittedURL().possibly_invalid_spec());
On 2014/08/20 20:29:20, Avi wrote:
> On 2014/08/20 20:22:30, Charlie Reis wrote:
> > It's possible for GetLastCommittedURL to return null in a new tab, and I bet
> we
> > could construct a case that we would even get an UpdateTitle message in that
> > situation (e.g., w=window.open(slow_url);w.document.title=foo).
> 
> Would that be a failure, though? Before the commit, the page ids would match
and
> we wouldn't fail. And GetLastCommittedURL can't return null; it returns GURL.

Ah, you're right, GetLastCommittedEntry can return null but not
GetLastCommittedURL.

[Avi (use Gerrit), 2014-08-20 21:00:02]

https://codereview.chromium.org/493853002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/493853002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:563:
base::debug::SetCrashKeyValue("url1",
> Let's be sure to revert it once it's out there for one build.

No disagreement there.

[Avi (use Gerrit), 2014-08-20 21:00:07]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2014-08-20 21:02:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/avi@chromium.org/493853002/1

[commit-bot: I haz the power, 2014-08-20 22:50:19]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[commit-bot: I haz the power, 2014-08-20 22:54:26]

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

[commit-bot: I haz the power, 2014-08-20 22:54:27]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[Avi (use Gerrit), 2014-08-20 23:45:48]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2014-08-20 23:47:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/avi@chromium.org/493853002/1

[commit-bot: I haz the power, 2014-08-21 00:53:42]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[commit-bot: I haz the power, 2014-08-21 00:57:33]

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

[commit-bot: I haz the power, 2014-08-21 00:57:34]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[Avi (use Gerrit), 2014-08-21 01:47:41]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2014-08-21 01:48:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/avi@chromium.org/493853002/1

[commit-bot: I haz the power, 2014-08-21 01:54:12]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[commit-bot: I haz the power, 2014-08-21 01:57:32]

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

[commit-bot: I haz the power, 2014-08-21 01:57:33]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[Avi (use Gerrit), 2014-08-21 13:23:08]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2014-08-21 13:23:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/avi@chromium.org/493853002/1

[commit-bot: I haz the power, 2014-08-21 15:26:18]

Committed patchset #1 (1) as 291077

[Avi (use Gerrit), 2014-08-22 14:40:12]

A revert of this CL (patchset #1) has been created in
https://codereview.chromium.org/500533002/ by avi@chromium.org.

The reason for reverting is: This hit canary and we're getting instrumented
crashes. That's what we were looking for so let's back it out.
.