Expose WebGL extension WEBGL_debug_renderer_info to Google domains

Expose WebGL extension WEBGL_debug_renderer_info to Google domains
through a chromium finch experiment.

We would launch with 5% first for data collection purpose.

Blink side CL: https://codereview.chromium.org/51323003/

BUG=309831
TEST=
R=asvitkine@chromium.org, bajones@chromium.org, jln@chromium.org, kbr@chromium.org, thestig@chromium.org

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

[Zhenyao Mo, 2013-10-29 22:44:23]

I am still working on adding a test for this to the GPU bot.

But the CL is tested locally and manually.

kbr: review the whole CL.
jln: content/common security review
asvitkine: finch related review
joi: content/public
jamesr: content/renderer

vangelis, darin: FYI

[Ken Russell (switch to Gerrit), 2013-10-29 23:15:35]

This looks almost perfect. One comment.

https://codereview.chromium.org/48113021/diff/1/content/browser/gpu/gpu_data_...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/48113021/diff/1/content/browser/gpu/gpu_data_...
content/browser/gpu/gpu_data_manager_impl_private.cc:1283: return true;
The test of the kEnableWebGLDebugRendererInfo switch should be moved after the
AllowWebGLDebugRendererInfoForDomain call below so that
ChromeContentBrowserClient::AllowWebGLDebugRendererInfoForDomain can be tested
too (using the kGoogleBaseURL flag).

[Zhenyao Mo, 2013-10-29 23:33:43]

https://codereview.chromium.org/48113021/diff/1/content/browser/gpu/gpu_data_...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/48113021/diff/1/content/browser/gpu/gpu_data_...
content/browser/gpu/gpu_data_manager_impl_private.cc:1283: return true;
On 2013/10/29 23:15:35, Ken Russell wrote:
> The test of the kEnableWebGLDebugRendererInfo switch should be moved after the
> AllowWebGLDebugRendererInfoForDomain call below so that
> ChromeContentBrowserClient::AllowWebGLDebugRendererInfoForDomain can be tested
> too (using the kGoogleBaseURL flag).

I just realized we can probably get rid of the kEnableWebGLDebugRendererInfo
switch, because we can use --force-fieldtrials combined with --google-base-url
to get this function to return true/false in all possible scenarios.  So
kEnableWebGLdebugRendererInfo doesn't need to be used in tests to emulate any
scenarios.

Also, it seems like finch has a mechanism to automatically insert a flag in
about:flags to let users opt in/out, so probably we don't need the
kDisableWebGLDebugRendererInfo switch also.

What do you think?

[Zhenyao Mo, 2013-10-29 23:43:36]

https://codereview.chromium.org/48113021/diff/1/content/browser/gpu/gpu_data_...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/48113021/diff/1/content/browser/gpu/gpu_data_...
content/browser/gpu/gpu_data_manager_impl_private.cc:1283: return true;
On 2013/10/29 23:33:43, Zhenyao Mo wrote:
> Also, it seems like finch has a mechanism to automatically insert a flag in
> about:flags to let users opt in/out, so probably we don't need the
> kDisableWebGLDebugRendererInfo switch also.
> 
> What do you think?

Never mind about this auto flag thing.  I read the documents more carefully, and
it's not what I thought it was.  We still need the disable- flag here.

[jamesr, 2013-10-29 23:44:00]

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

https://codereview.chromium.org/48113021/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:4351: NOTREACHED();
on old branches, i believe RenderViewImpl is the WebFrameClient implementation
that we use (instead of RenderFrameImpl) so if you plan to merge back you will
probably need to address this

[Ken Russell (switch to Gerrit), 2013-10-29 23:58:15]

https://codereview.chromium.org/48113021/diff/1/content/browser/gpu/gpu_data_...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/48113021/diff/1/content/browser/gpu/gpu_data_...
content/browser/gpu/gpu_data_manager_impl_private.cc:1283: return true;
On 2013/10/29 23:43:37, Zhenyao Mo wrote:
> On 2013/10/29 23:33:43, Zhenyao Mo wrote:
> > Also, it seems like finch has a mechanism to automatically insert a flag in
> > about:flags to let users opt in/out, so probably we don't need the
> > kDisableWebGLDebugRendererInfo switch also.
> > 
> > What do you think?
> 
> Never mind about this auto flag thing.  I read the documents more carefully,
and
> it's not what I thought it was.  We still need the disable- flag here.

OK. Using --force-fieldtrials and removing the kEnableWebGLDebugRendererInfo
switch sounds like a good plan.

[Jói, 2013-10-30 10:39:22]

//content/public LGTM

[vangelis, 2013-10-30 16:33:34]

On 2013/10/30 10:39:22, Jói wrote:
> //content/public LGTM

Mo, you seem to be missing the code that sets up the field trial.

[Alexei Svitkine (slow), 2013-10-30 16:52:45]

Please also make sure to file an experiment launch bug per the steps at
go/finch-guide.

[Zhenyao Mo, 2013-10-30 17:27:30]

On 2013/10/30 16:33:34, vangelis wrote:
> On 2013/10/30 10:39:22, Jói wrote:
> > //content/public LGTM
> 
> Mo, you seem to be missing the code that sets up the field trial.

Won't the trial be set up startup time via the Finch framework?

[Alexei Svitkine (slow), 2013-10-30 18:10:18]

No need to create the field trial in client code.

https://codereview.chromium.org/48113021/diff/1/content/browser/gpu/gpu_data_...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/48113021/diff/1/content/browser/gpu/gpu_data_...
content/browser/gpu/gpu_data_manager_impl_private.cc:324: return
trial->group_name() == kWebGLDebugRendererInfoFieldTrialEnabledName;
Instead, just use base::FieldTrialList::FindFullName() and just compare it to
kWebGLDebugRendererInfoFieldTrialEnabledName.

(If there's no trial, an empty string is returned.)

[Zhenyao Mo, 2013-10-30 20:20:50]

Revised.  Now it's purely chrome side CL.

kbr: main reviewer.
jln: _messages.h review
Akexei: field trial review
thestig: chrome/ owner review (there are no OWNER files in
chrome/browser/render_host and chrome/renderer)

https://codereview.chromium.org/48113021/diff/1/content/browser/gpu/gpu_data_...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/48113021/diff/1/content/browser/gpu/gpu_data_...
content/browser/gpu/gpu_data_manager_impl_private.cc:324: return
trial->group_name() == kWebGLDebugRendererInfoFieldTrialEnabledName;
On 2013/10/30 18:10:18, Alexei Svitkine wrote:
> Instead, just use base::FieldTrialList::FindFullName() and just compare it to
> kWebGLDebugRendererInfoFieldTrialEnabledName.
> 
> (If there's no trial, an empty string is returned.)

Done.

See chrome_render_message_filter.cc

[Ken Russell (switch to Gerrit), 2013-10-30 20:25:00]

The code looks good.

Could you please add a test under content/test/gpu/gpu_tests which invokes
Chrome with the appropriate command line arguments to trigger the field trial
and set the Google base URL to localhost, and assert that the extension is
available?

[Zhenyao Mo, 2013-10-30 20:31:46]

On 2013/10/30 20:25:00, Ken Russell wrote:
> The code looks good.
> 
> Could you please add a test under content/test/gpu/gpu_tests which invokes
> Chrome with the appropriate command line arguments to trigger the field trial
> and set the Google base URL to localhost, and assert that the extension is
> available?

Yes, I am working on it.  Unfortunately I still will only be able to test
manually instead of testing on try bots with the test, as it requires a change
in recipe outside source.

At the same time, can other people review the code?

kbr will be responsible to review the test once it's added.

[Alexei Svitkine (slow), 2013-10-30 20:37:57]

experiment code lgtm

[Ken Russell (switch to Gerrit), 2013-10-30 20:42:56]

On 2013/10/30 20:31:46, Zhenyao Mo wrote:
> Unfortunately I still will only be able to test
> manually instead of testing on try bots with the test, as it requires a change
> in recipe outside source.

That's fine. As long as the test runs successfully locally via run_gpu_test, we
can expect that it will work on the GPU bots.

[Lei Zhang, 2013-10-30 20:59:21]

lgtm

https://codereview.chromium.org/48113021/diff/150001/chrome/renderer/chrome_r...
File chrome/renderer/chrome_render_view_observer.cc (right):

https://codereview.chromium.org/48113021/diff/150001/chrome/renderer/chrome_r...
chrome/renderer/chrome_render_view_observer.cc:761:
GURL(frame->top()->document().securityOrigin().toString().utf8()),
Why do we use frame->top()->document() here, when other similar methods in this
file use frame->document()?

[Zhenyao Mo, 2013-10-30 21:02:59]

https://codereview.chromium.org/48113021/diff/150001/chrome/renderer/chrome_r...
File chrome/renderer/chrome_render_view_observer.cc (right):

https://codereview.chromium.org/48113021/diff/150001/chrome/renderer/chrome_r...
chrome/renderer/chrome_render_view_observer.cc:761:
GURL(frame->top()->document().securityOrigin().toString().utf8()),
On 2013/10/30 20:59:21, Lei Zhang wrote:
> Why do we use frame->top()->document() here, when other similar methods in
this
> file use frame->document()?

Ken, can you explain?  I copied this from your allowWebGL() code.

[jln (very slow on Chromium), 2013-10-30 21:17:07]

I think this would be worth a short discussion with the security-team, as I
don't understand the security consequences right away. I'll start a thread.

(also please, restrict any whitelist to httpS).

https://codereview.chromium.org/48113021/diff/150001/chrome/browser/renderer_...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

https://codereview.chromium.org/48113021/diff/150001/chrome/browser/renderer_...
chrome/browser/renderer_host/chrome_render_message_filter.cc:645: if
(!google_util::IsGoogleDomainUrl(origin, google_util::ALLOW_SUBDOMAIN,
Please, don't construct a whitelist without making it HTTPS only.

This would allow http://www.google.com, which could be anything with an attacker
on the network.

[Zhenyao Mo, 2013-10-30 21:35:15]

On 2013/10/30 21:17:07, jln wrote:
> I think this would be worth a short discussion with the security-team, as I
> don't understand the security consequences right away. I'll start a thread.
> 
> (also please, restrict any whitelist to httpS).
> 
>
https://codereview.chromium.org/48113021/diff/150001/chrome/browser/renderer_...
> File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):
> 
>
https://codereview.chromium.org/48113021/diff/150001/chrome/browser/renderer_...
> chrome/browser/renderer_host/chrome_render_message_filter.cc:645: if
> (!google_util::IsGoogleDomainUrl(origin, google_util::ALLOW_SUBDOMAIN,
> Please, don't construct a whitelist without making it HTTPS only.
> 
> This would allow http://www.google.com, which could be anything with an
attacker
> on the network.

The reason we want to restrict it to Google domain first is not because of
security concerns.  It's more like this is a feature we will remove if the
experiment returns negative results, so we don't want anyone to use it  yet. 
Does this remove the concerns you have?

[Ken Russell (switch to Gerrit), 2013-10-30 21:35:49]

https://codereview.chromium.org/48113021/diff/150001/chrome/renderer/chrome_r...
File chrome/renderer/chrome_render_view_observer.cc (right):

https://codereview.chromium.org/48113021/diff/150001/chrome/renderer/chrome_r...
chrome/renderer/chrome_render_view_observer.cc:761:
GURL(frame->top()->document().securityOrigin().toString().utf8()),
On 2013/10/30 21:03:00, Zhenyao Mo wrote:
> On 2013/10/30 20:59:21, Lei Zhang wrote:
> > Why do we use frame->top()->document() here, when other similar methods in
> this
> > file use frame->document()?
> 
> Ken, can you explain?  I copied this from your allowWebGL() code.

The basic idea for allowWebGL() was that we wanted to attribute GPU resets to
the outermost frame. If a frame embeds another one, then the embedding frame
should be considered "responsible" for having triggered the GPU reset. This
handles the case where a web page embeds malicious ads from a CDN.

I'm not sure whether using frame->document() would enable more use cases, such
as for example Maps embedded on some other web page. If people think that it
would, this can be changed. Otherwise it may be safer to use the style here.

[Zhenyao Mo, 2013-10-30 23:50:22]

kbr: test cases added.  Please take a look.

[bajones, 2013-10-31 00:04:48]

Test looks good with some nits.

https://codereview.chromium.org/48113021/diff/290001/content/test/gpu/gpu_tes...
File content/test/gpu/gpu_tests/webgl_debug_renderer_info.py (right):

https://codereview.chromium.org/48113021/diff/290001/content/test/gpu/gpu_tes...
content/test/gpu/gpu_tests/webgl_debug_renderer_info.py:15: wait_timeout = 20  #
seconds
wait_timeout not used. Please remove it.

https://codereview.chromium.org/48113021/diff/290001/content/test/gpu/gpu_tes...
content/test/gpu/gpu_tests/webgl_debug_renderer_info.py:33: console.log("Harness
injected.");
Please remove this console.log.

https://codereview.chromium.org/48113021/diff/290001/content/test/gpu/gpu_tes...
content/test/gpu/gpu_tests/webgl_debug_renderer_info.py:49:
options.AppendExtraBrowserArgs('--disable-webgl-debug-renderer-info')
Nit: Since the order of the browser args doesn't matter, it's a bit cleaner to
put this in the first "bypassed" earlier in the function

https://codereview.chromium.org/48113021/diff/290001/content/test/gpu/gpu_tes...
content/test/gpu/gpu_tests/webgl_debug_renderer_info.py:56: enabled = True
Nit: This enabled=True is not necessary. Remove it.

https://codereview.chromium.org/48113021/diff/290001/content/test/gpu/gpu_tes...
content/test/gpu/gpu_tests/webgl_debug_renderer_info.py:61: group =
optparse.OptionGroup(parser, 'WebGL conformance options')
Give the options group a better name. "Finch experiment options?"

https://codereview.chromium.org/48113021/diff/290001/content/test/gpu/gpu_tes...
content/test/gpu/gpu_tests/webgl_debug_renderer_info.py:63: help='Enable or
disable WebGLDebugRendererInfo finch experiment',
Would it be more helpful to have this read "Enable, disable, or bypass..."?

[Zhenyao Mo, 2013-10-31 00:26:12]

https://codereview.chromium.org/48113021/diff/290001/content/test/gpu/gpu_tes...
File content/test/gpu/gpu_tests/webgl_debug_renderer_info.py (right):

https://codereview.chromium.org/48113021/diff/290001/content/test/gpu/gpu_tes...
content/test/gpu/gpu_tests/webgl_debug_renderer_info.py:15: wait_timeout = 20  #
seconds
On 2013/10/31 00:04:49, bajones wrote:
> wait_timeout not used. Please remove it.

Done.

https://codereview.chromium.org/48113021/diff/290001/content/test/gpu/gpu_tes...
content/test/gpu/gpu_tests/webgl_debug_renderer_info.py:33: console.log("Harness
injected.");
On 2013/10/31 00:04:49, bajones wrote:
> Please remove this console.log.

Done.

https://codereview.chromium.org/48113021/diff/290001/content/test/gpu/gpu_tes...
content/test/gpu/gpu_tests/webgl_debug_renderer_info.py:49:
options.AppendExtraBrowserArgs('--disable-webgl-debug-renderer-info')
On 2013/10/31 00:04:49, bajones wrote:
> Nit: Since the order of the browser args doesn't matter, it's a bit cleaner to
> put this in the first "bypassed" earlier in the function

Done.

https://codereview.chromium.org/48113021/diff/290001/content/test/gpu/gpu_tes...
content/test/gpu/gpu_tests/webgl_debug_renderer_info.py:56: enabled = True
On 2013/10/31 00:04:49, bajones wrote:
> Nit: This enabled=True is not necessary. Remove it.

Done.

https://codereview.chromium.org/48113021/diff/290001/content/test/gpu/gpu_tes...
content/test/gpu/gpu_tests/webgl_debug_renderer_info.py:61: group =
optparse.OptionGroup(parser, 'WebGL conformance options')
On 2013/10/31 00:04:49, bajones wrote:
> Give the options group a better name. "Finch experiment options?"

Done.

https://codereview.chromium.org/48113021/diff/290001/content/test/gpu/gpu_tes...
content/test/gpu/gpu_tests/webgl_debug_renderer_info.py:63: help='Enable or
disable WebGLDebugRendererInfo finch experiment',
On 2013/10/31 00:04:49, bajones wrote:
> Would it be more helpful to have this read "Enable, disable, or bypass..."?

Done.

[jln (very slow on Chromium), 2013-10-31 06:49:47]

chrome/common/render_messages.h lgtm

(ignore my previous comment which was me not understanding the change)

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[bajones, 2013-10-31 16:56:36]

Revised test LGTM

[Zhenyao Mo, 2013-10-31 18:20:04]

Ken: please do the final review.

[Ken Russell (switch to Gerrit), 2013-10-31 21:21:55]

https://codereview.chromium.org/48113021/diff/430001/content/test/gpu/gpu_tes...
File content/test/gpu/gpu_tests/webgl_debug_renderer_info.py (right):

https://codereview.chromium.org/48113021/diff/430001/content/test/gpu/gpu_tes...
content/test/gpu/gpu_tests/webgl_debug_renderer_info.py:31: """
A CL was landed yesterday which changes how the harnesses are injected. You'll
need to restructure this into a separate .js file which is pulled in by
overriding an InjectedScripts method. Rebase your patch and look at the other
tests in this directory, which have all been updated.

https://codereview.chromium.org/48113021/diff/430001/content/test/gpu/gpu_tes...
content/test/gpu/gpu_tests/webgl_debug_renderer_info.py:58: help='Options:
enabled, disabled, bypassed',
Please add documentation for what these mean.
  enabled=experiment forced on, expect extension to be present
  disabled=experiment forced off, expect extension to be absent
  bypassed=experiment forced on, disabled via command line flag, expect
extension to be absent

[Zhenyao Mo, 2013-10-31 22:27:19]

Updated the test.  Please review again.

[Ken Russell (switch to Gerrit), 2013-10-31 22:47:27]

lgtm

[Zhenyao Mo, 2013-10-31 22:58:21]

Committed patchset #6 manually as r232231 (presubmit successful).