Allow content script insertion on about:-URLs.

Allow content script insertion on about:-URLs.

Adds a new key content_scripts[*].match_about_blank and
InjectionDetails.matchAboutBlank.
If true, content scripts will also be inserted in about:* frames
provided that its origin matches a pattern in the extension permissions.

BUG=76429, 55084, 146789
TEST=browser_tests: ExtensionApiTest.ContentScriptAboutBlank*:ScriptContextTest.GetEffectiveDocumentURL

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

[not at google - send to devlin, 2014-04-10 21:12:49]

I'd feel more comfortable only supporting about:blank in the way that we only
support chrome:favicon.

https://codereview.chromium.org/226663003/diff/20001/chrome/common/extensions...
File chrome/common/extensions/chrome_extensions_client.cc (right):

https://codereview.chromium.org/226663003/diff/20001/chrome/common/extensions...
chrome/common/extensions/chrome_extensions_client.cc:102: // Don't warn about
the about: scheme, because extensions cannot script
can we only support about:blank?

[robwu, 2014-04-10 21:22:03]

On 2014/04/10 21:12:49, kalman wrote:
> I'd feel more comfortable only supporting about:blank in the way that we only
> support chrome:favicon.

In Chrome, about:* is treated as about:blank (http://crbug.com/76429#c47).
Hence I think that it makes more sense to support all about:* URLs.
On ther other hand, I haven't seen anything that relies on the fact that
about:whatever is treated as about:blank.
WDYT?

> 
>
https://codereview.chromium.org/226663003/diff/20001/chrome/common/extensions...
> File chrome/common/extensions/chrome_extensions_client.cc (right):
> 
>
https://codereview.chromium.org/226663003/diff/20001/chrome/common/extensions...
> chrome/common/extensions/chrome_extensions_client.cc:102: // Don't warn about
> the about: scheme, because extensions cannot script
> can we only support about:blank?

At least about:srcdoc and about:blank, because they already have a clear
definition.

[not at google - send to devlin, 2014-04-10 21:25:51]

the bug we're trying to fix is that you can't inject script into an empty iframe
though, right? and that has a URL of about:blank, not about:srcdoc whatever that
is.

[robwu, 2014-04-10 21:31:37]

On 2014/04/10 21:25:51, kalman wrote:
> the bug we're trying to fix is that you can't inject script into an empty
iframe
> though, right? and that has a URL of about:blank, not about:srcdoc whatever
that
> is.

The bug that we're trying to fix is that content scripts can access the content
of
about:-frames, but content scripts cannot be declared for these frames.

Currently, the only way of running code in the context of these frames is to
detect
the creation of these frames, then run a content script from the parent frame.
Any
implementation of this idea in (JavaScript) extension code is highly
inefficient.

[not at google - send to devlin, 2014-04-10 22:25:58]

Ok. It makes sense then to allow about:* as a valid URL scheme and make it
implied by <all_urls>.

Charlie, can you think of a problem with allowing extensions to inject scripts
into arbitrary about: pages?

[Charlie Reis, 2014-04-10 22:50:29]

On 2014/04/10 22:25:58, kalman wrote:
> Ok. It makes sense then to allow about:* as a valid URL scheme and make it
> implied by <all_urls>.
> 
> Charlie, can you think of a problem with allowing extensions to inject scripts
> into arbitrary about: pages?

I'm probably missing background here, but there are some security policies based
on the assumption that about:blank is an unmodified empty page that commits
synchronously.  For example, we assume we can show a slow pending URL over
about:blank in a new tab without risking a phishing attack, at least until
another tab tries to access it.  (There doesn't appear to be a real concern in
that case because the user installs the extension, and the extension could
modify any page anyway.)

The only thing that comes to mind is that we wouldn't want this to apply to
swappedout:// (which is also registered as an empty page that commits
synchronously), but it doesn't sound like it's going to.

[not at google - send to devlin, 2014-04-10 22:52:47]

The use case is mostly that iframe have a src of about:blank by default, and
it's fairly common for web people to shove code in empty iframes to execute in a
sandbox (or something like that). Extensions want to script those.

[Charlie Reis, 2014-04-10 22:56:07]

On 2014/04/10 22:52:47, kalman wrote:
> The use case is mostly that iframe have a src of about:blank by default, and
> it's fairly common for web people to shove code in empty iframes to execute in
a
> sandbox (or something like that). Extensions want to script those.

Yeah, no concerns from my end.  I am curious why it's currently disallowed-- was
there a reason for it which has or hasn't changed?

[not at google - send to devlin, 2014-04-10 22:57:02]

On 2014/04/10 22:56:07, Charlie Reis wrote:
> On 2014/04/10 22:52:47, kalman wrote:
> > The use case is mostly that iframe have a src of about:blank by default, and
> > it's fairly common for web people to shove code in empty iframes to execute
in
> a
> > sandbox (or something like that). Extensions want to script those.
> 
> Yeah, no concerns from my end.  I am curious why it's currently disallowed--
was
> there a reason for it which has or hasn't changed?

The bug has some context. There was some issue with gmail iframes for a while,
then I think people we just cautious and/or it wasn't a priority to fix.

[Charlie Reis, 2014-04-10 23:10:15]

On 2014/04/10 22:57:02, kalman wrote:
> On 2014/04/10 22:56:07, Charlie Reis wrote:
> > On 2014/04/10 22:52:47, kalman wrote:
> > > The use case is mostly that iframe have a src of about:blank by default,
and
> > > it's fairly common for web people to shove code in empty iframes to
execute
> in
> > a
> > > sandbox (or something like that). Extensions want to script those.
> > 
> > Yeah, no concerns from my end.  I am curious why it's currently disallowed--
> was
> > there a reason for it which has or hasn't changed?
> 
> The bug has some context. There was some issue with gmail iframes for a while,
> then I think people we just cautious and/or it wasn't a priority to fix.

Wow, there's a lot of context and some subtle issues.  Justin sounds like he was
looking at this from a security perspective and has the background, so maybe
he's worth checking with.

Does this CL require all_urls permission?  I'm concerned about bullets #2 and #3
in comment 35, since top-level about:blank frames don't have a parent but *can*
inherit the security origin of their opener.  That means an extension with just
"about:blank" permission could take control of any web site that opens an
about:blank popup.

[robwu, 2014-04-10 23:16:14]

On 2014/04/10 23:10:15, Charlie Reis wrote:
> On 2014/04/10 22:57:02, kalman wrote:
> > On 2014/04/10 22:56:07, Charlie Reis wrote:
> > > On 2014/04/10 22:52:47, kalman wrote:
> > > > The use case is mostly that iframe have a src of about:blank by default,
> and
> > > > it's fairly common for web people to shove code in empty iframes to
> execute
> > in
> > > a
> > > > sandbox (or something like that). Extensions want to script those.
> > > 
> > > Yeah, no concerns from my end.  I am curious why it's currently
disallowed--
> > was
> > > there a reason for it which has or hasn't changed?
> > 
> > The bug has some context. There was some issue with gmail iframes for a
while,
> > then I think people we just cautious and/or it wasn't a priority to fix.
> 
> Wow, there's a lot of context and some subtle issues.  Justin sounds like he
was
> looking at this from a security perspective and has the background, so maybe
> he's worth checking with.
> 
> Does this CL require all_urls permission?  I'm concerned about bullets #2 and
#3
> in comment 35, since top-level about:blank frames don't have a parent but
*can*
> inherit the security origin of their opener.  That means an extension with
just
> "about:blank" permission could take control of any web site that opens an
> about:blank popup.

The permission checking is based on securityOrigin, which is inherit from the
parent frame (if applicable) and/or the window opener (if applicable). If
there's none (e.g. because the user typed "about:blank"), then the permission is
checked against the URL, which happens to be "about:blank".

[Charlie Reis, 2014-04-10 23:26:02]

On 2014/04/10 23:16:14, robwu wrote:
> On 2014/04/10 23:10:15, Charlie Reis wrote:
> > On 2014/04/10 22:57:02, kalman wrote:
> > > On 2014/04/10 22:56:07, Charlie Reis wrote:
> > > > On 2014/04/10 22:52:47, kalman wrote:
> > > > > The use case is mostly that iframe have a src of about:blank by
default,
> > and
> > > > > it's fairly common for web people to shove code in empty iframes to
> > execute
> > > in
> > > > a
> > > > > sandbox (or something like that). Extensions want to script those.
> > > > 
> > > > Yeah, no concerns from my end.  I am curious why it's currently
> disallowed--
> > > was
> > > > there a reason for it which has or hasn't changed?
> > > 
> > > The bug has some context. There was some issue with gmail iframes for a
> while,
> > > then I think people we just cautious and/or it wasn't a priority to fix.
> > 
> > Wow, there's a lot of context and some subtle issues.  Justin sounds like he
> was
> > looking at this from a security perspective and has the background, so maybe
> > he's worth checking with.
> > 
> > Does this CL require all_urls permission?  I'm concerned about bullets #2
and
> #3
> > in comment 35, since top-level about:blank frames don't have a parent but
> *can*
> > inherit the security origin of their opener.  That means an extension with
> just
> > "about:blank" permission could take control of any web site that opens an
> > about:blank popup.
> 
> The permission checking is based on securityOrigin, which is inherit from the
> parent frame (if applicable) and/or the window opener (if applicable). If
> there's none (e.g. because the user typed "about:blank"), then the permission
is
> checked against the URL, which happens to be "about:blank".

Great.  That works for me.

[not at google - send to devlin, 2014-04-11 00:19:38]

+justin

I'll take a proper look at this tomorrow.

[robwu, 2014-04-11 11:14:46]

It just occurred to me that there's an implementation flaw.

Because I use the serialization of securityOrigin() to determine whether a
content script needs to be injected or not, URL patterns that contain a
non-empty path (e.g. "http://example.com/anything*") will not be matched even if
the URL is identical to the frame's parent.

There are two ways to solve this problem:

1. Pass an extra parameter document_origin_url to CanExecuteScriptInPage. When
this value is set, user scripts are checked against the origin of the page.

2. Expose cookieURL() from Blink's Document via WebDocument. It's called
cookieURL(), but in reality it is actually an "effective security URL".

Which one do you prefer?

[jschuh, 2014-04-11 15:44:17]

I'll look over this today. My immediate concern is in cases where a subframe
navigation or manipulation would cause us to infer the wrong origin and
erroneously allow an extension to inject script.

[not at google - send to devlin, 2014-04-17 18:47:19]

ping @ justin

[jschuh, 2014-04-17 20:32:48]

On 2014/04/17 18:47:19, kalman wrote:
> ping @ justin

Sorry, slipped off my radar. I'll review today.

[jschuh, 2014-04-18 00:38:43]

Apologies for the delay, I had to trace through the loader and get confirmation
on how the origin is inherited in some frame navigation corner cases, but it
seems fine. So, lgtm.

[not at google - send to devlin, 2014-04-18 16:04:55]

see comment about just adding an option in addition to execute script.

e.g.

chrome.tabs.executeScript({
  file: ...,
  allFrames: true,
  aboutBlank: true
});

https://codereview.chromium.org/226663003/diff/20001/chrome/common/extensions...
File chrome/common/extensions/chrome_extensions_client.cc (right):

https://codereview.chromium.org/226663003/diff/20001/chrome/common/extensions...
chrome/common/extensions/chrome_extensions_client.cc:103: // these pages unless
they have the permission to edit the frame's origin.
@jschuh is the origin of a top-level about:blank "null", because that's what I
get in devtools? Why isn't it "about"?

https://codereview.chromium.org/226663003/diff/20001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/articles/match_patterns.html
(right):

https://codereview.chromium.org/226663003/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/templates/articles/match_patterns.html:192:
patterns.
what about top-level about: frames?

https://codereview.chromium.org/226663003/diff/20001/chrome/renderer/extensio...
File chrome/renderer/extensions/user_script_scheduler.cc (right):

https://codereview.chromium.org/226663003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_scheduler.cc:191: if
(document_url.SchemeIs(content::kAboutScheme)) {
mention that about: URLs inherit the origin from their creator frames, since
it's pretty subtle and I don't expect most reader are aware of that.

ideally link to the spec.

Incidentally I think the code would be a little cleaner if you pulled this out
into a helper function.

https://codereview.chromium.org/226663003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_scheduler.cc:210:
child_frame->document().url().spec());
use document_url here.

https://codereview.chromium.org/226663003/diff/20001/chrome/renderer/extensio...
File chrome/renderer/extensions/user_script_slave.cc (right):

https://codereview.chromium.org/226663003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_slave.cc:235: data_source_url =
document_origin_url;
pull this logic out into a helper function and use that from
user_script_scheduler.cc.

https://codereview.chromium.org/226663003/diff/20001/extensions/common/url_pa...
File extensions/common/url_pattern.cc (right):

https://codereview.chromium.org/226663003/diff/20001/extensions/common/url_pa...
extensions/common/url_pattern.cc:415: if (path_escaped_ == "*" || path_escaped_
== "/*")
this will change host permissions beyond just about:*. it will means that
extensions can specify

http://*

as host permissions, when they couldn't previously, for better or for worse.
which isn't forwards compatible.

I'm thinking that a better, less invasive way of doing what you're going for
here is to add another option to executeScript and content_scripts like
"aboutBlank" in addition to "allFrames". A host permission for this doesn't seem
right.

[robwu, 2014-04-20 15:57:24]

kalman: Reverted all changes to URL patterns and added a new property to toggle
about:blank injection. The security origin detection has not changed, except
that I'm comparing the origin URL against the set of effective host permissions
instead of the content script-specific "matches" patterns.

dtseng@:
Content scripts are now allowed to run in about:blank/about:srcdoc. I've set the
flag to false for ChromeVox. Let me know if you want to keep the functionality
as-is, or if you want me to change the flag for ChromeVox (so that the content
script also gets injected in about:blank).

[not at google - send to devlin, 2014-04-21 19:56:22]

https://codereview.chromium.org/226663003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/226663003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:1375: /* match_about_blank */
true,
comment typically goes after the value not before

and use // comments

https://codereview.chromium.org/226663003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/script_executor.h (right):

https://codereview.chromium.org/226663003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/script_executor.h:86: bool match_about_blank,
please make this an enum like every other option in this file. the reason why
they all use basically true/false enum is because functions calls would be crazy
with 6+ bool types.

https://codereview.chromium.org/226663003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/user_script_master.cc (right):

https://codereview.chromium.org/226663003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/user_script_master.cc:105: if (value == "about:blank")
{
there should be a constant declared somewhere for this?

though did you add this line because of a test failure, or speculatively? it
looks like this is invoked from an @include statement and I don't know why that
would have about:blank in it.

https://codereview.chromium.org/226663003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/user_script_master_unittest.cc (right):

https://codereview.chromium.org/226663003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/user_script_master_unittest.cc:235: "// @include
about:blank\n"
ah. do we really need to make this work for about:blank? does your use case
require user scripts to work?

https://codereview.chromium.org/226663003/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/tabs.json (right):

https://codereview.chromium.org/226663003/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/tabs.json:41: "matchesAboutBlank": {"type":
"boolean", "optional": true, "description": "If matchesAboutBlank is true, then
the code is also injected in about:blank and about:srcdoc frames if your
extension has access to its parent document. Code cannot be inserted in
top-level about:-frames. By default it is <code>false</code>."},
s/parent/opener ?

also, let's make this an action; "matchAboutBlank" not "matchesAboutBlank".

https://codereview.chromium.org/226663003/diff/40001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/articles/content_scripts.html
(right):

https://codereview.chromium.org/226663003/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/templates/articles/content_scripts.html:137:
<td><code>matches_about_blank<code></td>
same here, "match_about_blank".

https://codereview.chromium.org/226663003/diff/40001/chrome/renderer/extensio...
File chrome/renderer/extensions/user_script_slave.cc (right):

https://codereview.chromium.org/226663003/diff/40001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_slave.cc:204: // TODO(robwu): Iframes
with the sandbox HTML attribute are mistakenly
I .. think it makes sense to return an invalid URL if it's a sandbox. You can
check frame->document()->securityOrigin()->isUnique() for that.

https://codereview.chromium.org/226663003/diff/40001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_slave.cc:208: return
frame->document().url();
.GetOrigin()?

https://codereview.chromium.org/226663003/diff/40001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_slave.cc:241: const bool isAboutScheme =
data_source_url.SchemeIs(content::kAboutScheme);
is_about_scheme

https://codereview.chromium.org/226663003/diff/40001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_slave.cc:258: isAboutScheme ? NULL :
script,
don't have time to trace this down, why NULL here?

https://codereview.chromium.org/226663003/diff/40001/extensions/common/manife...
File extensions/common/manifest_constants.cc (right):

https://codereview.chromium.org/226663003/diff/40001/extensions/common/manife...
extensions/common/manifest_constants.cc:78: const char kMatchesAboutBlank[] =
"matches_about_blank";
kMatchAboutBlank / match_about_blank

[not at google - send to devlin, 2014-04-21 19:57:11]

+ dmazzoni as well re chromevox question

[dmazzoni, 2014-04-21 20:33:59]

On 2014/04/21 19:57:11, kalman wrote:
> + dmazzoni as well re chromevox question

We may not need ChromeVox if the user navigates to about:blank / opens an empty
tab, but we definitely need ChromeVox if a site creates a blank iframe and
starts adding content to it. I believe in those cases the url is about:blank.
Previously we had a big problem with this; it only works now because ChromeVox
is whitelisted to allow the content script to inject anywhere.

[dcheng, 2014-04-21 21:02:04]

https://codereview.chromium.org/226663003/diff/40001/chrome/renderer/extensio...
File chrome/renderer/extensions/user_script_slave.cc (right):

https://codereview.chromium.org/226663003/diff/40001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_slave.cc:205: // excluded by this method,
because their origin is "null" (i.e. unique).
I don't think this should match sandboxed iframes.

1) Can't extensions still inject by matching on a wildcard pattern?
2) If a sandboxed iframe wants to be treated as same origin, it can specify that
option. If not specified, it really should be treated as a unique origin that
can't match anything else.

[robwu, 2014-04-21 22:15:41]

kalman: addressed all of your comments.

ChromeVox: The match_about_blank flag state doesn't matter if the extension is
whitelisted, so I'll keep it false.

https://codereview.chromium.org/226663003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/226663003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:1375: /* match_about_blank */
true,
On 2014/04/21 19:56:22, kalman wrote:
> comment typically goes after the value not before
> 
> and use // comments

Done.

https://codereview.chromium.org/226663003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/script_executor.h (right):

https://codereview.chromium.org/226663003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/script_executor.h:86: bool match_about_blank,
On 2014/04/21 19:56:22, kalman wrote:
> please make this an enum like every other option in this file. the reason why
> they all use basically true/false enum is because functions calls would be
crazy
> with 6+ bool types.

Done.

https://codereview.chromium.org/226663003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/user_script_master.cc (right):

https://codereview.chromium.org/226663003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/user_script_master.cc:105: if (value == "about:blank")
{
On 2014/04/21 19:56:22, kalman wrote:
> there should be a constant declared somewhere for this?

Indeed, content::kAboutBlank if I'm not mistaken. Changed.

> 
> though did you add this line because of a test failure, or speculatively? it
> looks like this is invoked from an @include statement and I don't know why
that
> would have about:blank in it.

An "about:blank" glob would not grant access to about:blank unless the
matches_about_blank flag is set. It's easy to support "@include about:blank"
(just an extra conditional), so I've added this to be compatible with
Greasemonkey
(http://www.greasespot.net/2011/04/breaking-change-inject-into-aboutblank.html).

https://codereview.chromium.org/226663003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/user_script_master_unittest.cc (right):

https://codereview.chromium.org/226663003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/user_script_master_unittest.cc:235: "// @include
about:blank\n"
On 2014/04/21 19:56:22, kalman wrote:
> ah. do we really need to make this work for about:blank? does your use case
> require user scripts to work?

No, my use case does not require about:blank for user scripts. There are very
few user scripts that want to script about:blank, but supporting the feature is
easy (at this point), hence I've added support for it.
I'm neutral towards (not) supporting this feature, so if you'd rather have this
code removed, just say so.

(side note: Does Chrome still intend to support .user.js? Its implementation is
far from compatible with greasemonkey, and installing a user script for the
average user is impossible because of the standard requirement that "extensions"
need to be installed from the CWS.)

https://codereview.chromium.org/226663003/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/tabs.json (right):

https://codereview.chromium.org/226663003/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/tabs.json:41: "matchesAboutBlank": {"type":
"boolean", "optional": true, "description": "If matchesAboutBlank is true, then
the code is also injected in about:blank and about:srcdoc frames if your
extension has access to its parent document. Code cannot be inserted in
top-level about:-frames. By default it is <code>false</code>."},
On 2014/04/21 19:56:22, kalman wrote:
> s/parent/opener ?
> 
> also, let's make this an action; "matchAboutBlank" not "matchesAboutBlank".

Done.

https://codereview.chromium.org/226663003/diff/40001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/articles/content_scripts.html
(right):

https://codereview.chromium.org/226663003/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/templates/articles/content_scripts.html:137:
<td><code>matches_about_blank<code></td>
On 2014/04/21 19:56:22, kalman wrote:
> same here, "match_about_blank".

Done.

https://codereview.chromium.org/226663003/diff/40001/chrome/renderer/extensio...
File chrome/renderer/extensions/user_script_slave.cc (right):

https://codereview.chromium.org/226663003/diff/40001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_slave.cc:204: // TODO(robwu): Iframes
with the sandbox HTML attribute are mistakenly
On 2014/04/21 19:56:22, kalman wrote:
> I .. think it makes sense to return an invalid URL if it's a sandbox. You can
> check frame->document()->securityOrigin()->isUnique() for that.

Oops, this comment should be removed. I checked, and about:blank/about:srcdoc
can access its parent frame despite the sandbox attribute.

https://codereview.chromium.org/226663003/diff/40001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_slave.cc:205: // excluded by this method,
because their origin is "null" (i.e. unique).
On 2014/04/21 21:02:05, dcheng wrote:
> I don't think this should match sandboxed iframes.
> 
> 1) Can't extensions still inject by matching on a wildcard pattern?
> 2) If a sandboxed iframe wants to be treated as same origin, it can specify
that
> option. If not specified, it really should be treated as a unique origin that
> can't match anything else.

The "sandbox" attribute somehow doesn't apply to about:blank:
1. Visit http://jsfiddle.net/Q7tAw/show/
2. Open F12 devtools.
3. Change JavaScript context to the about:blank frame.
4. Type "parent" in the JS console and press enter.
5. Observe that the frame can access its parent document without being blocked.

Content scripts are declared for specific URLs, so the (non-)existence of the
sandbox attribute should not matter. Otherwise, adblockers could be circumvented
by sandboxing the iframe.

https://codereview.chromium.org/226663003/diff/40001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_slave.cc:208: return
frame->document().url();
On 2014/04/21 19:56:22, kalman wrote:
> .GetOrigin()?

Done.

https://codereview.chromium.org/226663003/diff/40001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_slave.cc:241: const bool isAboutScheme =
data_source_url.SchemeIs(content::kAboutScheme);
On 2014/04/21 19:56:22, kalman wrote:
> is_about_scheme

Done.

https://codereview.chromium.org/226663003/diff/40001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_slave.cc:258: isAboutScheme ? NULL :
script,
On 2014/04/21 19:56:22, kalman wrote:
> don't have time to trace this down, why NULL here?

Moved to separate variable, preceeded by a comment:
// If the page is about:blank, pass NULL instead of a UserScript. This
// ensures that the URL is checked against the extension's host permissions
// instead of the script's URL patterns.
const UserScript* script_or_null = is_about_scheme ? NULL : script;

https://codereview.chromium.org/226663003/diff/40001/extensions/common/manife...
File extensions/common/manifest_constants.cc (right):

https://codereview.chromium.org/226663003/diff/40001/extensions/common/manife...
extensions/common/manifest_constants.cc:78: const char kMatchesAboutBlank[] =
"matches_about_blank";
On 2014/04/21 19:56:22, kalman wrote:
> kMatchAboutBlank / match_about_blank

Done.

[not at google - send to devlin, 2014-04-21 22:34:41]

https://codereview.chromium.org/226663003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/226663003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:1375:
ScriptExecutor::MATCH_ABOUT_BLANK,
for backwards compatibility this should be DONT_...

https://codereview.chromium.org/226663003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/script_executor.h (right):

https://codereview.chromium.org/226663003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/script_executor.h:56: DONT_MATCH_ABOUT_BLANK,
switch the order of these, false should have the 0 value.

https://codereview.chromium.org/226663003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/user_script_master.cc (right):

https://codereview.chromium.org/226663003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/user_script_master.cc:106:
script->set_match_about_blank(true);
to answer your question: it might be easy, but it's still [a very small amount
of] code... let's just not change the way that user scripts work.

https://codereview.chromium.org/226663003/diff/60001/chrome/renderer/extensio...
File chrome/renderer/extensions/user_script_slave.cc (right):

https://codereview.chromium.org/226663003/diff/60001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_slave.cc:254: const UserScript*
script_or_null = is_about_scheme ? NULL : script;
I see. a bit of a hack to assume that's what the implementation of
CanExecuteScriptOnPage does.

it will end up querying GetActivePermissions:
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...
has that added the content script permissions at that point? I'm not sure, but
if not, this code won't work for manifest-declared content scripts.

I don't really understand why this needs to have a special case.

https://codereview.chromium.org/226663003/diff/60001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/content_scripts/about_blank_srcdoc/manifest.json
(right):

https://codereview.chromium.org/226663003/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/content_scripts/about_blank_srcdoc/manifest.json:12:
"matches": ["*://*/*"],
if you test with "permissions" and "matches" that don't overlap I think you'll
see that bug I mentioned previously.

[dcheng, 2014-04-21 22:35:17]

https://codereview.chromium.org/226663003/diff/40001/chrome/renderer/extensio...
File chrome/renderer/extensions/user_script_slave.cc (right):

https://codereview.chromium.org/226663003/diff/40001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_slave.cc:205: // excluded by this method,
because their origin is "null" (i.e. unique).
On 2014/04/21 22:15:41, robwu wrote:
> On 2014/04/21 21:02:05, dcheng wrote:
> > I don't think this should match sandboxed iframes.
> > 
> > 1) Can't extensions still inject by matching on a wildcard pattern?
> > 2) If a sandboxed iframe wants to be treated as same origin, it can specify
> that
> > option. If not specified, it really should be treated as a unique origin
that
> > can't match anything else.
> 
> The "sandbox" attribute somehow doesn't apply to about:blank:
> 1. Visit http://jsfiddle.net/Q7tAw/show/
> 2. Open F12 devtools.
> 3. Change JavaScript context to the about:blank frame.
> 4. Type "parent" in the JS console and press enter.
> 5. Observe that the frame can access its parent document without being
blocked.

Huh. I'm pretty sure that's a bug. I'll follow up in Blink on this.

> 
> Content scripts are declared for specific URLs, so the (non-)existence of the
> sandbox attribute should not matter. Otherwise, adblockers could be
circumvented
> by sandboxing the iframe.

That's the thing. Now we're mixing up origins and URLs, picking whichever one
happens to be convenient, and things start to get confusing. Wouldn't an
adblocker use wildcard permissions anyway, making the whole issue moot to begin
with?

[not at google - send to devlin, 2014-04-21 22:36:47]

https://codereview.chromium.org/226663003/diff/40001/chrome/renderer/extensio...
File chrome/renderer/extensions/user_script_slave.cc (right):

https://codereview.chromium.org/226663003/diff/40001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_slave.cc:205: // excluded by this method,
because their origin is "null" (i.e. unique).
On 2014/04/21 22:35:18, dcheng wrote:
> On 2014/04/21 22:15:41, robwu wrote:
> > On 2014/04/21 21:02:05, dcheng wrote:
> > > I don't think this should match sandboxed iframes.
> > > 
> > > 1) Can't extensions still inject by matching on a wildcard pattern?
> > > 2) If a sandboxed iframe wants to be treated as same origin, it can
specify
> > that
> > > option. If not specified, it really should be treated as a unique origin
> that
> > > can't match anything else.
> > 
> > The "sandbox" attribute somehow doesn't apply to about:blank:
> > 1. Visit http://jsfiddle.net/Q7tAw/show/
> > 2. Open F12 devtools.
> > 3. Change JavaScript context to the about:blank frame.
> > 4. Type "parent" in the JS console and press enter.
> > 5. Observe that the frame can access its parent document without being
> blocked.
> 
> Huh. I'm pretty sure that's a bug. I'll follow up in Blink on this.
> 
> > 
> > Content scripts are declared for specific URLs, so the (non-)existence of
the
> > sandbox attribute should not matter. Otherwise, adblockers could be
> circumvented
> > by sandboxing the iframe.
> 
> That's the thing. Now we're mixing up origins and URLs, picking whichever one
> happens to be convenient, and things start to get confusing. Wouldn't an
> adblocker use wildcard permissions anyway, making the whole issue moot to
begin
> with?

Yes seems like this code should be explicitly checking for an about: URL.

[robwu, 2014-04-21 23:21:45]

https://codereview.chromium.org/226663003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/226663003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:1375:
ScriptExecutor::MATCH_ABOUT_BLANK,
On 2014/04/21 22:34:41, kalman wrote:
> for backwards compatibility this should be DONT_...

Done.

https://codereview.chromium.org/226663003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/script_executor.h (right):

https://codereview.chromium.org/226663003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/script_executor.h:56: DONT_MATCH_ABOUT_BLANK,
On 2014/04/21 22:34:41, kalman wrote:
> switch the order of these, false should have the 0 value.

Done.

https://codereview.chromium.org/226663003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/user_script_master.cc (right):

https://codereview.chromium.org/226663003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/user_script_master.cc:106:
script->set_match_about_blank(true);
On 2014/04/21 22:34:41, kalman wrote:
> to answer your question: it might be easy, but it's still [a very small amount
> of] code... let's just not change the way that user scripts work.

Done.

https://codereview.chromium.org/226663003/diff/60001/chrome/renderer/extensio...
File chrome/renderer/extensions/user_script_slave.cc (right):

https://codereview.chromium.org/226663003/diff/60001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_slave.cc:254: const UserScript*
script_or_null = is_about_scheme ? NULL : script;
On 2014/04/21 22:34:41, kalman wrote:
> I see. a bit of a hack to assume that's what the implementation of
> CanExecuteScriptOnPage does.

I disliked the alternative (adding yet another (boolean) parameter to
CanExecuteScriptOnPage).

> 
> it will end up querying GetActivePermissions:
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...
> has that added the content script permissions at that point? I'm not sure, but
> if not, this code won't work for manifest-declared content scripts.

See my reply to your other comment.

> 
> I don't really understand why this needs to have a special case.

The intention is to always match as many about:blank pages as possible, based on
the origin and host permissions. The initial implementation (inserting on about
blank depending on the matches pattern did not work (because "about:"

https://codereview.chromium.org/226663003/diff/60001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/content_scripts/about_blank_srcdoc/manifest.json
(right):

https://codereview.chromium.org/226663003/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/content_scripts/about_blank_srcdoc/manifest.json:12:
"matches": ["*://*/*"],
On 2014/04/21 22:34:41, kalman wrote:
> if you test with "permissions" and "matches" that don't overlap I think you'll
> see that bug I mentioned previously.

After removing "permissions": ["*://*/*"], the browser tests still pass.

[not at google - send to devlin, 2014-04-21 23:37:38]

https://codereview.chromium.org/226663003/diff/60001/chrome/renderer/extensio...
File chrome/renderer/extensions/user_script_slave.cc (right):

https://codereview.chromium.org/226663003/diff/60001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_slave.cc:254: const UserScript*
script_or_null = is_about_scheme ? NULL : script;
On 2014/04/21 23:21:46, robwu wrote:
> On 2014/04/21 22:34:41, kalman wrote:
> > I see. a bit of a hack to assume that's what the implementation of
> > CanExecuteScriptOnPage does.
> 
> I disliked the alternative (adding yet another (boolean) parameter to
> CanExecuteScriptOnPage).
> 
> > 
> > it will end up querying GetActivePermissions:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...
> > has that added the content script permissions at that point? I'm not sure,
but
> > if not, this code won't work for manifest-declared content scripts.
> 
> See my reply to your other comment.
> 
> > 
> > I don't really understand why this needs to have a special case.
> 
> The intention is to always match as many about:blank pages as possible, based
on
> the origin and host permissions. The initial implementation (inserting on
about
> blank depending on the matches pattern did not work (because "about:"

We're in this code because we're running a declared content script, so we should
be using the content script's permission checking not the host permissions.

[dcheng, 2014-04-22 00:40:01]

https://codereview.chromium.org/226663003/diff/70001/chrome/renderer/extensio...
File chrome/renderer/extensions/user_script_slave.cc (right):

https://codereview.chromium.org/226663003/diff/70001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_slave.cc:198: // All pages served with
the about:-scheme inherit the security origin from
As I mentioned earlier, this comment is incorrect. This should only happen for
non-sandboxed iframes; the fact that it does not is a bug in Blink.

https://codereview.chromium.org/226663003/diff/70001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_slave.cc:206: return
GURL(frame->document().url()).GetOrigin();
The implementation here does not match the documentation. Here's what the
documentation says: "Content scripts will only be injected on pages if your
extension has access to the page's parent document."

Consider the case where a content script wants to match against
"http://example.com/path/*", and the extension manifest specifies
matches_about_blank. http://example.com/path/foo.html includes an about:blank
iframe. My understanding is that we will never match the about:blank iframe
because an "origin" of "http://example.com" will never match
"http://example.com/path/*", so no script will be injected.

I think we should either:
1) Follow the documentation and just match against the parent document URL. This
makes the checking window.opener (when you create a new about:blank window)
harder since we'll have to duplicate some logic to go poke at window.opener in
this case. But at least it will do what developers expect.

2) Update the documentation to explicitly mention that this needs to fall back
to origins for about:blank iframes/windows, and force extensions that really
want to match unique security origins to do so by specifying the appropriate
wildcard pattern.

Of these options, I strongly prefer #2 since security origins are well defined
(and we already have the code to make it work in Blink) and we won't have a
security model that's different from the rest of the web.

[robwu, 2014-04-22 13:29:51]

https://codereview.chromium.org/226663003/diff/60001/chrome/renderer/extensio...
File chrome/renderer/extensions/user_script_slave.cc (right):

https://codereview.chromium.org/226663003/diff/60001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_slave.cc:254: const UserScript*
script_or_null = is_about_scheme ? NULL : script;
On 2014/04/21 23:37:39, kalman wrote:
> On 2014/04/21 23:21:46, robwu wrote:
> > On 2014/04/21 22:34:41, kalman wrote:
> > > I see. a bit of a hack to assume that's what the implementation of
> > > CanExecuteScriptOnPage does.
> > 
> > I disliked the alternative (adding yet another (boolean) parameter to
> > CanExecuteScriptOnPage).
> > 
> > > 
> > > it will end up querying GetActivePermissions:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...
> > > has that added the content script permissions at that point? I'm not sure,
> but
> > > if not, this code won't work for manifest-declared content scripts.
> > 
> > See my reply to your other comment.
> > 
> > > 
> > > I don't really understand why this needs to have a special case.
> > 
> > The intention is to always match as many about:blank pages as possible,
based
> on
> > the origin and host permissions. The initial implementation (inserting on
> about
> > blank depending on the matches pattern did not work (because "about:"
> 
> We're in this code because we're running a declared content script, so we
should
> be using the content script's permission checking not the host permissions.

Submitted patch to Blink so I can get rid of this hack by using inheritedURL()
instead of securityOrigin() - https://codereview.chromium.org/246433004/.

https://codereview.chromium.org/226663003/diff/70001/chrome/renderer/extensio...
File chrome/renderer/extensions/user_script_slave.cc (right):

https://codereview.chromium.org/226663003/diff/70001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_slave.cc:206: return
GURL(frame->document().url()).GetOrigin();
On 2014/04/22 00:40:01, dcheng wrote:
> The implementation here does not match the documentation. Here's what the
> documentation says: "Content scripts will only be injected on pages if your
> extension has access to the page's parent document."
> 
> Consider the case where a content script wants to match against
> "http://example.com/path/*", and the extension manifest specifies
> matches_about_blank. http://example.com/path/foo.html includes an about:blank
> iframe. My understanding is that we will never match the about:blank iframe
> because an "origin" of "http://example.com" will never match
> "http://example.com/path/*", so no script will be injected.
> 
> I think we should either:
> 1) Follow the documentation and just match against the parent document URL.
This
> makes the checking window.opener (when you create a new about:blank window)
> harder since we'll have to duplicate some logic to go poke at window.opener in
> this case. But at least it will do what developers expect.
> 
> 2) Update the documentation to explicitly mention that this needs to fall back
> to origins for about:blank iframes/windows, and force extensions that really
> want to match unique security origins to do so by specifying the appropriate
> wildcard pattern.
> 
> Of these options, I strongly prefer #2 since security origins are well defined
> (and we already have the code to make it work in Blink) and we won't have a
> security model that's different from the rest of the web.

I prefer #1, because it avoids special casing like
https://codereview.chromium.org/226663003/patch/60001/60014#newcode251. After
https://codereview.chromium.org/226663003/ gets merged, I will get rid of
GetOriginURLForFrame and use frame->document()->inheritedURL() instead.

[robwu, 2014-05-01 17:52:25]

kalman:
I have changed origin to url-based checks. about:blank is now validated against
the URL patterns of a user script instead of the host permissions of the
extension.

PTAL, thanks!

[not at google - send to devlin, 2014-05-01 20:32:18]

fine apart from those nits in user_script_slave and that comment in
permissions_data.

dcheng@ should have a look at the changes in user_script_slave.

https://codereview.chromium.org/226663003/diff/90001/chrome/renderer/extensio...
File chrome/renderer/extensions/user_script_slave.cc (right):

https://codereview.chromium.org/226663003/diff/90001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_slave.cc:207: const WebSecurityOrigin
securityOrigin = originDocument.securityOrigin();
security_origin

https://codereview.chromium.org/226663003/diff/90001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_slave.cc:208: WebFrame* parent =
frame->parent() ? frame->parent() : frame->opener();
nit: this big block is hard to read, perhaps a blank line here will help

https://codereview.chromium.org/226663003/diff/90001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_slave.cc:211: while (parent != NULL) {
i think this loop, whole function really, could be simpler if this condition
were document_url.SchemeIs(content::kAboutScheme). you should only need to check
that once (not twice) and you should only really need to get the parent/opener
document once. i would find the logic here a bit easier to follow in that case.

https://codereview.chromium.org/226663003/diff/90001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_slave.cc:223: originDocument =
parentDocument;
you don't use this variable inside nor after the loop, so this assignment is
unnecessary.

so storing originDocument in a variable rather than just securityOrigin is
unnecessary.

https://codereview.chromium.org/226663003/diff/90001/chrome/renderer/extensio...
File chrome/renderer/extensions/user_script_slave.h (right):

https://codereview.chromium.org/226663003/diff/90001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_slave.h:46: 
delete 1 of these blank lines

https://codereview.chromium.org/226663003/diff/90001/extensions/common/permis...
File extensions/common/permissions/permissions_data.cc (right):

https://codereview.chromium.org/226663003/diff/90001/extensions/common/permis...
extensions/common/permissions/permissions_data.cc:515:
GetActivePermissions(extension)->HasEffectiveAccessToURL(document_url);
why do you need this change? there isn't a script, so we check against the
explicit host permissions. that is the right thing to do, not checking against
all permissions.

[robwu, 2014-05-01 21:30:37]

https://codereview.chromium.org/226663003/diff/90001/chrome/renderer/extensio...
File chrome/renderer/extensions/user_script_slave.cc (right):

https://codereview.chromium.org/226663003/diff/90001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_slave.cc:207: const WebSecurityOrigin
securityOrigin = originDocument.securityOrigin();
On 2014/05/01 20:32:19, kalman wrote:
> security_origin

Done. Should I also use underscores for |parentDocument| and
|parentDocumentUrl|?

https://codereview.chromium.org/226663003/diff/90001/chrome/renderer/extensio...
File chrome/renderer/extensions/user_script_slave.h (right):

https://codereview.chromium.org/226663003/diff/90001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_slave.h:46: 
On 2014/05/01 20:32:19, kalman wrote:
> delete 1 of these blank lines 

Done.

https://codereview.chromium.org/226663003/diff/90001/extensions/common/permis...
File extensions/common/permissions/permissions_data.cc (right):

https://codereview.chromium.org/226663003/diff/90001/extensions/common/permis...
extensions/common/permissions/permissions_data.cc:515:
GetActivePermissions(extension)->HasEffectiveAccessToURL(document_url);
On 2014/05/01 20:32:19, kalman wrote:
> why do you need this change? there isn't a script, so we check against the
> explicit host permissions. that is the right thing to do, not checking against
> all permissions.

The change is not needed any more, so I've reverted it.

My reasoning for not having reverted the change earlier is that if an extension
has declared a content script for a page, then it should also be able to
programatically insert a content script using chrome.tabs.executeScript.

[not at google - send to devlin, 2014-05-02 16:01:47]

sorry about the back and forth, I do want to get this submitted, it's a good
change to make.

https://codereview.chromium.org/226663003/diff/90001/chrome/renderer/extensio...
File chrome/renderer/extensions/user_script_slave.cc (right):

https://codereview.chromium.org/226663003/diff/90001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_slave.cc:207: const WebSecurityOrigin
securityOrigin = originDocument.securityOrigin();
On 2014/05/01 21:30:38, robwu wrote:
> On 2014/05/01 20:32:19, kalman wrote:
> > security_origin
> 
> Done. Should I also use underscores for |parentDocument| and
> |parentDocumentUrl|?

yep, thanks. always underscore style in Chromium (unless forced to camel case to
implement a Blink API).

https://codereview.chromium.org/226663003/diff/90001/extensions/common/permis...
File extensions/common/permissions/permissions_data.cc (right):

https://codereview.chromium.org/226663003/diff/90001/extensions/common/permis...
extensions/common/permissions/permissions_data.cc:515:
GetActivePermissions(extension)->HasEffectiveAccessToURL(document_url);
On 2014/05/01 21:30:38, robwu wrote:
> On 2014/05/01 20:32:19, kalman wrote:
> > why do you need this change? there isn't a script, so we check against the
> > explicit host permissions. that is the right thing to do, not checking
against
> > all permissions.
> 
> The change is not needed any more, so I've reverted it.
> 
> My reasoning for not having reverted the change earlier is that if an
extension
> has declared a content script for a page, then it should also be able to
> programatically insert a content script using chrome.tabs.executeScript.

Understood. We try to keep the permissions as narrow as possible.

https://codereview.chromium.org/226663003/diff/110001/chrome/renderer/extensi...
File chrome/renderer/extensions/user_script_slave.cc (right):

https://codereview.chromium.org/226663003/diff/110001/chrome/renderer/extensi...
chrome/renderer/extensions/user_script_slave.cc:212: 
not quite sure about this. if we're in an non-about-blank iframe on the same
domain then |parent| will be the non-NULL parent at this point.

e.g. "google.com/foo" has an iframe to "google.com/bar".

then the "if (parent)" check will pass, and the "securityOrigin.canAccess" check
will pass, so we'll end up returning "google.com/bar" here when really it should
be "google.com/foo".

this might matter in practice, if an extension requests a content script to be
executed on "google.com/foo/*" with allFrames=true, it will end up being
injected into the google.com/bar iframe too. unless there are checks elsewhere,
which is very possible. not a big deal for security reasons, but, it's still
wrong.

https://codereview.chromium.org/226663003/diff/110001/chrome/renderer/extensi...
chrome/renderer/extensions/user_script_slave.cc:215: const WebDocument&
parentDocument = parent->document();
as you pointed out, parent_document not parentDocument.

https://codereview.chromium.org/226663003/diff/110001/chrome/renderer/extensi...
chrome/renderer/extensions/user_script_slave.cc:218: return
GURL(parentDocument.url());
WebURL has a GURL operator so you don't need the explicit GURL(..) constructor:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[robwu, 2014-05-02 17:03:38]

On 2014/05/02 16:01:47, kalman wrote:
> sorry about the back and forth, I do want to get this submitted, it's a good
> change to make.

No problem, I appreciate the high quality assurance behind Chromium.

https://codereview.chromium.org/226663003/diff/110001/chrome/renderer/extensi...
File chrome/renderer/extensions/user_script_slave.cc (right):

https://codereview.chromium.org/226663003/diff/110001/chrome/renderer/extensi...
chrome/renderer/extensions/user_script_slave.cc:212: 
On 2014/05/02 16:01:48, kalman wrote:
> not quite sure about this. if we're in an non-about-blank iframe on the same
> domain then |parent| will be the non-NULL parent at this point.

If we're on a non-about:blank frame, then the code doesn't even reach this line.
This loop only runs for frames whose URL is about:blank. The caller of the
function explicitly passes the URL of the frame via |document_url|. If the
scheme is not "about:", then the function returns the same value again. I've
added a comment to emphasize this. 
> 
> e.g. "google.com/foo" has an iframe to "google.com/bar".
> 
> then the "if (parent)" check will pass, and the "securityOrigin.canAccess"
check
> will pass, so we'll end up returning "google.com/bar" here when really it
should
> be "google.com/foo".
> 
> this might matter in practice, if an extension requests a content script to be
> executed on "google.com/foo/*" with allFrames=true, it will end up being
> injected into the google.com/bar iframe too. unless there are checks
elsewhere,
> which is very possible. not a big deal for security reasons, but, it's still
> wrong.

https://codereview.chromium.org/226663003/diff/110001/chrome/renderer/extensi...
chrome/renderer/extensions/user_script_slave.cc:215: const WebDocument&
parentDocument = parent->document();
On 2014/05/02 16:01:48, kalman wrote:
> as you pointed out, parent_document not parentDocument.

Done.

https://codereview.chromium.org/226663003/diff/110001/chrome/renderer/extensi...
chrome/renderer/extensions/user_script_slave.cc:218: return
GURL(parentDocument.url());
On 2014/05/02 16:01:48, kalman wrote:
> WebURL has a GURL operator so you don't need the explicit GURL(..)
constructor:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Done.

[not at google - send to devlin, 2014-05-02 17:13:43]

lgtm. dcheng any more comments?

[dcheng, 2014-05-02 17:23:18]

https://codereview.chromium.org/226663003/diff/130001/chrome/renderer/extensi...
File chrome/renderer/extensions/user_script_slave.cc (right):

https://codereview.chromium.org/226663003/diff/130001/chrome/renderer/extensi...
chrome/renderer/extensions/user_script_slave.cc:220:
parent_document.securityOrigin()))
I believe this check should be inside the loop. Otherwise, you could traverse
through an iframe sandbox in the loop and miss that fact.

I would also like to see unit testing for this function--obviously it's already
partially covered by the new content script tests, but it'd be good to have a
more focused test on this portion as well.

[not at google - send to devlin, 2014-05-02 17:43:48]

https://codereview.chromium.org/226663003/diff/130001/chrome/renderer/extensi...
File chrome/renderer/extensions/user_script_slave.cc (right):

https://codereview.chromium.org/226663003/diff/130001/chrome/renderer/extensi...
chrome/renderer/extensions/user_script_slave.cc:220:
parent_document.securityOrigin()))
On 2014/05/02 17:23:19, dcheng wrote:
> I believe this check should be inside the loop. Otherwise, you could traverse
> through an iframe sandbox in the loop and miss that fact.

oh, good point.

> 
> I would also like to see unit testing for this function--obviously it's
already
> partially covered by the new content script tests, but it'd be good to have a
> more focused test on this portion as well.

ah unit testing on the renderer...

[dcheng, 2014-05-02 17:45:57]

On 2014/05/02 17:43:48, kalman wrote:
>
https://codereview.chromium.org/226663003/diff/130001/chrome/renderer/extensi...
> File chrome/renderer/extensions/user_script_slave.cc (right):
> 
>
https://codereview.chromium.org/226663003/diff/130001/chrome/renderer/extensi...
> chrome/renderer/extensions/user_script_slave.cc:220:
> parent_document.securityOrigin()))
> On 2014/05/02 17:23:19, dcheng wrote:
> > I believe this check should be inside the loop. Otherwise, you could
traverse
> > through an iframe sandbox in the loop and miss that fact.
> 
> oh, good point.
> 
> > 
> > I would also like to see unit testing for this function--obviously it's
> already
> > partially covered by the new content script tests, but it'd be good to have
a
> > more focused test on this portion as well.
> 
> ah unit testing on the renderer...

Yeah, this will be hard to do without the browser test harness unfortunately. So
it'll be a unit test living inside browser tests =(

[robwu, 2014-05-02 20:17:04]

https://codereview.chromium.org/226663003/diff/130001/chrome/renderer/extensi...
File chrome/renderer/extensions/user_script_slave.cc (right):

https://codereview.chromium.org/226663003/diff/130001/chrome/renderer/extensi...
chrome/renderer/extensions/user_script_slave.cc:220:
parent_document.securityOrigin()))
On 2014/05/02 17:23:19, dcheng wrote:
> I believe this check should be inside the loop. Otherwise, you could traverse
> through an iframe sandbox in the loop and miss that fact.

I've intentionally used do-while instead of while to allow script insertion in
sandboxed iframes. The purpose of this function is to find out whether to insert
a script in an about:-frame that's a child of some host. Obviously, script
insertion should be (and is) forbidden if the extension does not have any access
to the parent's effective origin.
When an about:-frame is sandboxed and the extension has permissions to access
the parent document, then there are no issues with allowing the extension to run
a content script in the sandbox. This is consistent with the existing
implementation (for non-about URLs), which also ignores the sandbox attribute of
a frame.

Without allowing sandboxed frames, adverts could trivially bypass AdBlock
extensions by setting the sandbox attribute, e.g. <iframe srcdoc="some advert
here" sandbox=""></iframe>.

> 
> I would also like to see unit testing for this function--obviously it's
already
> partially covered by the new content script tests, but it'd be good to have a
> more focused test on this portion as well.

Should I use IN_PROC_BROWSER_TEST_F in a file called
user_script_slave_browsertest.cc? If not, could you give an example of what you
want to see?

[not at google - send to devlin, 2014-05-02 20:20:09]

https://codereview.chromium.org/226663003/diff/130001/chrome/renderer/extensi...
File chrome/renderer/extensions/user_script_slave.cc (right):

https://codereview.chromium.org/226663003/diff/130001/chrome/renderer/extensi...
chrome/renderer/extensions/user_script_slave.cc:220:
parent_document.securityOrigin()))
On 2014/05/02 20:17:04, robwu wrote:
> On 2014/05/02 17:23:19, dcheng wrote:
> > I believe this check should be inside the loop. Otherwise, you could
traverse
> > through an iframe sandbox in the loop and miss that fact.
> 
> I've intentionally used do-while instead of while to allow script insertion in
> sandboxed iframes. The purpose of this function is to find out whether to
insert
> a script in an about:-frame that's a child of some host. Obviously, script
> insertion should be (and is) forbidden if the extension does not have any
access
> to the parent's effective origin.
> When an about:-frame is sandboxed and the extension has permissions to access
> the parent document, then there are no issues with allowing the extension to
run
> a content script in the sandbox. This is consistent with the existing
> implementation (for non-about URLs), which also ignores the sandbox attribute
of
> a frame.
> 
> Without allowing sandboxed frames, adverts could trivially bypass AdBlock
> extensions by setting the sandbox attribute, e.g. <iframe srcdoc="some advert
> here" sandbox=""></iframe>.

Such an ad network would be severely limiting its effectiveness though, since it
won't have access to its own cookies.

> 
> > 
> > I would also like to see unit testing for this function--obviously it's
> already
> > partially covered by the new content script tests, but it'd be good to have
a
> > more focused test on this portion as well.
> 
> Should I use IN_PROC_BROWSER_TEST_F in a file called
> user_script_slave_browsertest.cc? If not, could you give an example of what
you
> want to see?

[robwu, 2014-05-02 20:45:52]

https://codereview.chromium.org/226663003/diff/130001/chrome/renderer/extensi...
File chrome/renderer/extensions/user_script_slave.cc (right):

https://codereview.chromium.org/226663003/diff/130001/chrome/renderer/extensi...
chrome/renderer/extensions/user_script_slave.cc:220:
parent_document.securityOrigin()))
On 2014/05/02 20:20:10, kalman wrote:
> On 2014/05/02 20:17:04, robwu wrote:
> > On 2014/05/02 17:23:19, dcheng wrote:
> > > I believe this check should be inside the loop. Otherwise, you could
> traverse
> > > through an iframe sandbox in the loop and miss that fact.
> > 
> > I've intentionally used do-while instead of while to allow script insertion
in
> > sandboxed iframes. The purpose of this function is to find out whether to
> insert
> > a script in an about:-frame that's a child of some host. Obviously, script
> > insertion should be (and is) forbidden if the extension does not have any
> access
> > to the parent's effective origin.
> > When an about:-frame is sandboxed and the extension has permissions to
access
> > the parent document, then there are no issues with allowing the extension to
> run
> > a content script in the sandbox. This is consistent with the existing
> > implementation (for non-about URLs), which also ignores the sandbox
attribute
> of
> > a frame.
> > 
> > Without allowing sandboxed frames, adverts could trivially bypass AdBlock
> > extensions by setting the sandbox attribute, e.g. <iframe srcdoc="some
advert
> > here" sandbox=""></iframe>.
> 
> Such an ad network would be severely limiting its effectiveness though, since
it
> won't have access to its own cookies.

Actually, never mind this discussion. If an iframe is sandboxed, then its
securityOrigin is unique, meaning that canAccess() will always return false, see
http://src.chromium.org/viewvc/blink/trunk/Source/core/dom/Document.cpp?revis...
and
http://src.chromium.org/viewvc/blink/trunk/Source/platform/weborigin/Security...

A to-be-added unit test will verify this invariant. Could you comment on my
question about the unit test, below?

> 
> > 
> > > 
> > > I would also like to see unit testing for this function--obviously it's
> > already
> > > partially covered by the new content script tests, but it'd be good to
have
> a
> > > more focused test on this portion as well.
> > 
> > Should I use IN_PROC_BROWSER_TEST_F in a file called
> > user_script_slave_browsertest.cc? If not, could you give an example of what
> you
> > want to see?
>

[dcheng, 2014-05-06 21:30:27]

Sorry about the delayed reply. Comments inline.

On 2014/05/02 20:45:52, robwu wrote:
>
https://codereview.chromium.org/226663003/diff/130001/chrome/renderer/extensi...
> File chrome/renderer/extensions/user_script_slave.cc (right):
> 
>
https://codereview.chromium.org/226663003/diff/130001/chrome/renderer/extensi...
> chrome/renderer/extensions/user_script_slave.cc:220:
> parent_document.securityOrigin()))
> On 2014/05/02 20:20:10, kalman wrote:
> > On 2014/05/02 20:17:04, robwu wrote:
> > > On 2014/05/02 17:23:19, dcheng wrote:
> > > > I believe this check should be inside the loop. Otherwise, you could
> > traverse
> > > > through an iframe sandbox in the loop and miss that fact.
> > > 
> > > I've intentionally used do-while instead of while to allow script
insertion
> in
> > > sandboxed iframes. The purpose of this function is to find out whether to
> > insert
> > > a script in an about:-frame that's a child of some host. Obviously, script
> > > insertion should be (and is) forbidden if the extension does not have any
> > access
> > > to the parent's effective origin.
> > > When an about:-frame is sandboxed and the extension has permissions to
> access
> > > the parent document, then there are no issues with allowing the extension
to
> > run
> > > a content script in the sandbox. This is consistent with the existing
> > > implementation (for non-about URLs), which also ignores the sandbox
> attribute
> > of
> > > a frame.
> > > 
> > > Without allowing sandboxed frames, adverts could trivially bypass AdBlock
> > > extensions by setting the sandbox attribute, e.g. <iframe srcdoc="some
> advert
> > > here" sandbox=""></iframe>.
> > 
> > Such an ad network would be severely limiting its effectiveness though,
since
> it
> > won't have access to its own cookies.
> 
> Actually, never mind this discussion. If an iframe is sandboxed, then its
> securityOrigin is unique, meaning that canAccess() will always return false,
see
>
http://src.chromium.org/viewvc/blink/trunk/Source/core/dom/Document.cpp?revis...
> and
>
http://src.chromium.org/viewvc/blink/trunk/Source/platform/weborigin/Security...
> 

Yes, but skipping this check in the do-while loop misses this case:
Top-level frame (origin A)
 `- Child frame 1 (about:blank, unique origin because sandboxed)
     `- Child frame 2 (about:blank, origin A)

GetEffectiveDocumentURL() for child frame 2 will return the URL for the
top-level frame, even though its parent is sandboxed.

> A to-be-added unit test will verify this invariant. Could you comment on my
> question about the unit test, below?
> 
> > 
> > > 
> > > > 
> > > > I would also like to see unit testing for this function--obviously it's
> > > already
> > > > partially covered by the new content script tests, but it'd be good to
> have
> > a
> > > > more focused test on this portion as well.
> > > 
> > > Should I use IN_PROC_BROWSER_TEST_F in a file called
> > > user_script_slave_browsertest.cc? If not, could you give an example of
what
> > you
> > > want to see?
> >

This would probably work. Basically, I want to see us load a bunch of documents
with different frame structures and verify GetEffectiveDocumentURL returns the
expected value for various frames.

[robwu, 2014-05-07 21:52:54]

@dcheng

I have moved the securityOrigin check inside the loop and added a dedicated
browser_test for the GetEffectiveDocumentURL method. Could you take a look?

(I've also updated some tests because their expected behavior changed in an
earlier patch set)

https://codereview.chromium.org/226663003/diff/130001/chrome/renderer/extensi...
File chrome/renderer/extensions/user_script_slave.cc (right):

https://codereview.chromium.org/226663003/diff/130001/chrome/renderer/extensi...
chrome/renderer/extensions/user_script_slave.cc:220:
parent_document.securityOrigin()))
On 2014/05/02 17:23:19, dcheng wrote:
> I believe this check should be inside the loop. Otherwise, you could traverse
> through an iframe sandbox in the loop and miss that fact.
> 
> I would also like to see unit testing for this function--obviously it's
already
> partially covered by the new content script tests, but it'd be good to have a
> more focused test on this portion as well.

Done.

[dcheng, 2014-05-08 00:26:04]

https://codereview.chromium.org/226663003/diff/150001/chrome/renderer/extensi...
File chrome/renderer/extensions/user_script_slave.cc (right):

https://codereview.chromium.org/226663003/diff/150001/chrome/renderer/extensi...
chrome/renderer/extensions/user_script_slave.cc:216: if
(security_origin.canAccess(parent_document.securityOrigin())) {
Hm. Sorry, I think I confused myself when I was reviewing this CL last. I think
you're right that it's sufficient to just check canAccess() at the end--this
will already properly handle unique/sandbox origins, etc. Sorry for the
confusion.

https://codereview.chromium.org/226663003/diff/150001/chrome/renderer/extensi...
File chrome/renderer/extensions/user_script_slave_browsertest.cc (right):

https://codereview.chromium.org/226663003/diff/150001/chrome/renderer/extensi...
chrome/renderer/extensions/user_script_slave_browsertest.cc:66:
ProcessPendingMessages();
How come we call ProcessPendingMessages in a loop above, but not here? Also,
does this still work if you change useThreadedHTMLParserForDataURLs's initial
setting (in third_party/WebKit/Source/core/frame/Settings.in) to true?

[robwu, 2014-05-08 12:44:09]

https://codereview.chromium.org/226663003/diff/150001/chrome/renderer/extensi...
File chrome/renderer/extensions/user_script_slave.cc (right):

https://codereview.chromium.org/226663003/diff/150001/chrome/renderer/extensi...
chrome/renderer/extensions/user_script_slave.cc:216: if
(security_origin.canAccess(parent_document.securityOrigin())) {
On 2014/05/08 00:26:05, dcheng wrote:
> Hm. Sorry, I think I confused myself when I was reviewing this CL last. I
think
> you're right that it's sufficient to just check canAccess() at the end--this
> will already properly handle unique/sandbox origins, etc. Sorry for the
> confusion.

No need to apology, you were right. Consider the following scenario:

<iframe id=1 srcdoc="
  <iframe id=2 sandbox="" srcdoc="
     <iframe id=3 src="about:blank"></iframe>
  "></iframe>
</iframe>

When the method is called for frame 3, |document_url| is about:blank, so the
loop is entered.

In the old code, frame2 would be skipped because its URL is about:srcdoc.
In the new code with origin check, the loop is exited early because
iframe3.canAccess(iframe2) is false due to the sandbox attribute of frame 2.

https://codereview.chromium.org/226663003/diff/150001/chrome/renderer/extensi...
File chrome/renderer/extensions/user_script_slave_browsertest.cc (right):

https://codereview.chromium.org/226663003/diff/150001/chrome/renderer/extensi...
chrome/renderer/extensions/user_script_slave_browsertest.cc:66:
ProcessPendingMessages();
On 2014/05/08 00:26:05, dcheng wrote:
> How come we call ProcessPendingMessages in a loop above, but not here? Also,
> does this still work if you change useThreadedHTMLParserForDataURLs's initial
> setting (in third_party/WebKit/Source/core/frame/Settings.in) to true?

I honestly don't know why calling this method once is enough, but wrapping in a
loop does not do any harm, and could prevent test failures in case the messages
have to be pumped more often, so I have submitted a patch set that wraps this
line in a loop.

Changing |useThreadedHTMLParserForDataURLs| did not change the test behavior,
probably because the test does not use data-URIs.

[dcheng, 2014-05-09 01:18:49]

https://codereview.chromium.org/226663003/diff/150001/chrome/renderer/extensi...
File chrome/renderer/extensions/user_script_slave.cc (right):

https://codereview.chromium.org/226663003/diff/150001/chrome/renderer/extensi...
chrome/renderer/extensions/user_script_slave.cc:216: if
(security_origin.canAccess(parent_document.securityOrigin())) {
On 2014/05/08 12:44:10, robwu wrote:
> On 2014/05/08 00:26:05, dcheng wrote:
> > Hm. Sorry, I think I confused myself when I was reviewing this CL last. I
> think
> > you're right that it's sufficient to just check canAccess() at the end--this
> > will already properly handle unique/sandbox origins, etc. Sorry for the
> > confusion.
> 
> No need to apology, you were right. Consider the following scenario:
> 
> <iframe id=1 srcdoc="
>   <iframe id=2 sandbox="" srcdoc="
>      <iframe id=3 src="about:blank"></iframe>
>   "></iframe>
> </iframe>
> 
> When the method is called for frame 3, |document_url| is about:blank, so the
> loop is entered.
> 
> In the old code, frame2 would be skipped because its URL is about:srcdoc.
> In the new code with origin check, the loop is exited early because
> iframe3.canAccess(iframe2) is false due to the sandbox attribute of frame 2.

I don't think we actually exit early =)

But what I meant is that it's OK that we skip over frame 2 in this case. When we
exit the loop, we'll have parent == frame 1. But
frame3.securityOrigin().canAccess(frame1.securityOrigin()) will not be true.

[robwu, 2014-05-09 08:52:21]

https://codereview.chromium.org/226663003/diff/150001/chrome/renderer/extensi...
File chrome/renderer/extensions/user_script_slave.cc (right):

https://codereview.chromium.org/226663003/diff/150001/chrome/renderer/extensi...
chrome/renderer/extensions/user_script_slave.cc:216: if
(security_origin.canAccess(parent_document.securityOrigin())) {
On 2014/05/09 01:18:50, dcheng wrote:
> On 2014/05/08 12:44:10, robwu wrote:
> > On 2014/05/08 00:26:05, dcheng wrote:
> > > Hm. Sorry, I think I confused myself when I was reviewing this CL last. I
> > think
> > > you're right that it's sufficient to just check canAccess() at the
end--this
> > > will already properly handle unique/sandbox origins, etc. Sorry for the
> > > confusion.
> > 
> > No need to apology, you were right. Consider the following scenario:
> > 
> > <iframe id=1 srcdoc="
> >   <iframe id=2 sandbox="" srcdoc="
> >      <iframe id=3 src="about:blank"></iframe>
> >   "></iframe>
> > </iframe>
> > 
> > When the method is called for frame 3, |document_url| is about:blank, so the
> > loop is entered.
> > 
> > In the old code, frame2 would be skipped because its URL is about:srcdoc.
> > In the new code with origin check, the loop is exited early because
> > iframe3.canAccess(iframe2) is false due to the sandbox attribute of frame 2.
> 
> I don't think we actually exit early =)
> 
> But what I meant is that it's OK that we skip over frame 2 in this case. When
we
> exit the loop, we'll have parent == frame 1. But
> frame3.securityOrigin().canAccess(frame1.securityOrigin()) will not be true.

Ah, of course, because the sandbox is also inherit by the child frame. I've
reverted this change, the origin check after the loop is indeed sufficient.

[dcheng, 2014-05-13 19:49:06]

UserScriptSlave changes and associated tests LGTM.

Thanks for being patient through the reviews!

[robwu, 2014-05-13 19:50:37]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2014-05-13 19:50:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rob@robwu.nl/226663003/190001

[commit-bot: I haz the power, 2014-05-13 20:22:53]

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_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[commit-bot: I haz the power, 2014-05-13 20:27:33]

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

[commit-bot: I haz the power, 2014-05-13 20:27:34]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[robwu, 2014-05-14 12:16:15]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2014-05-14 12:16:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rob@robwu.nl/226663003/270001

[commit-bot: I haz the power, 2014-05-14 15:34:39]

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:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[commit-bot: I haz the power, 2014-05-14 15:47:25]

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

[commit-bot: I haz the power, 2014-05-14 15:47:26]

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

[robwu, 2014-05-14 17:10:05]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2014-05-14 17:10:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rob@robwu.nl/226663003/270001

[commit-bot: I haz the power, 2014-05-14 17:29:04]

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:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[commit-bot: I haz the power, 2014-05-14 17:45:13]

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

[commit-bot: I haz the power, 2014-05-14 17:45:14]

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

[robwu, 2014-05-15 09:37:12]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2014-05-15 09:38:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rob@robwu.nl/226663003/270001

[commit-bot: I haz the power, 2014-05-15 16:40:19]

Change committed as 270690