Prevent duplicate navigation to debug URLs from Telemetry.

Prevent duplicate navigation to debug URLs from Telemetry.

Previously Telemetry enabled navigation to Debug URLs by adding a custom
URL handler.  However, URL handlers can be called multiple times per
Navigation, and Debug URL actions must happen only once.

This CL enables Telemetry URL handling to be done by the normal
NavigationControllerImpl::LoadURLWithParams() to HandleDebugURL() path.

This also removes the prior workaround added in crrev.com/277113002 .

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

[vmiura, 2014-07-23 22:45:14]

Please review.  This is targeting a P1 flakiness bug in GPU bots.

[Ken Russell (switch to Gerrit), 2014-07-23 23:03:04]

Thanks for diagnosing and fixing this.

The code changes look good to me but I'd like to see a test land at the same
time as this, as we're discussing offline.

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

https://codereview.chromium.org/418733002/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigation_controller_impl.cc:1043: bool skip_rewrite
=
Could you also remove IsDebugURL entirely from
content/browser/frame_host/debug_urls.{cc,h}? It shouldn't be referenced any
more.

[Charlie Reis, 2014-07-23 23:04:18]

I'm mainly deferring to kbr@, since I'm not 100% sure on the implications.  See
one question below about IsRendererDebugURL, which is unrelated to the telemetry
stuff.

https://codereview.chromium.org/418733002/diff/1/content/browser/browser_url_...
File content/browser/browser_url_handler_impl.cc (left):

https://codereview.chromium.org/418733002/diff/1/content/browser/browser_url_...
content/browser/browser_url_handler_impl.cc:89: return IsRendererDebugURL(*url);
Why is this ok to delete?

[Ken Russell (switch to Gerrit), 2014-07-23 23:08:44]

https://codereview.chromium.org/418733002/diff/1/content/browser/browser_url_...
File content/browser/browser_url_handler_impl.cc (left):

https://codereview.chromium.org/418733002/diff/1/content/browser/browser_url_...
content/browser/browser_url_handler_impl.cc:89: return IsRendererDebugURL(*url);
On 2014/07/23 23:04:17, Charlie Reis wrote:
> Why is this ok to delete?

Yes, that's a good point. This should instead be reverted back to its state from
before crrev.com/27603004.

[vmiura, 2014-07-23 23:39:59]

https://codereview.chromium.org/418733002/diff/1/content/browser/browser_url_...
File content/browser/browser_url_handler_impl.cc (left):

https://codereview.chromium.org/418733002/diff/1/content/browser/browser_url_...
content/browser/browser_url_handler_impl.cc:89: return IsRendererDebugURL(*url);
On 2014/07/23 23:08:43, Ken Russell wrote:
> On 2014/07/23 23:04:17, Charlie Reis wrote:
> > Why is this ok to delete?
> 
> Yes, that's a good point. This should instead be reverted back to its state
from
> before crrev.com/27603004.

Done.  I'm not sure what that's there for, but I don't see a need to remove it.

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

https://codereview.chromium.org/418733002/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigation_controller_impl.cc:1043: bool skip_rewrite
=
On 2014/07/23 23:03:03, Ken Russell wrote:
> Could you also remove IsDebugURL entirely from
> content/browser/frame_host/debug_urls.{cc,h}? It shouldn't be referenced any
> more.

Done.

[Ken Russell (switch to Gerrit), 2014-07-23 23:53:58]

Thanks. Looks good modulo explicit test.

https://codereview.chromium.org/418733002/diff/20001/content/browser/browser_...
File content/browser/browser_url_handler_impl.cc (right):

https://codereview.chromium.org/418733002/diff/20001/content/browser/browser_...
content/browser/browser_url_handler_impl.cc:7: #include "base/command_line.h"
No longer needed.

https://codereview.chromium.org/418733002/diff/20001/content/browser/browser_...
content/browser/browser_url_handler_impl.cc:9: #include "cc/base/switches.h"
No longer needed.

[vmiura, 2014-07-24 23:40:40]

+ nasko for gpu_messages.h review.

I've added the discussed unit test using the about://gpu information page to
read back the GPU process crash count.

https://codereview.chromium.org/418733002/diff/20001/content/browser/browser_...
File content/browser/browser_url_handler_impl.cc (right):

https://codereview.chromium.org/418733002/diff/20001/content/browser/browser_...
content/browser/browser_url_handler_impl.cc:7: #include "base/command_line.h"
On 2014/07/23 23:53:58, Ken Russell wrote:
> No longer needed.

Done.

https://codereview.chromium.org/418733002/diff/20001/content/browser/browser_...
content/browser/browser_url_handler_impl.cc:9: #include "cc/base/switches.h"
On 2014/07/23 23:53:57, Ken Russell wrote:
> No longer needed.

Done.

[Ken Russell (switch to Gerrit), 2014-07-25 00:53:09]

+more security team reviewers for gpu_messages.h

[Ken Russell (switch to Gerrit), 2014-07-25 01:08:27]

Looks good overall. Only one significant comment.

https://codereview.chromium.org/418733002/diff/60001/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/418733002/diff/60001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_data_manager_impl_private.cc:761:
gpu_info_.process_crash_count++;
I'm sorry to be pedantic but could you please move the static variables in
GpuProcessHost::~GpuProcessHost into static class members and provide a static
method to query the gpu_crash_count? This should get the crash count from the
GpuProcessHost instead of keeping its own copy. Also, this should be moved up
one line. At this point the GpuDataManagerImpl can't be accessed in a thread
safe manner. (The above object is an "unlocker", not a locker.)

https://codereview.chromium.org/418733002/diff/60001/content/test/gpu/gpu_tes...
File content/test/gpu/gpu_tests/context_lost.py (right):

https://codereview.chromium.org/418733002/diff/60001/content/test/gpu/gpu_tes...
content/test/gpu/gpu_tests/context_lost.py:132: number_of_crashes =int(
space after '='

https://codereview.chromium.org/418733002/diff/60001/content/test/gpu/gpu_tes...
content/test/gpu/gpu_tests/context_lost.py:203: class
GPUProcessExitPageCrashesOnce(page.Page):
Suggest something like "GPUProcessCrashesExactlyOnce" instead.

https://codereview.chromium.org/418733002/diff/60001/content/test/gpu/gpu_tes...
content/test/gpu/gpu_tests/context_lost.py:212: self.number_of_gpu_process_kills
= 2
Running the test twice looks good. Just checking though, was this intentional?

[Ken Russell (switch to Gerrit), 2014-07-25 01:08:55]

+zmo as FYI for GPU data manager changes

[nasko, 2014-07-25 06:40:24]

gpu_messages.h LGTM

[vmiura, 2014-07-25 19:56:03]

Addressed comments.  PTAL.

https://codereview.chromium.org/418733002/diff/60001/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/418733002/diff/60001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_data_manager_impl_private.cc:761:
gpu_info_.process_crash_count++;
On 2014/07/25 01:08:26, Ken Russell wrote:
> I'm sorry to be pedantic but could you please move the static variables in
> GpuProcessHost::~GpuProcessHost into static class members and provide a static
> method to query the gpu_crash_count? This should get the crash count from the
> GpuProcessHost instead of keeping its own copy. Also, this should be moved up
> one line. At this point the GpuDataManagerImpl can't be accessed in a thread
> safe manner. (The above object is an "unlocker", not a locker.)

Done.

https://codereview.chromium.org/418733002/diff/60001/content/test/gpu/gpu_tes...
File content/test/gpu/gpu_tests/context_lost.py (right):

https://codereview.chromium.org/418733002/diff/60001/content/test/gpu/gpu_tes...
content/test/gpu/gpu_tests/context_lost.py:132: number_of_crashes =int(
On 2014/07/25 01:08:27, Ken Russell wrote:
> space after '='

Done.

https://codereview.chromium.org/418733002/diff/60001/content/test/gpu/gpu_tes...
content/test/gpu/gpu_tests/context_lost.py:203: class
GPUProcessExitPageCrashesOnce(page.Page):
On 2014/07/25 01:08:26, Ken Russell wrote:
> Suggest something like "GPUProcessCrashesExactlyOnce" instead.

Done.

https://codereview.chromium.org/418733002/diff/60001/content/test/gpu/gpu_tes...
content/test/gpu/gpu_tests/context_lost.py:212: self.number_of_gpu_process_kills
= 2
On 2014/07/25 01:08:26, Ken Russell wrote:
> Running the test twice looks good. Just checking though, was this intentional?

Yes I thought it would be good to check behavior of multiple kills in one
session, and keep the N>1 case in the test from rotting.

[Charlie Reis, 2014-07-25 20:01:37]

I'll defer on the gpu/ stuff, but the rest LGTM.

[vmiura, 2014-07-25 20:40:26]

Added: dtu@chromium.org, tonyg@chromium.org for review of changes in:
 tools/telemetry/telemetry/core/backends/chrome/system_info_backend.py

[vmiura, 2014-07-25 20:41:47]

Ken, as discussed I've changed the test to use Browser System Info instead of
scrubbing 'chrome://gpu' info page.

[tonyg, 2014-07-25 20:47:35]

https://codereview.chromium.org/418733002/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/chrome/system_info_backend.py
(left):

https://codereview.chromium.org/418733002/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/system_info_backend.py:15:
@decorators.Cache
Let's pull the Telemetry change into a separate CL that also includes a unittest
that demonstrates the fix for the bug.

[tonyg, 2014-07-25 20:50:14]

On 2014/07/25 20:47:35, tonyg wrote:
>
https://codereview.chromium.org/418733002/diff/100001/tools/telemetry/telemet...
> File tools/telemetry/telemetry/core/backends/chrome/system_info_backend.py
> (left):
> 
>
https://codereview.chromium.org/418733002/diff/100001/tools/telemetry/telemet...
> tools/telemetry/telemetry/core/backends/chrome/system_info_backend.py:15:
> @decorators.Cache
> Let's pull the Telemetry change into a separate CL that also includes a
unittest
> that demonstrates the fix for the bug.

Oh, and by bug, I mean the bug that caching was causing here (which isn't clear
for the patch description).

[vmiura, 2014-07-25 20:52:59]

On 2014/07/25 20:47:35, tonyg wrote:
>
https://codereview.chromium.org/418733002/diff/100001/tools/telemetry/telemet...
> File tools/telemetry/telemetry/core/backends/chrome/system_info_backend.py
> (left):
> 
>
https://codereview.chromium.org/418733002/diff/100001/tools/telemetry/telemet...
> tools/telemetry/telemetry/core/backends/chrome/system_info_backend.py:15:
> @decorators.Cache
> Let's pull the Telemetry change into a separate CL that also includes a
unittest
> that demonstrates the fix for the bug.

tonyg@, coud you give me some pointers what bug needs to be tested?

The unit test in this CL triggers
system_info.gpu.aux_attributes[u'process_crash_count'] to change.
If we cache the result then this test will fail.  However it is dependent on the
rest of this patch.

[Ken Russell (switch to Gerrit), 2014-07-25 21:21:36]

This is much nicer than scraping the about:gpu page. LGTM.

https://codereview.chromium.org/418733002/diff/100001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/418733002/diff/100001/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host.cc:73: base::Time
GpuProcessHost::last_gpu_crash_time_;
We need to ensure that this doesn't add any static initializers to the binary. I
think the Linux build verifies the number of static initializers, but am not
sure whether the try servers do.

https://codereview.chromium.org/418733002/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/chrome/system_info_backend.py
(left):

https://codereview.chromium.org/418733002/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/system_info_backend.py:15:
@decorators.Cache
On 2014/07/25 20:47:35, tonyg wrote:
> Let's pull the Telemetry change into a separate CL that also includes a
unittest
> that demonstrates the fix for the bug.

Talking with Victor about this. While I completely agree that a separate
unittest is a good idea -- and we've figured out how to write it -- I would like
to propose that we include that test with this patch rather than landing it
separately. That will double the time for making this fix, which we'd like to
land ASAP -- it will clear up flakiness currently seen on the GPU bots.

[vmiura, 2014-07-25 21:30:29]

As Ken mentioned, I've added a browser unittest in this patch.

[tonyg, 2014-07-25 21:50:46]

On 2014/07/25 20:52:59, vmiura wrote:
> On 2014/07/25 20:47:35, tonyg wrote:
> >
>
https://codereview.chromium.org/418733002/diff/100001/tools/telemetry/telemet...
> > File tools/telemetry/telemetry/core/backends/chrome/system_info_backend.py
> > (left):
> > 
> >
>
https://codereview.chromium.org/418733002/diff/100001/tools/telemetry/telemet...
> > tools/telemetry/telemetry/core/backends/chrome/system_info_backend.py:15:
> > @decorators.Cache
> > Let's pull the Telemetry change into a separate CL that also includes a
> unittest
> > that demonstrates the fix for the bug.
> 
> tonyg@, coud you give me some pointers what bug needs to be tested?
> 
> The unit test in this CL triggers
> system_info.gpu.aux_attributes[u'process_crash_count'] to change.
> If we cache the result then this test will fail.  However it is dependent on
the
> rest of this patch.

Why is process_crash_count in system info? That's not system info.

[Ken Russell (switch to Gerrit), 2014-07-25 21:57:36]

On 2014/07/25 21:50:46, tonyg wrote:
> On 2014/07/25 20:52:59, vmiura wrote:
> > On 2014/07/25 20:47:35, tonyg wrote:
> > >
> >
>
https://codereview.chromium.org/418733002/diff/100001/tools/telemetry/telemet...
> > > File tools/telemetry/telemetry/core/backends/chrome/system_info_backend.py
> > > (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/418733002/diff/100001/tools/telemetry/telemet...
> > > tools/telemetry/telemetry/core/backends/chrome/system_info_backend.py:15:
> > > @decorators.Cache
> > > Let's pull the Telemetry change into a separate CL that also includes a
> > unittest
> > > that demonstrates the fix for the bug.
> > 
> > tonyg@, coud you give me some pointers what bug needs to be tested?
> > 
> > The unit test in this CL triggers
> > system_info.gpu.aux_attributes[u'process_crash_count'] to change.
> > If we cache the result then this test will fail.  However it is dependent on
> the
> > rest of this patch.
> 
> Why is process_crash_count in system info? That's not system info.

It's in the GPU info's aux attributes, where there's other similar Chrome- and
GPU-process-specific information.

[tonyg, 2014-07-26 01:58:22]

On 2014/07/25 21:57:36, Ken Russell wrote:
> On 2014/07/25 21:50:46, tonyg wrote:
> > On 2014/07/25 20:52:59, vmiura wrote:
> > > On 2014/07/25 20:47:35, tonyg wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/418733002/diff/100001/tools/telemetry/telemet...
> > > > File
tools/telemetry/telemetry/core/backends/chrome/system_info_backend.py
> > > > (left):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/418733002/diff/100001/tools/telemetry/telemet...
> > > >
tools/telemetry/telemetry/core/backends/chrome/system_info_backend.py:15:
> > > > @decorators.Cache
> > > > Let's pull the Telemetry change into a separate CL that also includes a
> > > unittest
> > > > that demonstrates the fix for the bug.
> > > 
> > > tonyg@, coud you give me some pointers what bug needs to be tested?
> > > 
> > > The unit test in this CL triggers
> > > system_info.gpu.aux_attributes[u'process_crash_count'] to change.
> > > If we cache the result then this test will fail.  However it is dependent
on
> > the
> > > rest of this patch.
> > 
> > Why is process_crash_count in system info? That's not system info.
> 
> It's in the GPU info's aux attributes, where there's other similar Chrome- and
> GPU-process-specific information.

Can you point me at the source where there's similar Chrome information? My
understanding was that SystemInfo.getInfo should return static information about
the system. Not information that changes like crash counts.

When I run it on my MBP, everything looks static:
{
  "id": 0, 
  "result": {
    "gpu": {
      "auxAttributes": {
        "softwareRendering": false, 
        "vertexShaderVersion": "", 
        "optimus": true, 
        "glExtensions": "", 
        "driverDate": "", 
        "glRenderer": "", 
        "finalized": false, 
        "lenovoDcute": false, 
        "driverVersion": "", 
        "adapterLuid": 0.0, 
        "glResetNotificationStrategy": 0, 
        "driverVendor": "", 
        "initializationTime": 0.001755, 
        "can_lose_context": false, 
        "pixelShaderVersion": "", 
        "glWsVendor": "", 
        "glWsExtensions": "", 
        "amdSwitchable": false, 
        "glVendor": "", 
        "directRendering": true, 
        "glWsVersion": "", 
        "sandboxed": true, 
        "glVersion": ""
      }, 
      "featureStatus": {
        "flash_stage3d": "enabled", 
        "rasterization": "unavailable_software", 
        "threaded_rasterization": "disabled_off_ok", 
        "video_decode": "unavailable_software", 
        "flash_3d": "enabled", 
        "webgl": "enabled", 
        "video_encode": "enabled", 
        "2d_canvas": "enabled", 
        "gpu_compositing": "enabled", 
        "flash_stage3d_baseline": "enabled"
      }, 
      "devices": [
        {
          "vendorId": 4318, 
          "vendorString": "", 
          "deviceId": 4073, 
          "deviceString": ""
        }, 
        {
          "vendorId": 32902, 
          "vendorString": "", 
          "deviceId": 3366, 
          "deviceString": ""
        }
      ], 
      "driverBugWorkarounds": [
        "disable_arb_sync", 
        "disable_multimonitor_multisampling", 
        "init_varyings_without_static_use", 
        "needs_offscreen_buffer_workaround", 
        "scalarize_vec_and_mat_constructor_args", 
        "unfold_short_circuit_as_ternary_operation", 
        "validate_multisample_buffer_allocation"
      ]
    }, 
    "modelName": "MacBookPro", 
    "modelVersion": "11.3"
  }
}

If we're breaking that paradigm, then I'd like to detach Telemetry's concept of
system info from devtools' concept. In other words, keep a cacheable Telemetry
method that returns static information about the system (whatever the source).

It also seems like someone interested in the devtools protocol should opine
since this is changing the API.

[Ken Russell (switch to Gerrit), 2014-07-28 18:45:53]

On 2014/07/26 01:58:22, tonyg wrote:
> 
> Can you point me at the source where there's similar Chrome information? My
> understanding was that SystemInfo.getInfo should return static information
about
> the system. Not information that changes like crash counts.

The GPU information reported in about:gpu already changes dynamically; for
example, on a dual-GPU MacBook Pro, if the system switches from the integrated
to the discrete GPU, the active GPU reported will change.

The caching that was previously added to Telemetry for the system info was a
workaround for a limitation in DevTools that's since been fixed. Telemetry's
GetSystemInfo method should always return the most up-to-date information.

[vmiura, 2014-07-28 22:50:50]

PTAL, I think this is ready.

[Ken Russell (switch to Gerrit), 2014-07-28 23:01:35]

Nice work. Thanks for persevering with this fix. One issue then LGTM.

https://codereview.chromium.org/418733002/diff/160001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/context_lost.py (right):

https://codereview.chromium.org/418733002/diff/160001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/context_lost.py:133:
system_info.gpu.aux_attributes[u'process_crash_count']
This needs to call tab.browser.GetSystemInfo() again to fetch the latest data.

[tonyg, 2014-07-28 23:04:13]

lgtm with Ken's explanation.

[vmiura, 2014-07-28 23:08:22]

https://codereview.chromium.org/418733002/diff/160001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/context_lost.py (right):

https://codereview.chromium.org/418733002/diff/160001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/context_lost.py:133:
system_info.gpu.aux_attributes[u'process_crash_count']
On 2014/07/28 23:01:34, Ken Russell wrote:
> This needs to call tab.browser.GetSystemInfo() again to fetch the latest data.

Done.

[Ken Russell (switch to Gerrit), 2014-07-28 23:09:32]

LGTM again.

https://codereview.chromium.org/418733002/diff/180001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/context_lost.py (right):

https://codereview.chromium.org/418733002/diff/180001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/context_lost.py:130: # to attempt to catch latent
process crashes.
also, typo in comment: "in attempt to attempt to"

[vmiura, 2014-07-28 23:09:52]

The CQ bit was checked by vmiura@chromium.org

[vmiura, 2014-07-28 23:10:30]

The CQ bit was unchecked by vmiura@chromium.org

[vmiura, 2014-07-28 23:12:55]

https://codereview.chromium.org/418733002/diff/180001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/context_lost.py (right):

https://codereview.chromium.org/418733002/diff/180001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/context_lost.py:130: # to attempt to catch latent
process crashes.
On 2014/07/28 23:09:32, Ken Russell wrote:
> also, typo in comment: "in attempt to attempt to"

Done.

[vmiura, 2014-07-28 23:13:38]

The CQ bit was checked by vmiura@chromium.org

[commit-bot: I haz the power, 2014-07-28 23:14:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vmiura@chromium.org/418733002/200001

[commit-bot: I haz the power, 2014-07-29 08:01:46]

Change committed as 286117