Fix broken mixed script Rappor metric and add browser test

Fix broken mixed script Rappor metric and add browser test

This metric has not been recording meaningful data because of a mistake
while passing the mixed script hostname across the IPC boundary. We had
been passing a hostname and passing that into a GURL constructor to
pass into Rappor, but GURL expects a valid URL, including both scheme
and hostname, and returns an empty URL otherwise. So this change passes
the full origin (as a string), which correctly gets turned into a valid GURL.

Fix is based on a patch started by jialiul@ at
https://codereview.chromium.org/1424933014/

BUG=536975, 536981

[estark, 2016-06-09 06:34:30]

Description was changed from

==========
0;256;0c# Enter a description of the change.
Fix broken mixed script Rappor metric and add browser test

This metric has not been recording meaningful data because of a mistake
while passing the mixed script hostname across the IPC boundary.

Fix is based on a patch started by jialiul@ at
https://codereview.chromium.org/1424933014/

BUG=536975,536981
==========

to

==========
Fix broken mixed script Rappor metric and add browser test

This metric has not been recording meaningful data because of a mistake
while passing the mixed script hostname across the IPC boundary.

Fix is based on a patch started by jialiul@ at
https://codereview.chromium.org/1424933014/

BUG=536975,536981
==========

[estark, 2016-06-09 06:37:25]

estark@chromium.org changed reviewers:
+ felt@chromium.org, raymes@chromium.org

[estark, 2016-06-09 06:37:26]

raymes: can you please review chrome/browser/content_settings?
felt: can you please review chrome/browser/ui/content_settings?

lgarron, jialiul: could you confirm that we still actively want this Rappor
metric? If it's been broken for 6 months and hasn't been urgent enough to fix,
I'm wondering if we actually still need/want it?

[lgarron, 2016-06-09 23:39:43]

On 2016/06/09 at 06:37:26, estark wrote:
> raymes: can you please review chrome/browser/content_settings?
> felt: can you please review chrome/browser/ui/content_settings?
> 
> lgarron, jialiul: could you confirm that we still actively want this Rappor
metric? If it's been broken for 6 months and hasn't been urgent enough to fix,
I'm wondering if we actually still need/want it?

Yeah, I still actively want it, given that we haven't deprecated active mixed
content.

[felt, 2016-06-10 03:41:08]

lgtm

https://codereview.chromium.org/2048303002/diff/1/chrome/browser/content_sett...
File chrome/browser/content_settings/tab_specific_content_settings.cc (left):

https://codereview.chromium.org/2048303002/diff/1/chrome/browser/content_sett...
chrome/browser/content_settings/tab_specific_content_settings.cc:847: 
nit: why kill this line break?

https://codereview.chromium.org/2048303002/diff/1/chrome/browser/content_sett...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://codereview.chromium.org/2048303002/diff/1/chrome/browser/content_sett...
chrome/browser/content_settings/tab_specific_content_settings.cc:352:
GURL(details));
A note for posterity: this code path used to only send the host. Now it sends
the full URL. That's OK because it ends up in
GetDomainAndRegistrySampleFromGURL, which strips down the parts of the URL
appropriately.

https://codereview.chromium.org/2048303002/diff/1/chrome/renderer/content_set...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/2048303002/diff/1/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:486:
DidBlockContentType(CONTENT_SETTINGS_TYPE_MIXEDSCRIPT, origin.toString());
Man, this is a tricky bug. It wasn't obvious to me at first what was going on
with this bug (or bugfix). I think it's so tricky for two reasons:

1. It isn't particularly clear that this string is going to end up turned back
into a GURL.
2. I would have expected TabSpecificContentSettings::OnContentBlockedWithDetail
to DCHECK that GURL(base::UTF16ToUTF8(details)).is_valid(). This would ensure
that the contract was upheld by the caller a few layers back.

I'm not suggesting that we change anything, since the bug is now found and
fixed. However: it's something I'll keep in mind the next time I see a similar
pattern!

[estark, 2016-06-10 03:55:20]

https://codereview.chromium.org/2048303002/diff/1/chrome/browser/content_sett...
File chrome/browser/content_settings/tab_specific_content_settings.cc (left):

https://codereview.chromium.org/2048303002/diff/1/chrome/browser/content_sett...
chrome/browser/content_settings/tab_specific_content_settings.cc:847: 
On 2016/06/10 03:41:08, felt wrote:
> nit: why kill this line break?

Oops, undone.

https://codereview.chromium.org/2048303002/diff/1/chrome/renderer/content_set...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/2048303002/diff/1/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:486:
DidBlockContentType(CONTENT_SETTINGS_TYPE_MIXEDSCRIPT, origin.toString());
On 2016/06/10 03:41:08, felt wrote:
> Man, this is a tricky bug. It wasn't obvious to me at first what was going on
> with this bug (or bugfix). I think it's so tricky for two reasons:
> 
> 1. It isn't particularly clear that this string is going to end up turned back
> into a GURL.
> 2. I would have expected
TabSpecificContentSettings::OnContentBlockedWithDetail
> to DCHECK that GURL(base::UTF16ToUTF8(details)).is_valid(). This would ensure
> that the contract was upheld by the caller a few layers back.
> 
> I'm not suggesting that we change anything, since the bug is now found and
> fixed. However: it's something I'll keep in mind the next time I see a similar
> pattern!

Easy enough to add a DCHECK, so I did that. Also will crib jialiul's description
of the issue that she had put in the CL she started and put it in this one, so
that the fix is a bit easier to understand.

[estark, 2016-06-10 03:56:51]

Description was changed from

==========
Fix broken mixed script Rappor metric and add browser test

This metric has not been recording meaningful data because of a mistake
while passing the mixed script hostname across the IPC boundary.

Fix is based on a patch started by jialiul@ at
https://codereview.chromium.org/1424933014/

BUG=536975,536981
==========

to

==========
Fix broken mixed script Rappor metric and add browser test

This metric has not been recording meaningful data because of a mistake
while passing the mixed script hostname across the IPC boundary. We had
been passing a hostname and passing that into a GURL constructor to
pass into Rappor, but GURL expects a valid URL, including both scheme
and hostname, and returns an empty URL otherwise. So this change passes
the full origin (as a string), which correctly gets turned into a valid GURL.

Fix is based on a patch started by jialiul@ at
https://codereview.chromium.org/1424933014/

BUG=536975,536981
==========

[estark, 2016-06-12 17:12:16]

friendly ping to raymes

https://codereview.chromium.org/2048303002/diff/40001/chrome/browser/content_...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://codereview.chromium.org/2048303002/diff/40001/chrome/browser/content_...
chrome/browser/content_settings/tab_specific_content_settings.cc:849:
OnContentBlockedWithDetail(CONTENT_SETTINGS_TYPE_MIXEDSCRIPT,
note: an alternative could be to make OnContentBlockedWithDetail() accept an
empty |details| for mixed script, but enforce that if non-empty it must be a
valid GURL. That would not, however, catch a hypothetical bug in which the
renderer sends an empty URL.

[raymes, 2016-06-13 22:10:44]

lgtm

[estark, 2016-06-13 22:13:04]

estark@chromium.org changed reviewers:
+ sky@chromium.org

[estark, 2016-06-13 22:13:05]

sky@ can you please review chrome/renderer/content_settings_observer.cc?

[estark, 2016-06-15 19:33:31]

friendly ping to sky

[sky, 2016-06-16 16:22:47]

https://codereview.chromium.org/2048303002/diff/40001/chrome/renderer/content...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/2048303002/diff/40001/chrome/renderer/content...
chrome/renderer/content_settings_observer.cc:486:
DidBlockContentType(CONTENT_SETTINGS_TYPE_MIXEDSCRIPT, origin.toString());
I'm not familiar with this code, so why is toString better than host? A comment
would help. What is the expectation for the argument supplied to
DidBlockContentType? A comment in the header would be helpful.

[estark, 2016-06-16 16:56:58]

https://codereview.chromium.org/2048303002/diff/40001/chrome/renderer/content...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/2048303002/diff/40001/chrome/renderer/content...
chrome/renderer/content_settings_observer.cc:486:
DidBlockContentType(CONTENT_SETTINGS_TYPE_MIXEDSCRIPT, origin.toString());
On 2016/06/16 16:22:47, sky wrote:
> I'm not familiar with this code, so why is toString better than host? A
comment
> would help. What is the expectation for the argument supplied to
> DidBlockContentType? A comment in the header would be helpful.

The browser-side IPC handler expects |details| to be a valid GURL for this
content type (and a hostname is not a valid GURL). Added comments.

[sky, 2016-06-16 17:42:50]

https://codereview.chromium.org/2048303002/diff/40001/chrome/renderer/content...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/2048303002/diff/40001/chrome/renderer/content...
chrome/renderer/content_settings_observer.cc:486:
DidBlockContentType(CONTENT_SETTINGS_TYPE_MIXEDSCRIPT, origin.toString());
On 2016/06/16 16:56:58, estark wrote:
> On 2016/06/16 16:22:47, sky wrote:
> > I'm not familiar with this code, so why is toString better than host? A
> comment
> > would help. What is the expectation for the argument supplied to
> > DidBlockContentType? A comment in the header would be helpful.
> 
> The browser-side IPC handler expects |details| to be a valid GURL for this
> content type (and a hostname is not a valid GURL). Added comments.

Can DidBlockContentType take a GURL then? Also, it's my understanding toString()
may not return a url. It returns something 'like' a url. For if the
websecurityorigin is empty toString() returns 'null'. How do you know you won't
hit that code path?

[estark, 2016-06-16 18:29:23]

https://codereview.chromium.org/2048303002/diff/40001/chrome/renderer/content...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/2048303002/diff/40001/chrome/renderer/content...
chrome/renderer/content_settings_observer.cc:486:
DidBlockContentType(CONTENT_SETTINGS_TYPE_MIXEDSCRIPT, origin.toString());
On 2016/06/16 17:42:49, sky wrote:
> On 2016/06/16 16:56:58, estark wrote:
> > On 2016/06/16 16:22:47, sky wrote:
> > > I'm not familiar with this code, so why is toString better than host? A
> > comment
> > > would help. What is the expectation for the argument supplied to
> > > DidBlockContentType? A comment in the header would be helpful.
> > 
> > The browser-side IPC handler expects |details| to be a valid GURL for this
> > content type (and a hostname is not a valid GURL). Added comments.
> 
> Can DidBlockContentType take a GURL then? Also, it's my understanding
toString()
> may not return a url. It returns something 'like' a url. For if the
> websecurityorigin is empty toString() returns 'null'. How do you know you
won't
> hit that code path?

Ah, that's a good point. It appears that this could indeed be empty if the
origin is, for example, sandboxed. My understanding, also, is that this string
argument is a generic details argument that can't simply be changed to a GURL.
(Other content settings type send other string values in the |details| argument,
IIUC.)

I don't think I can spend any more time on this. lgarron and/or jialiul, since
you did the original metric + fix, are either of you interested in taking this
over, now that there's a test for it?

Otherwise I think we should just revert the commit that introduced this metric
until someone has time to fix it properly.

[Jialiu Lin, 2016-06-16 20:47:16]

jialiul@chromium.org changed reviewers:
+ jialiul@chromium.org

[Jialiu Lin, 2016-06-16 20:47:17]

https://codereview.chromium.org/2048303002/diff/40001/chrome/renderer/content...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/2048303002/diff/40001/chrome/renderer/content...
chrome/renderer/content_settings_observer.cc:486:
DidBlockContentType(CONTENT_SETTINGS_TYPE_MIXEDSCRIPT, origin.toString());
On 2016/06/16 at 18:29:23, estark wrote:
> On 2016/06/16 17:42:49, sky wrote:
> > On 2016/06/16 16:56:58, estark wrote:
> > > On 2016/06/16 16:22:47, sky wrote:
> > > > I'm not familiar with this code, so why is toString better than host? A
> > > comment
> > > > would help. What is the expectation for the argument supplied to
> > > > DidBlockContentType? A comment in the header would be helpful.
> > > 
> > > The browser-side IPC handler expects |details| to be a valid GURL for this
> > > content type (and a hostname is not a valid GURL). Added comments.
> > 
> > Can DidBlockContentType take a GURL then? Also, it's my understanding
toString()
> > may not return a url. It returns something 'like' a url. For if the
> > websecurityorigin is empty toString() returns 'null'. How do you know you
won't
> > hit that code path?
> 
> Ah, that's a good point. It appears that this could indeed be empty if the
origin is, for example, sandboxed. My understanding, also, is that this string
argument is a generic details argument that can't simply be changed to a GURL.
(Other content settings type send other string values in the |details| argument,
IIUC.)
> 
> I don't think I can spend any more time on this. lgarron and/or jialiul, since
you did the original metric + fix, are either of you interested in taking this
over, now that there's a test for it?
> 
> Otherwise I think we should just revert the commit that introduced this metric
until someone has time to fix it properly.

Here is the CL originally introduced this rappor metric.
https://codereview.chromium.org/1162123002
If nobody is actually consuming this rappor metric, I agree with estark that we
probably should simply revert it.

[estark, 2016-06-16 22:38:09]

On 2016/06/16 20:47:17, Jialiu Lin wrote:
>
https://codereview.chromium.org/2048303002/diff/40001/chrome/renderer/content...
> File chrome/renderer/content_settings_observer.cc (right):
> 
>
https://codereview.chromium.org/2048303002/diff/40001/chrome/renderer/content...
> chrome/renderer/content_settings_observer.cc:486:
> DidBlockContentType(CONTENT_SETTINGS_TYPE_MIXEDSCRIPT, origin.toString());
> On 2016/06/16 at 18:29:23, estark wrote:
> > On 2016/06/16 17:42:49, sky wrote:
> > > On 2016/06/16 16:56:58, estark wrote:
> > > > On 2016/06/16 16:22:47, sky wrote:
> > > > > I'm not familiar with this code, so why is toString better than host?
A
> > > > comment
> > > > > would help. What is the expectation for the argument supplied to
> > > > > DidBlockContentType? A comment in the header would be helpful.
> > > > 
> > > > The browser-side IPC handler expects |details| to be a valid GURL for
this
> > > > content type (and a hostname is not a valid GURL). Added comments.
> > > 
> > > Can DidBlockContentType take a GURL then? Also, it's my understanding
> toString()
> > > may not return a url. It returns something 'like' a url. For if the
> > > websecurityorigin is empty toString() returns 'null'. How do you know you
> won't
> > > hit that code path?
> > 
> > Ah, that's a good point. It appears that this could indeed be empty if the
> origin is, for example, sandboxed. My understanding, also, is that this string
> argument is a generic details argument that can't simply be changed to a GURL.
> (Other content settings type send other string values in the |details|
argument,
> IIUC.)
> > 
> > I don't think I can spend any more time on this. lgarron and/or jialiul,
since
> you did the original metric + fix, are either of you interested in taking this
> over, now that there's a test for it?
> > 
> > Otherwise I think we should just revert the commit that introduced this
metric
> until someone has time to fix it properly.
> 
> Here is the CL originally introduced this rappor metric.
> https://codereview.chromium.org/1162123002
> If nobody is actually consuming this rappor metric, I agree with estark that
we
> probably should simply revert it.

That looks like a different one, actually. That's
"ContentSettings.MixedScript.RanMixedScript", whereas this is
"ContentSettings.MixedScript.DisplayedShield".

[Jialiu Lin, 2016-06-20 20:25:57]

On 2016/06/16 at 22:38:09, estark wrote:
> On 2016/06/16 20:47:17, Jialiu Lin wrote:
> >
https://codereview.chromium.org/2048303002/diff/40001/chrome/renderer/content...
> > File chrome/renderer/content_settings_observer.cc (right):
> > 
> >
https://codereview.chromium.org/2048303002/diff/40001/chrome/renderer/content...
> > chrome/renderer/content_settings_observer.cc:486:
> > DidBlockContentType(CONTENT_SETTINGS_TYPE_MIXEDSCRIPT, origin.toString());
> > On 2016/06/16 at 18:29:23, estark wrote:
> > > On 2016/06/16 17:42:49, sky wrote:
> > > > On 2016/06/16 16:56:58, estark wrote:
> > > > > On 2016/06/16 16:22:47, sky wrote:
> > > > > > I'm not familiar with this code, so why is toString better than
host? A
> > > > > comment
> > > > > > would help. What is the expectation for the argument supplied to
> > > > > > DidBlockContentType? A comment in the header would be helpful.
> > > > > 
> > > > > The browser-side IPC handler expects |details| to be a valid GURL for
this
> > > > > content type (and a hostname is not a valid GURL). Added comments.
> > > > 
> > > > Can DidBlockContentType take a GURL then? Also, it's my understanding
> > toString()
> > > > may not return a url. It returns something 'like' a url. For if the
> > > > websecurityorigin is empty toString() returns 'null'. How do you know
you
> > won't
> > > > hit that code path?
> > > 
> > > Ah, that's a good point. It appears that this could indeed be empty if the
> > origin is, for example, sandboxed. My understanding, also, is that this
string
> > argument is a generic details argument that can't simply be changed to a
GURL.
> > (Other content settings type send other string values in the |details|
argument,
> > IIUC.)
> > > 
> > > I don't think I can spend any more time on this. lgarron and/or jialiul,
since
> > you did the original metric + fix, are either of you interested in taking
this
> > over, now that there's a test for it?
> > > 
> > > Otherwise I think we should just revert the commit that introduced this
metric
> > until someone has time to fix it properly.
> > 
> > Here is the CL originally introduced this rappor metric.
> > https://codereview.chromium.org/1162123002
> > If nobody is actually consuming this rappor metric, I agree with estark that
we
> > probably should simply revert it.
> 
> That looks like a different one, actually. That's
"ContentSettings.MixedScript.RanMixedScript", whereas this is
"ContentSettings.MixedScript.DisplayedShield".
My bad. It was actually added by lgarron@ in
https://codereview.chromium.org/1148773005