We have a problem in the process on destroying WebContentsImpl because

We have a problem in the process on destroying WebContentsImpl because
each WebContentsObserver first NULLs its web_contents_ pointer and
performs cleanup code after that.

This could work if we guaranteed that every WebContentsObserver doesn't
directly or indirectly affect any other WebContentsObserver. But
actually WebContentsObservers can do anything and also affect each
other. And if one WebContentsObserver already NULLed its web_contents_
pointer (e.g. ZoomController) other WebContentsObserver
(e.g. InterstitialPageImpl) calls it (indirectly in this case) we have
problems and result depends on ordering of WebContentsObservers in the
list of observers.

The solution splits that process into two phases. First we let each
WebContentsObserver to perform their cleanup code and at that point they
can freely call each other, we completely don't care about the ordering.
We can call this phase as untrusted phase because we deal with some cleanup
code that can do anything. After that we make second pass and NULL each
web_contents_ pointer and this phase is trusted because we call only private
implementation of WebContentsObserver that we definitely can trust.

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

[zverre, 2014-04-29 09:44:55]

Hi could you please take a look?

[Avi (use Gerrit), 2014-04-29 14:14:30]

This is a fundamental content API change, sending to John.

[zverre, 2014-04-29 14:30:55]

On 2014/04/29 14:14:30, Avi wrote:
> This is a fundamental content API change, sending to John.

Thank you. Could you please run some try jobs to ensure nothing is broken.
Especially Linux unit_tests and browser_tests.

[jam, 2014-04-29 18:26:28]

On 2014/04/29 14:30:55, zverre wrote:
> On 2014/04/29 14:14:30, Avi wrote:
> > This is a fundamental content API change, sending to John.
> 
> Thank you. Could you please run some try jobs to ensure nothing is broken.
> Especially Linux unit_tests and browser_tests.

Seems reasonable, few comments:

1) if we do this, can we revert https://codereview.chromium.org/239393009?
2) we can remove WCO::WebContentsImplDestroyed() and just have WCImpl set
web_contents_ pointer to NULL
3) WebContentsDestroyed should now not take a WC pointer anymore, since
web_contents() getter will work. so please update all the WCO child classes

[zverre, 2014-04-30 09:12:04]

On 2014/04/29 18:26:28, jam wrote:
> On 2014/04/29 14:30:55, zverre wrote:
> > On 2014/04/29 14:14:30, Avi wrote:
> > > This is a fundamental content API change, sending to John.
> > 
> > Thank you. Could you please run some try jobs to ensure nothing is broken.
> > Especially Linux unit_tests and browser_tests.
> 
> Seems reasonable, few comments:
> 
> 1) if we do this, can we revert https://codereview.chromium.org/239393009
> 2) we can remove WCO::WebContentsImplDestroyed() and just have WCImpl set
> web_contents_ pointer to NULL
> 3) WebContentsDestroyed should now not take a WC pointer anymore, since
> web_contents() getter will work. so please update all the WCO child classes

Thank you Jam!
Answers to your questions are:
1. Yes of course. We should revert it.
2. Did you mean manual "for loop" through all WCOs? Could you please explain me
a little bit more what's the reason for that? As for me since it is private
function in the WCO and nobody can affect it it's absolutely OK to call
FOR_EACH_OBSERVER and do it in that fashion. The only thing I can offer is to
rename it to may be WebContentsDestroyedPrivate to underline its nature.
3. OK. No problems. I'll do that.

[Avi (use Gerrit), 2014-04-30 14:44:38]

On 2014/04/29 18:26:28, jam wrote:
> Seems reasonable

I'm looking at the history of this.

Scott's change r77146 (http://crrev.com/3173036) brought in the predecessors to
WebContentsImplDestroyed/WebContentsDestroyed, but even before that the
web_contents_ variable was nulled to indicate that the WebContents was going
away ever since that variable was introduced in r75226
(http://crrev.com/6537004) by you three years ago.

My concern isn't that this can't work, but that this has had three years for
every WCO to rely on today's semantics, what it means for web_contents() to be
null. If web_contents() stops being null and control flows through where it
didn't before, behavior will change.

If this is a desirable refactor, let's do it. But I _do_ see this as a
_project_, not a quick change.

[jam, 2014-04-30 16:22:08]

On 2014/04/30 09:12:04, zverre wrote:
> On 2014/04/29 18:26:28, jam wrote:
> > On 2014/04/29 14:30:55, zverre wrote:
> > > On 2014/04/29 14:14:30, Avi wrote:
> > > > This is a fundamental content API change, sending to John.
> > > 
> > > Thank you. Could you please run some try jobs to ensure nothing is broken.
> > > Especially Linux unit_tests and browser_tests.
> > 
> > Seems reasonable, few comments:
> > 
> > 1) if we do this, can we revert https://codereview.chromium.org/239393009
> > 2) we can remove WCO::WebContentsImplDestroyed() and just have WCImpl set
> > web_contents_ pointer to NULL
> > 3) WebContentsDestroyed should now not take a WC pointer anymore, since
> > web_contents() getter will work. so please update all the WCO child classes
> 
> Thank you Jam!
> Answers to your questions are:
> 1. Yes of course. We should revert it.
> 2. Did you mean manual "for loop" through all WCOs? Could you please explain
me
> a little bit more what's the reason for that? As for me since it is private
> function in the WCO and nobody can affect it it's absolutely OK to call
> FOR_EACH_OBSERVER and do it in that fashion. The only thing I can offer is to
> rename it to may be WebContentsDestroyedPrivate to underline its nature.

It's already a private method. I just thought that since WCImpl is friends with
WCO, there's no need to have a private method which nulls an internal variable.
> 3. OK. No problems. I'll do that.

[jam, 2014-04-30 16:22:09]

On 2014/04/30 09:12:04, zverre wrote:
> On 2014/04/29 18:26:28, jam wrote:
> > On 2014/04/29 14:30:55, zverre wrote:
> > > On 2014/04/29 14:14:30, Avi wrote:
> > > > This is a fundamental content API change, sending to John.
> > > 
> > > Thank you. Could you please run some try jobs to ensure nothing is broken.
> > > Especially Linux unit_tests and browser_tests.
> > 
> > Seems reasonable, few comments:
> > 
> > 1) if we do this, can we revert https://codereview.chromium.org/239393009
> > 2) we can remove WCO::WebContentsImplDestroyed() and just have WCImpl set
> > web_contents_ pointer to NULL
> > 3) WebContentsDestroyed should now not take a WC pointer anymore, since
> > web_contents() getter will work. so please update all the WCO child classes
> 
> Thank you Jam!
> Answers to your questions are:
> 1. Yes of course. We should revert it.
> 2. Did you mean manual "for loop" through all WCOs? Could you please explain
me
> a little bit more what's the reason for that? As for me since it is private
> function in the WCO and nobody can affect it it's absolutely OK to call
> FOR_EACH_OBSERVER and do it in that fashion. The only thing I can offer is to
> rename it to may be WebContentsDestroyedPrivate to underline its nature.

It's already a private method. I just thought that since WCImpl is friends with
WCO, there's no need to have a private method which nulls an internal variable.
> 3. OK. No problems. I'll do that.

[jam, 2014-04-30 16:22:34]

On 2014/04/30 14:44:38, Avi wrote:
> On 2014/04/29 18:26:28, jam wrote:
> > Seems reasonable
> 
> I'm looking at the history of this.
> 
> Scott's change r77146 (http://crrev.com/3173036) brought in the predecessors
to
> WebContentsImplDestroyed/WebContentsDestroyed, but even before that the
> web_contents_ variable was nulled to indicate that the WebContents was going
> away ever since that variable was introduced in r75226
> (http://crrev.com/6537004) by you three years ago.
> 
> My concern isn't that this can't work, but that this has had three years for
> every WCO to rely on today's semantics, what it means for web_contents() to be
> null. If web_contents() stops being null and control flows through where it
> didn't before, behavior will change.

Avi: I'm not sure I understand your concern? web_contents_ would still be
NULL'd?
> 
> If this is a desirable refactor, let's do it. But I _do_ see this as a
> _project_, not a quick change.

[Avi (use Gerrit), 2014-04-30 16:40:54]

On 2014/04/30 16:22:34, jam wrote:
> Avi: I'm not sure I understand your concern? web_contents_ would still be
> NULL'd?

Yes, it would be NULLed, but _after_ everything is over. Maybe I'm a worrier.

[jam, 2014-04-30 22:14:58]

On 2014/04/30 16:40:54, Avi wrote:
> On 2014/04/30 16:22:34, jam wrote:
> > Avi: I'm not sure I understand your concern? web_contents_ would still be
> > NULL'd?
> 
> Yes, it would be NULLed, but _after_ everything is over. Maybe I'm a worrier.

I'm not sure what you mean by "everything"? Before, it was nulled before calling
WebContentsDestroyed before because otherwise if the object deleted itself in
that method, then we would be nulling the pointer of a deleted object. this
problem doesn't happen anymore with the proposed patch, because the nulling of
the pointer would happen in a separate loop over the observers, which would only
go over live objects.

[zverre, 2014-05-01 21:03:42]

On 2014/04/30 22:14:58, jam wrote:
> On 2014/04/30 16:40:54, Avi wrote:
> > On 2014/04/30 16:22:34, jam wrote:
> > > Avi: I'm not sure I understand your concern? web_contents_ would still be
> > > NULL'd?
> > 
> > Yes, it would be NULLed, but _after_ everything is over. Maybe I'm a
worrier.
> 
> I'm not sure what you mean by "everything"? Before, it was nulled before
calling
> WebContentsDestroyed before because otherwise if the object deleted itself in
> that method, then we would be nulling the pointer of a deleted object. this
> problem doesn't happen anymore with the proposed patch, because the nulling of
> the pointer would happen in a separate loop over the observers, which would
only
> go over live objects.

PTAL
And again could you please run some try jobs?

[zverre, 2014-05-02 15:17:05]

Hi guys! Could please take a look? And also I wanted to ask if we should revert
changes according to https://codereview.chromium.org/239393009?

[Avi (use Gerrit), 2014-05-02 15:32:24]

I pointed out a sample issue I caught while browsing through the change. I did
not fully review this change.

Yes, I'm sure you can make this work. However, you need to go through _each_use_
and not just make the syntactic changes, but double-check the semantics of _how_
that class uses WebContentsDestroyed.

I would urge you to be a little more paranoid.

Have you tested a local build to make sure that it works? Run tests locally?

https://codereview.chromium.org/257153003/diff/40001/chrome/browser/ui/panels...
File chrome/browser/ui/panels/panel_host.cc (right):

https://codereview.chromium.org/257153003/diff/40001/chrome/browser/ui/panels...
chrome/browser/ui/panels/panel_host.cc:213: CHECK(!web_contents_.get());
This line will explode, because you changed the semantics of
WebContentsDestroyed.

[zverre, 2014-05-02 19:10:23]

https://codereview.chromium.org/257153003/diff/40001/chrome/browser/ui/panels...
File chrome/browser/ui/panels/panel_host.cc (right):

https://codereview.chromium.org/257153003/diff/40001/chrome/browser/ui/panels...
chrome/browser/ui/panels/panel_host.cc:213: CHECK(!web_contents_.get());
On 2014/05/02 15:32:25, Avi wrote:
> This line will explode, because you changed the semantics of
> WebContentsDestroyed.

I think that everything is OK here because this is scoped_ptr member of
PanelHost, not that of WebContentsObserver and it definitely should be NULL at
that point.

[Avi (use Gerrit), 2014-05-02 19:17:46]

On 2014/05/02 19:10:23, zverre wrote:
>
https://codereview.chromium.org/257153003/diff/40001/chrome/browser/ui/panels...
> File chrome/browser/ui/panels/panel_host.cc (right):
> 
>
https://codereview.chromium.org/257153003/diff/40001/chrome/browser/ui/panels...
> chrome/browser/ui/panels/panel_host.cc:213: CHECK(!web_contents_.get());
> On 2014/05/02 15:32:25, Avi wrote:
> > This line will explode, because you changed the semantics of
> > WebContentsDestroyed.
> 
> I think that everything is OK here because this is scoped_ptr member of
> PanelHost, not that of WebContentsObserver and it definitely should be NULL at
> that point.

OK, that's fair.

Next step is to get green trybots.

[jam, 2014-05-02 19:34:10]

lgtm with comments, thanks

https://codereview.chromium.org/257153003/diff/40001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_popup_controller_interactive_uitest.cc
(right):

https://codereview.chromium.org/257153003/diff/40001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_popup_controller_interactive_uitest.cc:98:
content::WebContents* web_contents_;
remove this as well

https://codereview.chromium.org/257153003/diff/40001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right):

https://codereview.chromium.org/257153003/diff/40001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:134: web_contents_
= NULL;
please remove web_contents_ from this class and switch it to use web_contents()
instead?

https://codereview.chromium.org/257153003/diff/40001/content/public/browser/w...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/257153003/diff/40001/content/public/browser/w...
content/public/browser/web_contents_observer.h:296: // process is split in two
phases (untrusted and trusted).
this trusted and untrusted terminology is a bit confusing and not standard, so
i'd leave this out.

i would stay with something like 

// Invoked when the WebContents is being destroyed. Gives subclasses a chance
// to cleanup. After this method returns, web_contents() returns NULL.

https://codereview.chromium.org/257153003/diff/40001/content/public/browser/w...
content/public/browser/web_contents_observer.h:372: void
WebContentsDestroyedPrivate() { web_contents_ = NULL; }
nit: since it's inline and it's a simple method, it should just be
reset_web_contents() { web_contents_ = NULL; }

or really since it's private and the calling class is friends, i don't see a
need to have a method at all

[zverre, 2014-05-05 12:40:43]

https://codereview.chromium.org/257153003/diff/40001/content/public/browser/w...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/257153003/diff/40001/content/public/browser/w...
content/public/browser/web_contents_observer.h:372: void
WebContentsDestroyedPrivate() { web_contents_ = NULL; }
On 2014/05/02 19:34:11, jam wrote:
> nit: since it's inline and it's a simple method, it should just be
> reset_web_contents() { web_contents_ = NULL; }
> 
> or really since it's private and the calling class is friends, i don't see a
> need to have a method at all

Jam, thank you for your comments. I don't really want to write loop manually
because actual list of observers is private and only ObserverListBase::Iterator
can get actual observers' pointers. Also this approach seems not standard and
looks a bit weird in Chromium code. It's much more easy to find observers'
invocation in the code if it is always done with FOR_EACH_OBSERVER. So I'd
better stay with reset_web_contents as a better alternative to
WebContentsDestroyedPrivate.

[jam, 2014-05-05 15:05:02]

https://codereview.chromium.org/257153003/diff/40001/content/public/browser/w...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/257153003/diff/40001/content/public/browser/w...
content/public/browser/web_contents_observer.h:372: void
WebContentsDestroyedPrivate() { web_contents_ = NULL; }
On 2014/05/05 12:40:43, zverre wrote:
> On 2014/05/02 19:34:11, jam wrote:
> > nit: since it's inline and it's a simple method, it should just be
> > reset_web_contents() { web_contents_ = NULL; }
> > 
> > or really since it's private and the calling class is friends, i don't see a
> > need to have a method at all
> 
> Jam, thank you for your comments. I don't really want to write loop manually
> because actual list of observers is private and only
ObserverListBase::Iterator
> can get actual observers' pointers. Also this approach seems not standard and
> looks a bit weird in Chromium code. It's much more easy to find observers'
> invocation in the code if it is always done with FOR_EACH_OBSERVER. So I'd
> better stay with reset_web_contents as a better alternative to
> WebContentsDestroyedPrivate.

ok sure. i don't feel strongly about it (btw for the record, we have some places
that iterate manually over the observer list)

[zverre, 2014-05-05 16:40:24]

https://codereview.chromium.org/257153003/diff/40001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_popup_controller_interactive_uitest.cc
(right):

https://codereview.chromium.org/257153003/diff/40001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_popup_controller_interactive_uitest.cc:98:
content::WebContents* web_contents_;
On 2014/05/02 19:34:11, jam wrote:
> remove this as well

Done.

https://codereview.chromium.org/257153003/diff/40001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right):

https://codereview.chromium.org/257153003/diff/40001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:134: web_contents_
= NULL;
On 2014/05/02 19:34:11, jam wrote:
> please remove web_contents_ from this class and switch it to use
web_contents()
> instead?

Done.

https://codereview.chromium.org/257153003/diff/40001/content/public/browser/w...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/257153003/diff/40001/content/public/browser/w...
content/public/browser/web_contents_observer.h:296: // process is split in two
phases (untrusted and trusted).
On 2014/05/02 19:34:11, jam wrote:
> this trusted and untrusted terminology is a bit confusing and not standard, so
> i'd leave this out.
> 
> i would stay with something like 
> 
> // Invoked when the WebContents is being destroyed. Gives subclasses a chance
> // to cleanup. After this method returns, web_contents() returns NULL.

Done.

https://codereview.chromium.org/257153003/diff/40001/content/public/browser/w...
content/public/browser/web_contents_observer.h:372: void
WebContentsDestroyedPrivate() { web_contents_ = NULL; }
On 2014/05/05 15:05:03, jam wrote:
> On 2014/05/05 12:40:43, zverre wrote:
> > On 2014/05/02 19:34:11, jam wrote:
> > > nit: since it's inline and it's a simple method, it should just be
> > > reset_web_contents() { web_contents_ = NULL; }
> > > 
> > > or really since it's private and the calling class is friends, i don't see
a
> > > need to have a method at all
> > 
> > Jam, thank you for your comments. I don't really want to write loop manually
> > because actual list of observers is private and only
> ObserverListBase::Iterator
> > can get actual observers' pointers. Also this approach seems not standard
and
> > looks a bit weird in Chromium code. It's much more easy to find observers'
> > invocation in the code if it is always done with FOR_EACH_OBSERVER. So I'd
> > better stay with reset_web_contents as a better alternative to
> > WebContentsDestroyedPrivate.
> 
> ok sure. i don't feel strongly about it (btw for the record, we have some
places
> that iterate manually over the observer list)

Done.

[zverre, 2014-05-05 17:29:29]

Guys PTAL.
benm@chromium.org: PTAL android_weview/
wittman@chromium.org: PTAL components/web_modal/
sadrul@chromium.org: PTAL for ui/keyboard/

[sadrul, 2014-05-05 17:34:13]

stamp lgtm for ui/keyboard/

[jam, 2014-05-05 17:46:45]

https://codereview.chromium.org/257153003/diff/70099/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_popup_controller_interactive_uitest.cc
(right):

https://codereview.chromium.org/257153003/diff/70099/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_popup_controller_interactive_uitest.cc:92:
OVERRIDE {
nit: move override to previous line

https://codereview.chromium.org/257153003/diff/70099/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right):

https://codereview.chromium.org/257153003/diff/70099/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:105:
content::WebContents* web_contents_;
need to remove this also

https://codereview.chromium.org/257153003/diff/70099/content/public/browser/w...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/257153003/diff/70099/content/public/browser/w...
content/public/browser/web_contents_observer.h:297: // has been finished,
web_contents() returns NULL.
nit: don't indent before hitting 80 chars

https://codereview.chromium.org/257153003/diff/70099/content/public/browser/w...
content/public/browser/web_contents_observer.h:363: // of destroying the
WebContents. See comments near WebContentsDestroyed.
no need to comment a simple setter, remove this whole comment

[Mike Wittman, 2014-05-05 18:02:34]

components/web_modal lgtm

[zverre, 2014-05-06 09:37:25]

https://codereview.chromium.org/257153003/diff/70099/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_popup_controller_interactive_uitest.cc
(right):

https://codereview.chromium.org/257153003/diff/70099/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_popup_controller_interactive_uitest.cc:92:
OVERRIDE {
On 2014/05/05 17:46:46, jam wrote:
> nit: move override to previous line

Done.

https://codereview.chromium.org/257153003/diff/70099/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right):

https://codereview.chromium.org/257153003/diff/70099/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:105:
content::WebContents* web_contents_;
On 2014/05/05 17:46:46, jam wrote:
> need to remove this also

Done.

https://codereview.chromium.org/257153003/diff/70099/content/public/browser/w...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/257153003/diff/70099/content/public/browser/w...
content/public/browser/web_contents_observer.h:297: // has been finished,
web_contents() returns NULL.
On 2014/05/05 17:46:46, jam wrote:
> nit: don't indent before hitting 80 chars

Done.

https://codereview.chromium.org/257153003/diff/70099/content/public/browser/w...
content/public/browser/web_contents_observer.h:363: // of destroying the
WebContents. See comments near WebContentsDestroyed.
On 2014/05/05 17:46:46, jam wrote:
> no need to comment a simple setter, remove this whole comment

Done.

[benm (inactive), 2014-05-06 11:20:27]

On 2014/05/06 09:37:25, zverre wrote:
>
https://codereview.chromium.org/257153003/diff/70099/chrome/browser/ui/autofi...
> File
chrome/browser/ui/autofill/autofill_popup_controller_interactive_uitest.cc
> (right):
> 
>
https://codereview.chromium.org/257153003/diff/70099/chrome/browser/ui/autofi...
> chrome/browser/ui/autofill/autofill_popup_controller_interactive_uitest.cc:92:
> OVERRIDE {
> On 2014/05/05 17:46:46, jam wrote:
> > nit: move override to previous line
> 
> Done.
> 
>
https://codereview.chromium.org/257153003/diff/70099/chrome/browser/ui/passwo...
> File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right):
> 
>
https://codereview.chromium.org/257153003/diff/70099/chrome/browser/ui/passwo...
> chrome/browser/ui/passwords/manage_passwords_bubble_model.h:105:
> content::WebContents* web_contents_;
> On 2014/05/05 17:46:46, jam wrote:
> > need to remove this also
> 
> Done.
> 
>
https://codereview.chromium.org/257153003/diff/70099/content/public/browser/w...
> File content/public/browser/web_contents_observer.h (right):
> 
>
https://codereview.chromium.org/257153003/diff/70099/content/public/browser/w...
> content/public/browser/web_contents_observer.h:297: // has been finished,
> web_contents() returns NULL.
> On 2014/05/05 17:46:46, jam wrote:
> > nit: don't indent before hitting 80 chars
> 
> Done.
> 
>
https://codereview.chromium.org/257153003/diff/70099/content/public/browser/w...
> content/public/browser/web_contents_observer.h:363: // of destroying the
> WebContents. See comments near WebContentsDestroyed.
> On 2014/05/05 17:46:46, jam wrote:
> > no need to comment a simple setter, remove this whole comment
> 
> Done.

aw lgtm

[zverre, 2014-05-07 20:01:52]

I made some changes. Fixed tests and also fixed some problems that occurred
while I was fixing build problems. Jam, could you please take a look?

[zverre, 2014-05-07 20:06:49]

The CQ bit was checked by zverre@yandex-team.ru

[commit-bot: I haz the power, 2014-05-07 20:08:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zverre@yandex-team.ru/257153003/190001

[commit-bot: I haz the power, 2014-05-07 23:13:37]

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
  android_chromium_gn_compile_rel on tryserver.chromium
  android_clang_dbg on tryserver.chromium
  android_dbg on tryserver.chromium
  chromium_presubmit on tryserver.chromium
  ios_dbg_simulator on tryserver.chromium
  ios_rel_device on tryserver.chromium
  ios_rel_device_ninja on tryserver.chromium
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
  linux_chromium_chromeos_rel on tryserver.chromium
  linux_chromium_clang_dbg on tryserver.chromium
  linux_chromium_gn_rel on tryserver.chromium
  linux_chromium_rel on tryserver.chromium
  mac_chromium_compile_dbg on tryserver.chromium
  mac_chromium_rel on tryserver.chromium
  win_chromium_rel on tryserver.chromium
  win_chromium_x64_rel on tryserver.chromium

[commit-bot: I haz the power, 2014-05-08 02:50:59]

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
  android_chromium_gn_compile_rel on tryserver.chromium
  android_clang_dbg on tryserver.chromium
  android_dbg on tryserver.chromium
  chromium_presubmit on tryserver.chromium
  ios_dbg_simulator on tryserver.chromium
  ios_rel_device on tryserver.chromium
  ios_rel_device_ninja on tryserver.chromium
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
  linux_chromium_chromeos_rel on tryserver.chromium
  linux_chromium_clang_dbg on tryserver.chromium
  linux_chromium_gn_rel on tryserver.chromium
  linux_chromium_rel on tryserver.chromium
  mac_chromium_compile_dbg on tryserver.chromium
  mac_chromium_rel on tryserver.chromium
  win_chromium_compile_dbg on tryserver.chromium
  win_chromium_rel on tryserver.chromium
  win_chromium_x64_rel on tryserver.chromium

[commit-bot: I haz the power, 2014-05-08 02:55:44]

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

[commit-bot: I haz the power, 2014-05-08 02:55:45]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium
  chromium_presubmit on tryserver.chromium
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
  linux_chromium_chromeos_rel on tryserver.chromium
  linux_chromium_gn_rel on tryserver.chromium
  linux_chromium_rel on tryserver.chromium

[zverre, 2014-05-08 05:18:26]

The CQ bit was checked by zverre@yandex-team.ru

[commit-bot: I haz the power, 2014-05-08 05:20:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zverre@yandex-team.ru/257153003/210001

[Avi (use Gerrit), 2014-05-08 05:27:35]

The CQ bit was unchecked by avi@chromium.org

[Avi (use Gerrit), 2014-05-08 05:30:46]

The CQ bit was checked by avi@chromium.org

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

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zverre@yandex-team.ru/257153003/210001

[zverre, 2014-05-08 05:39:25]

The CQ bit was unchecked by zverre@yandex-team.ru

[zverre, 2014-05-08 05:41:04]

The CQ bit was checked by zverre@yandex-team.ru

[commit-bot: I haz the power, 2014-05-08 05:42:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zverre@yandex-team.ru/257153003/230001

[commit-bot: I haz the power, 2014-05-08 11:35:55]

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:
  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/...)

[commit-bot: I haz the power, 2014-05-09 13:36:12]

Change committed as 269264