Block content scripts from executing until user grants permission

Block content scripts from executing until user grants permission

Prevent extensions with <all_urls> from running content scripts without user
consent if the scripts-require-action switch is on.

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

[Devlin, 2014-05-14 22:36:29]

This follows https://codereview.chromium.org/286003004/.  No rush.

[not at google - send to devlin, 2014-05-15 00:31:54]

That UserScriptSlave change is pretty epic. It would be great if you could do
the refactoring (pulling state into real data objects) in a pre-change so that I
don't need to pick apart what the differing behaviour should be.

Other note, and this could equally have gone on your other CL: this code needs
to integrate better with active tab at some point, for example,
- PermissionsData::RequiresActionForScriptExecution needs to check for a
specific host permission (i.e. rather than HasEffectiveAccessToAll() it should
be !HasSpecificAccess(host) && HasEffectiveAccessToAll()
- Code checking it needs to be aware of the tab, so that tab specific
permissions are taken into account.

I alluded to these in the previous patch regarding trying to execute scripts
after it's been allowed (it should work; and active tab should be the mechanism
to enable that).

https://codereview.chromium.org/288053002/diff/1/chrome/browser/extensions/ac...
File chrome/browser/extensions/active_script_controller_browsertest.cc (right):

https://codereview.chromium.org/288053002/diff/1/chrome/browser/extensions/ac...
chrome/browser/extensions/active_script_controller_browsertest.cc:153: // block
them.
comment needs updating?

https://codereview.chromium.org/288053002/diff/1/extensions/renderer/user_scr...
File extensions/renderer/user_script_slave.cc (right):

https://codereview.chromium.org/288053002/diff/1/extensions/renderer/user_scr...
extensions/renderer/user_script_slave.cc:7: #include <algorithm>  // std::max
grepping through Chromium, the comment is almost never included

https://codereview.chromium.org/288053002/diff/1/extensions/renderer/user_scr...
extensions/renderer/user_script_slave.cc:100: };
this factoring into objects makes sense. could you make it more OO though?
rather than having these functions that interpret a PendingInjection and a
ScriptsRunInfo, make these classes that know how to run themselves.

https://codereview.chromium.org/288053002/diff/1/extensions/renderer/user_scr...
extensions/renderer/user_script_slave.cc:257: UserScript* script = NULL;
why declared outside scope?

[Devlin, 2014-05-20 16:48:09]

The requested refactor was committed in
https://codereview.chromium.org/284153006/, so we can revisit this one now.

https://codereview.chromium.org/288053002/diff/1/chrome/browser/extensions/ac...
File chrome/browser/extensions/active_script_controller_browsertest.cc (right):

https://codereview.chromium.org/288053002/diff/1/chrome/browser/extensions/ac...
chrome/browser/extensions/active_script_controller_browsertest.cc:153: // block
them.
On 2014/05/15 00:31:54, kalman wrote:
> comment needs updating?

Whoops, thanks.

https://codereview.chromium.org/288053002/diff/1/extensions/renderer/user_scr...
File extensions/renderer/user_script_slave.cc (right):

https://codereview.chromium.org/288053002/diff/1/extensions/renderer/user_scr...
extensions/renderer/user_script_slave.cc:7: #include <algorithm>  // std::max
On 2014/05/15 00:31:54, kalman wrote:
> grepping through Chromium, the comment is almost never included

Looks like you're right.  I must have seen one of the rare occurrences, and
picked up on it. Shame, because I kinda like it for the STL util-style files.

https://codereview.chromium.org/288053002/diff/1/extensions/renderer/user_scr...
extensions/renderer/user_script_slave.cc:100: };
On 2014/05/15 00:31:54, kalman wrote:
> this factoring into objects makes sense. could you make it more OO though?
> rather than having these functions that interpret a PendingInjection and a
> ScriptsRunInfo, make these classes that know how to run themselves.

Semi-massive refactor done.  Woohoo!

https://codereview.chromium.org/288053002/diff/1/extensions/renderer/user_scr...
extensions/renderer/user_script_slave.cc:257: UserScript* script = NULL;
On 2014/05/15 00:31:54, kalman wrote:
> why declared outside scope?

Carryover from Uni days, when allocating 8 bytes of memory repeatedly actually
made a difference.

[not at google - send to devlin, 2014-05-21 15:01:06]

sweet.

I'm surprised the were so few changes to the test though...

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

https://codereview.chromium.org/288053002/diff/30001/chrome/browser/extension...
chrome/browser/extensions/user_script_master.cc:427:
changed_extensions_.insert(extension->id());
why only if the extension service is ready?

https://codereview.chromium.org/288053002/diff/30001/chrome/browser/extension...
chrome/browser/extensions/user_script_master.cc:477: std::set<std::string>() /*
include all extensions */);
mention this on the SendUpdate interface

https://codereview.chromium.org/288053002/diff/30001/chrome/browser/extension...
chrome/browser/extensions/user_script_master.cc:477: std::set<std::string>() /*
include all extensions */);
nit: use // style comments unless you really can't (like inside macros)

https://codereview.chromium.org/288053002/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/user_script_master.h (right):

https://codereview.chromium.org/288053002/diff/30001/chrome/browser/extension...
chrome/browser/extensions/user_script_master.h:152: // Sends the renderer
process a new set of user scripts.
you might want to add that |changed_extensions| is the set of extension IDs that
have changed since the last update, or ??? if it's the first update.

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/scri...
File extensions/renderer/script_injection.cc (right):

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/scri...
extensions/renderer/script_injection.cc:72: int64 id;
it seems like |page_id| is enough to distinguish requests? you're checking
script injection per extension, so if an extension is allowed then all its
script injections (and into the future, for that page) should be allowed too.

(note to future self/devlin: check for race conditions with active tab and/or
just generally optional permissions)

having this modelled as a request ID can't *hurt* I suppose but it is redundant
and complicates things a little. I think the only code that would need to change
is not terminating the loop in OnContentScriptGrantedPermission?

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/scri...
extensions/renderer/script_injection.cc:75: blink::WebString web_frame_name;
why not just hold onto the WebFrame pointer? code below doesn't imply it's
unsafe given you're observing NotifyFrameDetached.

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/scri...
extensions/renderer/script_injection.cc:131: DCHECK(extension);  // WantsToRun()
should be false if there's no extension.
CHECK

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/scri...
extensions/renderer/script_injection.cc:134:
content::RenderView::FromWebView(frame->top()->view());
please write a nice comment explaining why you're checking the top render view
here, not the render view for |frame|.

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/scri...
extensions/renderer/script_injection.cc:158: if (!render_view)
does this actually get called with a null render view..?

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/scri...
extensions/renderer/script_injection.cc:198: void
ScriptInjection::NotifyFrameDetached(blink::WebFrame* frame) {
ditto FrameDetached

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/user...
File extensions/renderer/user_script_slave.cc (right):

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/user...
extensions/renderer/user_script_slave.cc:137: // that we are updating.
could you mention "this is important to maintain pending script injection state
for each ScriptInjection"

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/user...
extensions/renderer/user_script_slave.cc:150: 
you might as well still call reserve()

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/user...
extensions/renderer/user_script_slave.cc:220: LogScriptsRun(frame,
UserScript::UNDEFINED, run_info);
could you define a new run location here like DEFERRED and handle that case
separately? e.g. we might as well count up and UMA the deferred css/js scripts
too.

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/user...
extensions/renderer/user_script_slave.cc:226: void
UserScriptSlave::NotifyFrameDetached(blink::WebFrame* frame) {
everywhere else in the codebase this is called "FrameDetached" so please call it
that.

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/user...
extensions/renderer/user_script_slave.cc:250: if (location ==
UserScript::DOCUMENT_START) {
bleh, can we change this to a switch() along the way?

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/user...
extensions/renderer/user_script_slave.cc:263:
UMA_HISTOGRAM_TIMES("Extensions.InjectIdle_Time", info.timer.Elapsed());
see comment about deferred

[Devlin, 2014-05-21 17:05:11]

On 2014/05/21 15:01:06, kalman wrote:
> sweet.
> 
> I'm surprised the were so few changes to the test though...

Everything was already in place in the test, only change is to make sure it
doesn't actually execute until we grant permission.  Makes it easy :)

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

https://codereview.chromium.org/288053002/diff/30001/chrome/browser/extension...
chrome/browser/extensions/user_script_master.cc:427:
changed_extensions_.insert(extension->id());
On 2014/05/21 15:01:07, kalman wrote:
> why only if the extension service is ready?

If the extension service isn't ready, then we haven't sent over the initial
batch of (all) extension user scripts, and this will be included in that set.

https://codereview.chromium.org/288053002/diff/30001/chrome/browser/extension...
chrome/browser/extensions/user_script_master.cc:477: std::set<std::string>() /*
include all extensions */);
On 2014/05/21 15:01:07, kalman wrote:
> nit: use // style comments unless you really can't (like inside macros)

Done.

https://codereview.chromium.org/288053002/diff/30001/chrome/browser/extension...
chrome/browser/extensions/user_script_master.cc:477: std::set<std::string>() /*
include all extensions */);
On 2014/05/21 15:01:07, kalman wrote:
> mention this on the SendUpdate interface

Done.

https://codereview.chromium.org/288053002/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/user_script_master.h (right):

https://codereview.chromium.org/288053002/diff/30001/chrome/browser/extension...
chrome/browser/extensions/user_script_master.h:152: // Sends the renderer
process a new set of user scripts.
On 2014/05/21 15:01:07, kalman wrote:
> you might want to add that |changed_extensions| is the set of extension IDs
that
> have changed since the last update, or ??? if it's the first update.

Done.

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/scri...
File extensions/renderer/script_injection.cc (right):

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/scri...
extensions/renderer/script_injection.cc:72: int64 id;
On 2014/05/21 15:01:07, kalman wrote:
> it seems like |page_id| is enough to distinguish requests? you're checking
> script injection per extension, so if an extension is allowed then all its
> script injections (and into the future, for that page) should be allowed too.
> 
> (note to future self/devlin: check for race conditions with active tab and/or
> just generally optional permissions)
> 
> having this modelled as a request ID can't *hurt* I suppose but it is
redundant
> and complicates things a little. I think the only code that would need to
change
> is not terminating the loop in OnContentScriptGrantedPermission?

We'd actually need page id + extension id, because just sending back a page id
doesn't let us know which extension is approved.  Just by that, the int64 is
smaller and faster to send across (and I think int64 is preferred to even an
extension id string for IPC, but the security folks would know much better).

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/scri...
extensions/renderer/script_injection.cc:75: blink::WebString web_frame_name;
On 2014/05/21 15:01:07, kalman wrote:
> why not just hold onto the WebFrame pointer? code below doesn't imply it's
> unsafe given you're observing NotifyFrameDetached.

Good point.  Let's try that.

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/scri...
extensions/renderer/script_injection.cc:131: DCHECK(extension);  // WantsToRun()
should be false if there's no extension.
On 2014/05/21 15:01:07, kalman wrote:
> CHECK

If you insist...

My thinking is generally that if you can *know* (logically) that something is a
certain way, there's no point in CHECKing (and slowing down a production build
by x fractions of a microsecond).  Instead, DCHECK, and if there's something
wrong, we'll learn about it that way.  But, anyway.  Just my $0.02.  Done.

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/scri...
extensions/renderer/script_injection.cc:134:
content::RenderView::FromWebView(frame->top()->view());
On 2014/05/21 15:01:07, kalman wrote:
> please write a nice comment explaining why you're checking the top render view
> here, not the render view for |frame|.

Done.

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/scri...
extensions/renderer/script_injection.cc:158: if (!render_view)
On 2014/05/21 15:01:07, kalman wrote:
> does this actually get called with a null render view..?

I'd certainly hope not, but I hate to make crashing assumptions. ;)  Want me to
change it to a CHECK(), omit it, or leave it as-is?

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/scri...
extensions/renderer/script_injection.cc:198: void
ScriptInjection::NotifyFrameDetached(blink::WebFrame* frame) {
On 2014/05/21 15:01:07, kalman wrote:
> ditto FrameDetached

Done.

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/user...
File extensions/renderer/user_script_slave.cc (right):

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/user...
extensions/renderer/user_script_slave.cc:137: // that we are updating.
On 2014/05/21 15:01:07, kalman wrote:
> could you mention "this is important to maintain pending script injection
state
> for each ScriptInjection"

Good call. Done.

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/user...
extensions/renderer/user_script_slave.cc:150: 
On 2014/05/21 15:01:07, kalman wrote:
> you might as well still call reserve()

Done.

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/user...
extensions/renderer/user_script_slave.cc:220: LogScriptsRun(frame,
UserScript::UNDEFINED, run_info);
On 2014/05/21 15:01:07, kalman wrote:
> could you define a new run location here like DEFERRED and handle that case
> separately? e.g. we might as well count up and UMA the deferred css/js scripts
> too.

New location added.  It okay to add histograms in a follow-up to make it clear
to the histograms reviewer what's changing?  (Added a TODO in the meantime.)

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/user...
extensions/renderer/user_script_slave.cc:226: void
UserScriptSlave::NotifyFrameDetached(blink::WebFrame* frame) {
On 2014/05/21 15:01:07, kalman wrote:
> everywhere else in the codebase this is called "FrameDetached" so please call
it
> that.

Logic was to make it clear that this wasn't OVERRIDEing the observer method, but
I guess since it's just a direct forward of it, we should name it the same for
consistency. Done.

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/user...
extensions/renderer/user_script_slave.cc:250: if (location ==
UserScript::DOCUMENT_START) {
On 2014/05/21 15:01:07, kalman wrote:
> bleh, can we change this to a switch() along the way?

Done.

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/user...
extensions/renderer/user_script_slave.cc:263:
UMA_HISTOGRAM_TIMES("Extensions.InjectIdle_Time", info.timer.Elapsed());
On 2014/05/21 15:01:07, kalman wrote:
> see comment about deferred

Done.

[not at google - send to devlin, 2014-05-21 17:36:22]

lgtm

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/scri...
File extensions/renderer/script_injection.cc (right):

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/scri...
extensions/renderer/script_injection.cc:72: int64 id;
On 2014/05/21 17:05:11, Devlin wrote:
> On 2014/05/21 15:01:07, kalman wrote:
> > it seems like |page_id| is enough to distinguish requests? you're checking
> > script injection per extension, so if an extension is allowed then all its
> > script injections (and into the future, for that page) should be allowed
too.
> > 
> > (note to future self/devlin: check for race conditions with active tab
and/or
> > just generally optional permissions)
> > 
> > having this modelled as a request ID can't *hurt* I suppose but it is
> redundant
> > and complicates things a little. I think the only code that would need to
> change
> > is not terminating the loop in OnContentScriptGrantedPermission?
> 
> We'd actually need page id + extension id, because just sending back a page id
> doesn't let us know which extension is approved.  Just by that, the int64 is
> smaller and faster to send across (and I think int64 is preferred to even an
> extension id string for IPC, but the security folks would know much better).

good point.

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/scri...
extensions/renderer/script_injection.cc:131: DCHECK(extension);  // WantsToRun()
should be false if there's no extension.
On 2014/05/21 17:05:11, Devlin wrote:
> On 2014/05/21 15:01:07, kalman wrote:
> > CHECK
> 
> If you insist...
> 
> My thinking is generally that if you can *know* (logically) that something is
a
> certain way, there's no point in CHECKing (and slowing down a production build
> by x fractions of a microsecond).  Instead, DCHECK, and if there's something
> wrong, we'll learn about it that way.  But, anyway.  Just my $0.02.  Done.

I trust many things, but not the existence of extensions.

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/scri...
extensions/renderer/script_injection.cc:158: if (!render_view)
On 2014/05/21 17:05:11, Devlin wrote:
> On 2014/05/21 15:01:07, kalman wrote:
> > does this actually get called with a null render view..?
> 
> I'd certainly hope not, but I hate to make crashing assumptions. ;)  Want me
to
> change it to a CHECK(), omit it, or leave it as-is?

I'd take it out entirely. it's seemingly arbitrary to check the render view and
not, say, |scripts_run_info|. if there's a good reasons to check it (i.e. my
question) then do so but doesn't seem that way.

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/user...
File extensions/renderer/user_script_slave.cc (right):

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/user...
extensions/renderer/user_script_slave.cc:220: LogScriptsRun(frame,
UserScript::UNDEFINED, run_info);
On 2014/05/21 17:05:11, Devlin wrote:
> On 2014/05/21 15:01:07, kalman wrote:
> > could you define a new run location here like DEFERRED and handle that case
> > separately? e.g. we might as well count up and UMA the deferred css/js
scripts
> > too.
> 
> New location added.  It okay to add histograms in a follow-up to make it clear
> to the histograms reviewer what's changing?  (Added a TODO in the meantime.)

yes!

https://codereview.chromium.org/288053002/diff/50001/extensions/renderer/user...
File extensions/renderer/user_script_slave.cc (right):

https://codereview.chromium.org/288053002/diff/50001/extensions/renderer/user...
extensions/renderer/user_script_slave.cc:276: case UserScript::UNDEFINED:
does anybody actually used UNDEFINED? can we kill it?

[Devlin, 2014-05-21 18:28:27]

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/scri...
File extensions/renderer/script_injection.cc (right):

https://codereview.chromium.org/288053002/diff/30001/extensions/renderer/scri...
extensions/renderer/script_injection.cc:158: if (!render_view)
On 2014/05/21 17:36:23, kalman wrote:
> On 2014/05/21 17:05:11, Devlin wrote:
> > On 2014/05/21 15:01:07, kalman wrote:
> > > does this actually get called with a null render view..?
> > 
> > I'd certainly hope not, but I hate to make crashing assumptions. ;)  Want me
> to
> > change it to a CHECK(), omit it, or leave it as-is?
> 
> I'd take it out entirely. it's seemingly arbitrary to check the render view
and
> not, say, |scripts_run_info|. if there's a good reasons to check it (i.e. my
> question) then do so but doesn't seem that way.

Done.

https://codereview.chromium.org/288053002/diff/50001/extensions/renderer/user...
File extensions/renderer/user_script_slave.cc (right):

https://codereview.chromium.org/288053002/diff/50001/extensions/renderer/user...
extensions/renderer/user_script_slave.cc:276: case UserScript::UNDEFINED:
On 2014/05/21 17:36:23, kalman wrote:
> does anybody actually used UNDEFINED? can we kill it?

UserScriptScheduler uses it as a "before anything else" indication, so no-go on
the killing :(

[Devlin, 2014-05-21 18:28:46]

Justin, can you take a look at extension_messages.h?

[Devlin, 2014-05-23 16:13:29]

On 2014/05/21 18:28:46, Devlin wrote:
> Justin, can you take a look at extension_messages.h?

Justin, friendly ping.

[not at google - send to devlin, 2014-05-23 18:05:43]

https://codereview.chromium.org/288053002/diff/90001/extensions/renderer/scri...
File extensions/renderer/script_injection.cc (right):

https://codereview.chromium.org/288053002/diff/90001/extensions/renderer/scri...
extensions/renderer/script_injection.cc:140: if
(PermissionsData::RequiresActionForScriptExecution(extension) &&
as noted over chat, we also need a check that the flag is actually enabled lest
we break document_start/end scripts.

note to readers: we know we're going to break it when the flag is turned on. we
just don't want to break it just yet.

[not at google - send to devlin, 2014-05-23 18:06:55]

https://codereview.chromium.org/288053002/diff/90001/extensions/renderer/scri...
File extensions/renderer/script_injection.cc (right):

https://codereview.chromium.org/288053002/diff/90001/extensions/renderer/scri...
extensions/renderer/script_injection.cc:140: if
(PermissionsData::RequiresActionForScriptExecution(extension) &&
On 2014/05/23 18:05:43, kalman wrote:
> as noted over chat, we also need a check that the flag is actually enabled
lest
> we break document_start/end scripts.
> 
> note to readers: we know we're going to break it when the flag is turned on.
we
> just don't want to break it just yet.

and it's very worrying that this didn't break any tests.

[Devlin, 2014-05-23 19:19:46]

https://codereview.chromium.org/288053002/diff/90001/extensions/renderer/scri...
File extensions/renderer/script_injection.cc (right):

https://codereview.chromium.org/288053002/diff/90001/extensions/renderer/scri...
extensions/renderer/script_injection.cc:140: if
(PermissionsData::RequiresActionForScriptExecution(extension) &&
On 2014/05/23 18:05:43, kalman wrote:
> as noted over chat, we also need a check that the flag is actually enabled
lest
> we break document_start/end scripts.
> 
> note to readers: we know we're going to break it when the flag is turned on.
we
> just don't want to break it just yet.

Done.

[not at google - send to devlin, 2014-05-23 21:22:26]

lgtm though pls add a test for the flag being off.

https://codereview.chromium.org/288053002/diff/160001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/288053002/diff/160001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1641:
extensions::switches::kEnableScriptsRequireAction,
kEn < kEx

[Devlin, 2014-05-23 21:49:51]

And test added

https://codereview.chromium.org/288053002/diff/160001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/288053002/diff/160001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1641:
extensions::switches::kEnableScriptsRequireAction,
On 2014/05/23 21:22:26, kalman wrote:
> kEn < kEx

Done.

[Devlin, 2014-05-27 20:35:38]

Justin, friendly ping?

[jschuh, 2014-05-28 00:51:33]

On 2014/05/27 20:35:38, Devlin wrote:
> Justin, friendly ping?

Sorry, I was OOO today and most of last week. I'll take a look tomorrow.

[jschuh, 2014-05-28 22:26:09]

IPC security lgtm. (Notes: Adding opaque strings as extension IDs and an integer
request ID. Weak pointer in web_frame but lifetime management looks good.)

[Devlin, 2014-05-29 22:00:57]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-05-29 22:01:59]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/288053002/...

[commit-bot: I haz the power, 2014-05-30 02:24:21]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-30 04:58:58]

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

[commit-bot: I haz the power, 2014-05-30 04:58:58]

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

[Devlin, 2014-05-30 15:26:32]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-05-30 15:28:08]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/288053002/...

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

Change committed as 273866