Fixed flakiness of context_lost tests on GPU bots.

Fixed flakiness of context_lost tests on GPU bots.

Two failures were observed:

1. If the GPU process crashed between EstablishGpuChannel and
   CreateViewCommandBuffer, the channel would never be properly marked
   as lost. Now, if CreateViewCommandBuffer fails, mark the
   GpuChannelHost as lost on the renderer side. The RenderThreadImpl
   will then establish a new connection to the new GPU process.

2. In Issue 308675, support was added to allow Telemetry to navigate
   to about:gpucrash, specifically for this test. It turns out that
   each navigation from Telemetry was provoking *two* GPU process
   crashes. Depending on when they came in, the test would
   intermittently receive two lost context events and fail. Squelch
   the second debug URL handling in NavigationControllerImpl.

These fix the locally reproducible failures of this test.

BUG=365904
TEST=src/content/test/gpu/run_gpu_test.py context_lost --browser=debug --show-stdout -v --page-filter=WebGLContextLostFromGPUProcessExit --page-repeat=30

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

[Ken Russell (switch to Gerrit), 2014-05-09 22:39:33]

Please review. The flakiness of this test is the last blocker toward putting the
GPU tryservers on the commit queue and making the GPU bots tree closers.

[jam, 2014-05-09 22:47:46]

https://codereview.chromium.org/277113002/diff/1/content/browser/frame_host/d...
File content/browser/frame_host/debug_urls.h (right):

https://codereview.chromium.org/277113002/diff/1/content/browser/frame_host/d...
content/browser/frame_host/debug_urls.h:18: // Checks if the given URL is a url
used for debugging purposes.
nit: expand this comment since it's a bit confusing at first glance why there's
this method and the one below. i.e. say something like this is a superset of
IsRendererDebugURL and debugging urls that are handled in the browser process

https://codereview.chromium.org/277113002/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/277113002/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigation_controller_impl.cc:1025:
cc::switches::kEnableGpuBenchmarking);
i can't tell what kEnableGpuBenchmarking has to do with this code?

[piman, 2014-05-09 22:50:41]

LGTM for content/common/gpu.

[jam, 2014-05-09 22:55:29]

+creis for navigation_controller_impl.cc, he knows this code better than me

[Ken Russell (switch to Gerrit), 2014-05-09 23:05:29]

On 2014/05/09 22:55:29, jam wrote:
> +creis for navigation_controller_impl.cc, he knows this code better than me

Note that this fixes a problem which was introduced in
https://codereview.chromium.org/27603004 .

I would really appreciate it if this could be reviewed today because this test
failure is the last blocker for making the GPU bots tree closers.

[Charlie Reis, 2014-05-09 23:21:51]

Looks right, after some digging.  Just a few comment requests and it should be
good to go.

https://codereview.chromium.org/277113002/diff/1/content/browser/frame_host/d...
File content/browser/frame_host/debug_urls.cc (right):

https://codereview.chromium.org/277113002/diff/1/content/browser/frame_host/d...
content/browser/frame_host/debug_urls.cc:86: (url.host() ==
kChromeUIBrowserCrashHost ||
I worry about this getting stale when people add things to HandleDebugURL but
not here.  Maybe there's a good place to add a comment to make sure both get
updated together?

https://codereview.chromium.org/277113002/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/277113002/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigation_controller_impl.cc:1022: // debug URLs
must be handled exactly once.
Sorry, it took me a while to understand what this comment meant, and thus why
this was correct.  Can we rewrite it to be a little more concrete?  Something
like:

"We call RewriteURLIfNecessary twice: once when page navigation begins in
CreateNavigationEntry, and once here when it commits.  For debug URLs, rewriting
the URL causes an action to occur and thus should not be done a second time."

https://codereview.chromium.org/277113002/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigation_controller_impl.cc:1025:
cc::switches::kEnableGpuBenchmarking);
On 2014/05/09 22:47:46, jam wrote:
> i can't tell what kEnableGpuBenchmarking has to do with this code?

+1.  I think we want to skip the second rewriting call for all debug URLs,
right?

[Ken Russell (switch to Gerrit), 2014-05-09 23:43:05]

https://codereview.chromium.org/277113002/diff/1/content/browser/frame_host/d...
File content/browser/frame_host/debug_urls.cc (right):

https://codereview.chromium.org/277113002/diff/1/content/browser/frame_host/d...
content/browser/frame_host/debug_urls.cc:86: (url.host() ==
kChromeUIBrowserCrashHost ||
On 2014/05/09 23:21:51, Charlie Reis wrote:
> I worry about this getting stale when people add things to HandleDebugURL but
> not here.  Maybe there's a good place to add a comment to make sure both get
> updated together?

Added comments to both HandleDebugURL and IsDebugURL.

https://codereview.chromium.org/277113002/diff/1/content/browser/frame_host/d...
File content/browser/frame_host/debug_urls.h (right):

https://codereview.chromium.org/277113002/diff/1/content/browser/frame_host/d...
content/browser/frame_host/debug_urls.h:18: // Checks if the given URL is a url
used for debugging purposes.
On 2014/05/09 22:47:46, jam wrote:
> nit: expand this comment since it's a bit confusing at first glance why
there's
> this method and the one below. i.e. say something like this is a superset of
> IsRendererDebugURL and debugging urls that are handled in the browser process

Done.

https://codereview.chromium.org/277113002/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/277113002/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigation_controller_impl.cc:1022: // debug URLs
must be handled exactly once.
On 2014/05/09 23:21:51, Charlie Reis wrote:
> Sorry, it took me a while to understand what this comment meant, and thus why
> this was correct.  Can we rewrite it to be a little more concrete?  Something
> like:
> 
> "We call RewriteURLIfNecessary twice: once when page navigation begins in
> CreateNavigationEntry, and once here when it commits.  For debug URLs,
rewriting
> the URL causes an action to occur and thus should not be done a second time."

Yes, thanks. Done.

https://codereview.chromium.org/277113002/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigation_controller_impl.cc:1025:
cc::switches::kEnableGpuBenchmarking);
On 2014/05/09 23:21:51, Charlie Reis wrote:
> On 2014/05/09 22:47:46, jam wrote:
> > i can't tell what kEnableGpuBenchmarking has to do with this code?
> 
> +1.  I think we want to skip the second rewriting call for all debug URLs,
> right?

Most of the time, the browser's debug URL handler will ignore debug URLs. I only
added support for processing them when kEnableGpuBenchmarking is enabled, since
most of the time they are navigated to by the omnibox. See Issue 308675 and
https://codereview.chromium.org/27603004 , and DebugURLHandler in
browser_url_handler_impl.cc.

I was aiming for the minimum behavioral change with this CL. I'd be glad to
expand the effects if you like, but I think it's worth being conservative.

[Charlie Reis, 2014-05-09 23:53:39]

Thanks.   LGTM with one change.

https://codereview.chromium.org/277113002/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/277113002/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigation_controller_impl.cc:1025:
cc::switches::kEnableGpuBenchmarking);
On 2014/05/09 23:43:05, Ken Russell wrote:
> On 2014/05/09 23:21:51, Charlie Reis wrote:
> > On 2014/05/09 22:47:46, jam wrote:
> > > i can't tell what kEnableGpuBenchmarking has to do with this code?
> > 
> > +1.  I think we want to skip the second rewriting call for all debug URLs,
> > right?
> 
> Most of the time, the browser's debug URL handler will ignore debug URLs. I
only
> added support for processing them when kEnableGpuBenchmarking is enabled,
since
> most of the time they are navigated to by the omnibox. See Issue 308675 and
> https://codereview.chromium.org/27603004 , and DebugURLHandler in
> browser_url_handler_impl.cc.
> 
> I was aiming for the minimum behavioral change with this CL. I'd be glad to
> expand the effects if you like, but I think it's worth being conservative.

Oh, I misunderstood what was happening.  Yes, let's keep the flag check here,
since it only matters in that case.  (It's admittedly a little awkward to have
this logic here, but maybe we can revisit how you're handling these URLs later.)

Accordingly, let's change the comment above to:

"We call RewriteURLIfNecessary twice: once when page navigation begins in
CreateNavigationEntry, and once here when it commits.  With the
kEnableGpuBenchmarking flag, the rewriting step includes handling debug URLs
which cause an action to occur, and thus we should not rewrite them a second
time."

[Ken Russell (switch to Gerrit), 2014-05-09 23:56:40]

https://codereview.chromium.org/277113002/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/277113002/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigation_controller_impl.cc:1025:
cc::switches::kEnableGpuBenchmarking);
On 2014/05/09 23:53:40, Charlie Reis wrote:
> On 2014/05/09 23:43:05, Ken Russell wrote:
> > On 2014/05/09 23:21:51, Charlie Reis wrote:
> > > On 2014/05/09 22:47:46, jam wrote:
> > > > i can't tell what kEnableGpuBenchmarking has to do with this code?
> > > 
> > > +1.  I think we want to skip the second rewriting call for all debug URLs,
> > > right?
> > 
> > Most of the time, the browser's debug URL handler will ignore debug URLs. I
> only
> > added support for processing them when kEnableGpuBenchmarking is enabled,
> since
> > most of the time they are navigated to by the omnibox. See Issue 308675 and
> > https://codereview.chromium.org/27603004 , and DebugURLHandler in
> > browser_url_handler_impl.cc.
> > 
> > I was aiming for the minimum behavioral change with this CL. I'd be glad to
> > expand the effects if you like, but I think it's worth being conservative.
> 
> Oh, I misunderstood what was happening.  Yes, let's keep the flag check here,
> since it only matters in that case.  (It's admittedly a little awkward to have
> this logic here, but maybe we can revisit how you're handling these URLs
later.)
> 
> Accordingly, let's change the comment above to:
> 
> "We call RewriteURLIfNecessary twice: once when page navigation begins in
> CreateNavigationEntry, and once here when it commits.  With the
> kEnableGpuBenchmarking flag, the rewriting step includes handling debug URLs
> which cause an action to occur, and thus we should not rewrite them a second
> time."

Done.

[Ken Russell (switch to Gerrit), 2014-05-09 23:58:22]

The CQ bit was checked by kbr@chromium.org

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

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kbr@chromium.org/277113002/40001

[commit-bot: I haz the power, 2014-05-10 03:50:05]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-10 04:41:35]

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

[commit-bot: I haz the power, 2014-05-10 04:41:35]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[Ken Russell (switch to Gerrit), 2014-05-11 06:59:10]

The CQ bit was checked by kbr@chromium.org

[commit-bot: I haz the power, 2014-05-11 06:59:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kbr@chromium.org/277113002/40001

[commit-bot: I haz the power, 2014-05-11 08:11:41]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-11 09:30:32]

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

[commit-bot: I haz the power, 2014-05-11 09:30:33]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[Ken Russell (switch to Gerrit), 2014-05-12 09:00:56]

The CQ bit was checked by kbr@chromium.org

[commit-bot: I haz the power, 2014-05-12 09:01:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kbr@chromium.org/277113002/40001

[commit-bot: I haz the power, 2014-05-12 10:07:31]

Change committed as 269763