Fixing a problem, where a hung renderer process is not killed when navigating away

Fixing a problem, where a hung renderer process is not killed when navigating away


BUG=104346
TEST=Steps to reproduce listed in the bug.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=132018

[nasko, 2012-04-06 15:16:53]

Charlie, can you review the new iteration of changes for the hung renderer bug?

[Charlie Reis, 2012-04-06 22:34:14]

Getting closer!

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
File content/browser/renderer_host/render_view_host_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/render_view_host_browsertest.cc:238: // unload
acks.
We should update the last sentence now that we handle it differently.

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/render_view_host_browsertest.cc:277:
content::NOTIFICATION_RENDERER_PROCESS_CLOSED,
Might be nice if we had a more specific killed notification to listen for, but I
don't know if that's easily doable.  Could we listen for both HUNG and CLOSED?

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/render_view_host_impl.cc:458: // views using the
process as of this writing.
We can update this comment now that the API has changed from listeners to
widgets.

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/render_view_host_impl.cc:468:
DCHECK(delegate_->GetRenderViewType() == content::VIEW_TYPE_TAB_CONTENTS);
nit: Let's use DCHECK_EQ, which prints better error messages.

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/render_view_host_impl.cc:482: // the histogram.
What are the other RendererTypes?  It's possible to be one of the other types,
right?

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/render_view_host_impl.cc:496:
StartHangMonitorTimeout(TimeDelta::FromMilliseconds(kUnloadTimeoutMS));
We'll need to increment the in-flight event count here, too, right?

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/render_view_host_impl.cc:680: success ?
kUnloadTimeoutMS : hung_renderer_delay_ms_));
A comment explaining why we use the short vs the long delay would help.

Also, do we want to increment the in-flight event count in this case as well? 
We're still expecting to hear the ShouldCloseACK.

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/render_view_host_impl.cc:1785: 
nit: No whitespace needed here.  This falls in the same category.

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
File content/browser/renderer_host/render_widget_helper.h (right):

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/render_widget_helper.h:193: // Called on the IO
thread to resume a cross-site response.
nit: ...if the ack is not received as expected.

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/render_widget_host_impl.cc:631: // Set
time_when_considered_hung_ if it's null, or if it is further in the
nit: Split this into a separate sentence to make it easier to read.  Something
like "Otherwise, update time_when_considered_hung_ if the requested end time is
now sooner than before."

I don't suppose we have any unit tests for this logic, do we?  If we do, it
would be nice to add one for this case.

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/render_widget_host_impl.h:420: int
DecrementInFlightEventCount() { return --in_flight_event_count_; }
These are small enough to be inlined, so we should use unix_hacker_style().

http://www.chromium.org/developers/coding-style
"Google style strongly encourages (but does not mandate) unix_hacker style on
member functions that do so little work they're effectively free, such as
simple, non-virtual getters and setters. This is also the team position. Some
code uses CamelCase for everything; avoid writing patches just to convert these
functions to unix_hacker style, but try to use it in new code you write unless
there are encapsulation reasons why CamelCase would be much more appropriate."

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
File content/browser/renderer_host/resource_dispatcher_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/resource_dispatcher_host_impl.cc:292: bool
timed_out) {
For method declarations that spill over, each argument should be on its own
line, aligned with the open paren if possible.

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/resource_dispatcher_host_impl.cc:1095:
OnSwapOutACKImpl(params, true);
Let's add a comment saying that true means it's considered timed_out.

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/resource_dispatcher_host_impl.cc:1098: void
ResourceDispatcherHostImpl::OnSwapOutACKImpl(
nit: HandleSwapOutACK

[nasko, 2012-04-10 00:16:37]

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
File content/browser/renderer_host/render_view_host_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/render_view_host_browsertest.cc:238: // unload
acks.
On 2012/04/06 22:34:14, creis wrote:
> We should update the last sentence now that we handle it differently.

Good point! Removed the note.

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/render_view_host_browsertest.cc:277:
content::NOTIFICATION_RENDERER_PROCESS_CLOSED,
On 2012/04/06 22:34:14, creis wrote:
> Might be nice if we had a more specific killed notification to listen for, but
I
> don't know if that's easily doable.  Could we listen for both HUNG and CLOSED?

As we chatted, there isn't a current notification to listen to for killing a
process. The WindowedNotificationObserver cannot wait on more than one
notification type.

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/render_view_host_impl.cc:458: // views using the
process as of this writing.
On 2012/04/06 22:34:14, creis wrote:
> We can update this comment now that the API has changed from listeners to
> widgets.

Done.

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/render_view_host_impl.cc:468:
DCHECK(delegate_->GetRenderViewType() == content::VIEW_TYPE_TAB_CONTENTS);
On 2012/04/06 22:34:14, creis wrote:
> nit: Let's use DCHECK_EQ, which prints better error messages.

Done.

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/render_view_host_impl.cc:482: // the histogram.
On 2012/04/06 22:34:14, creis wrote:
> What are the other RendererTypes?  It's possible to be one of the other types,
> right?

No, it is an enum of Renderer and Extension. We don't kill extensions in this
code path, so we are safe with just this value. Also, the enum is defined in
histogram.xml and there is no symbolic constant to use, hence I put the actual
comment.

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/render_view_host_impl.cc:496:
StartHangMonitorTimeout(TimeDelta::FromMilliseconds(kUnloadTimeoutMS));
On 2012/04/06 22:34:14, creis wrote:
> We'll need to increment the in-flight event count here, too, right?

Yes. Good catch!

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/render_view_host_impl.cc:680: success ?
kUnloadTimeoutMS : hung_renderer_delay_ms_));
On 2012/04/06 22:34:14, creis wrote:
> A comment explaining why we use the short vs the long delay would help.
> 
> Also, do we want to increment the in-flight event count in this case as well? 
> We're still expecting to hear the ShouldCloseACK.

Yes, we are still expecting the ACK, but the count is still there. If we
increment, then we have to decrement first in the OnMsgRunBeforeUnloadConfirm
function, where we stop the timer. Technically, we do have an outstanding ACK
even in-flight, but we have an interrupt with JavaScript prompt.

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/render_view_host_impl.cc:1785: 
On 2012/04/06 22:34:14, creis wrote:
> nit: No whitespace needed here.  This falls in the same category.

Done.

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
File content/browser/renderer_host/render_widget_helper.h (right):

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/render_widget_helper.h:193: // Called on the IO
thread to resume a cross-site response.
On 2012/04/06 22:34:14, creis wrote:
> nit: ...if the ack is not received as expected.

Done.

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/render_widget_host_impl.cc:631: // Set
time_when_considered_hung_ if it's null, or if it is further in the
On 2012/04/06 22:34:14, creis wrote:
> nit: Split this into a separate sentence to make it easier to read.  Something
> like "Otherwise, update time_when_considered_hung_ if the requested end time
is
> now sooner than before."
> 
> I don't suppose we have any unit tests for this logic, do we?  If we do, it
> would be nice to add one for this case.

No, there is no unit test for this particular case. I will add one, as there is
an existing unit test for that timer.

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/render_widget_host_impl.h:420: int
DecrementInFlightEventCount() { return --in_flight_event_count_; }
On 2012/04/06 22:34:14, creis wrote:
> These are small enough to be inlined, so we should use unix_hacker_style().
> 
> http://www.chromium.org/developers/coding-style
> "Google style strongly encourages (but does not mandate) unix_hacker style on
> member functions that do so little work they're effectively free, such as
> simple, non-virtual getters and setters. This is also the team position. Some
> code uses CamelCase for everything; avoid writing patches just to convert
these
> functions to unix_hacker style, but try to use it in new code you write unless
> there are encapsulation reasons why CamelCase would be much more appropriate."

Done.

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
File content/browser/renderer_host/resource_dispatcher_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/resource_dispatcher_host_impl.cc:292: bool
timed_out) {
On 2012/04/06 22:34:14, creis wrote:
> For method declarations that spill over, each argument should be on its own
> line, aligned with the open paren if possible.

Done.

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/resource_dispatcher_host_impl.cc:1095:
OnSwapOutACKImpl(params, true);
On 2012/04/06 22:34:14, creis wrote:
> Let's add a comment saying that true means it's considered timed_out.

Done.

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/resource_dispatcher_host_impl.cc:1098: void
ResourceDispatcherHostImpl::OnSwapOutACKImpl(
On 2012/04/06 22:34:14, creis wrote:
> nit: HandleSwapOutACK

Done.

[Charlie Reis, 2012-04-10 01:19:35]

Thanks for the additional tests-- those will help a lot!

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
File content/browser/renderer_host/render_view_host_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/render_view_host_browsertest.cc:277:
content::NOTIFICATION_RENDERER_PROCESS_CLOSED,
On 2012/04/10 00:16:37, nasko wrote:
> On 2012/04/06 22:34:14, creis wrote:
> > Might be nice if we had a more specific killed notification to listen for,
but
> I
> > don't know if that's easily doable.  Could we listen for both HUNG and
CLOSED?
> 
> As we chatted, there isn't a current notification to listen to for killing a
> process. The WindowedNotificationObserver cannot wait on more than one
> notification type.

No, but we can nest them without adding any flakiness:

ui_test_utils::WindowedNotificationObserver process_exit_observer(...);
ui_test_utils::WindowedNotificationObserver process_hang_observer(...);
// Navigate
process_hang_observer.Wait();
process_exit_observer.Wait();

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/render_view_host_impl.cc:482: // the histogram.
On 2012/04/10 00:16:37, nasko wrote:
> On 2012/04/06 22:34:14, creis wrote:
> > What are the other RendererTypes?  It's possible to be one of the other
types,
> > right?
> 
> No, it is an enum of Renderer and Extension. We don't kill extensions in this
> code path, so we are safe with just this value. Also, the enum is defined in
> histogram.xml and there is no symbolic constant to use, hence I put the actual
> comment.

Actually, I think it would be possible to kill extensions with this.  For
example, an options page for an extension is loaded as a web page running in the
extension process.

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
File content/browser/renderer_host/resource_dispatcher_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/resource_dispatcher_host_impl.cc:292: bool
timed_out) {
On 2012/04/10 00:16:37, nasko wrote:
> On 2012/04/06 22:34:14, creis wrote:
> > For method declarations that spill over, each argument should be on its own
> > line, aligned with the open paren if possible.
> 
> Done.

Almost.  :)  For method declarations (as opposed to call sites), render_view_id
needs to be on its own line as well.

http://www.chromium.org/developers/coding-style
"For function declarations and definitions, put each argument on a separate line
unless everything fits on one line. [...] When calling a function, however, feel
free to collapse things to take less space.  Do whatever seems most readable."

https://chromiumcodereview.appspot.com/10008015/diff/16001/content/browser/re...
File content/browser/renderer_host/render_view_host_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10008015/diff/16001/content/browser/re...
content/browser/renderer_host/render_view_host_browsertest.cc:347: // process
when we navigate away from it.
nit: when we try to close it.

https://chromiumcodereview.appspot.com/10008015/diff/16001/content/browser/re...
content/browser/renderer_host/render_view_host_browsertest.cc:390:
ui_test_utils::NavigateToURL(browser(), infinite_unload_url);
Comment for this last one?

https://chromiumcodereview.appspot.com/10008015/diff/16001/content/browser/re...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10008015/diff/16001/content/browser/re...
content/browser/renderer_host/render_view_host_impl.cc:469:
content::VIEW_TYPE_TAB_CONTENTS);
nit: Align with the open paren.

[nasko, 2012-04-10 14:31:25]

Adding sky@chromium.org for owners approval of:
    content/browser
    content/test

rdsmith@chromium.org for owners approval of content/public/browser/.

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
File content/browser/renderer_host/render_view_host_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/render_view_host_browsertest.cc:277:
content::NOTIFICATION_RENDERER_PROCESS_CLOSED,
On 2012/04/10 01:19:35, creis wrote:
> On 2012/04/10 00:16:37, nasko wrote:
> > On 2012/04/06 22:34:14, creis wrote:
> > > Might be nice if we had a more specific killed notification to listen for,
> but
> > I
> > > don't know if that's easily doable.  Could we listen for both HUNG and
> CLOSED?
> > 
> > As we chatted, there isn't a current notification to listen to for killing a
> > process. The WindowedNotificationObserver cannot wait on more than one
> > notification type.
> 
> No, but we can nest them without adding any flakiness:
> 
> ui_test_utils::WindowedNotificationObserver process_exit_observer(...);
> ui_test_utils::WindowedNotificationObserver process_hang_observer(...);
> // Navigate
> process_hang_observer.Wait();
> process_exit_observer.Wait();

What would this add to the test? In your example, if the HANG notification is
not fired, the test will hang and fail.

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/render_view_host_impl.cc:482: // the histogram.
On 2012/04/10 01:19:35, creis wrote:
> On 2012/04/10 00:16:37, nasko wrote:
> > On 2012/04/06 22:34:14, creis wrote:
> > > What are the other RendererTypes?  It's possible to be one of the other
> types,
> > > right?
> > 
> > No, it is an enum of Renderer and Extension. We don't kill extensions in
this
> > code path, so we are safe with just this value. Also, the enum is defined in
> > histogram.xml and there is no symbolic constant to use, hence I put the
actual
> > comment.
> 
> Actually, I think it would be possible to kill extensions with this.  For
> example, an options page for an extension is loaded as a web page running in
the
> extension process.

Considering this code is in the content module, which is not aware of
extensions, do we even have this information here? If not, might be better to
just not log the type.

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
File content/browser/renderer_host/resource_dispatcher_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/resource_dispatcher_host_impl.cc:292: bool
timed_out) {
On 2012/04/10 01:19:35, creis wrote:
> On 2012/04/10 00:16:37, nasko wrote:
> > On 2012/04/06 22:34:14, creis wrote:
> > > For method declarations that spill over, each argument should be on its
own
> > > line, aligned with the open paren if possible.
> > 
> > Done.
> 
> Almost.  :)  For method declarations (as opposed to call sites),
render_view_id
> needs to be on its own line as well.
> 
> http://www.chromium.org/developers/coding-style
> "For function declarations and definitions, put each argument on a separate
line
> unless everything fits on one line. [...] When calling a function, however,
feel
> free to collapse things to take less space.  Do whatever seems most readable."

Done.

https://chromiumcodereview.appspot.com/10008015/diff/16001/content/browser/re...
File content/browser/renderer_host/render_view_host_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10008015/diff/16001/content/browser/re...
content/browser/renderer_host/render_view_host_browsertest.cc:347: // process
when we navigate away from it.
On 2012/04/10 01:19:35, creis wrote:
> nit: when we try to close it.

Done.

https://chromiumcodereview.appspot.com/10008015/diff/16001/content/browser/re...
content/browser/renderer_host/render_view_host_browsertest.cc:390:
ui_test_utils::NavigateToURL(browser(), infinite_unload_url);
On 2012/04/10 01:19:35, creis wrote:
> Comment for this last one?

Done.

https://chromiumcodereview.appspot.com/10008015/diff/16001/content/browser/re...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10008015/diff/16001/content/browser/re...
content/browser/renderer_host/render_view_host_impl.cc:469:
content::VIEW_TYPE_TAB_CONTENTS);
On 2012/04/10 01:19:35, creis wrote:
> nit: Align with the open paren.

Done.

[Charlie Reis, 2012-04-10 17:22:04]

LGTM.

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
File content/browser/renderer_host/render_view_host_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/render_view_host_browsertest.cc:277:
content::NOTIFICATION_RENDERER_PROCESS_CLOSED,
On 2012/04/10 14:31:26, nasko wrote:
> What would this add to the test? In your example, if the HANG notification is
> not fired, the test will hang and fail.

Thanks for adding it in patch set 6, since it ensures we're hitting your logic
and not just exiting cleanly.

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10008015/diff/5013/content/browser/ren...
content/browser/renderer_host/render_view_host_impl.cc:482: // the histogram.
On 2012/04/10 14:31:26, nasko wrote:
> On 2012/04/10 01:19:35, creis wrote:
> > On 2012/04/10 00:16:37, nasko wrote:
> > > On 2012/04/06 22:34:14, creis wrote:
> > > > What are the other RendererTypes?  It's possible to be one of the other
> > types,
> > > > right?
> > > 
> > > No, it is an enum of Renderer and Extension. We don't kill extensions in
> this
> > > code path, so we are safe with just this value. Also, the enum is defined
in
> > > histogram.xml and there is no symbolic constant to use, hence I put the
> actual
> > > comment.
> > 
> > Actually, I think it would be possible to kill extensions with this.  For
> > example, an options page for an extension is loaded as a web page running in
> the
> > extension process.
> 
> Considering this code is in the content module, which is not aware of
> extensions, do we even have this information here? If not, might be better to
> just not log the type.

Yeah, this doesn't matter enough to plumb it through.  It won't really give us
any more useful data in the histogram, since extensions hanging in unload
handlers aren't qualitatively different.  No worries.

https://chromiumcodereview.appspot.com/10008015/diff/19005/content/browser/re...
File content/browser/renderer_host/render_view_host_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10008015/diff/19005/content/browser/re...
content/browser/renderer_host/render_view_host_browsertest.cc:343:
"files/prompt_beforeunload_infinite_unload.html"));
This apparently isn't used anymore, so we don't need to add the file.

[sky, 2012-04-10 17:28:44]

LGTM

[commit-bot: I haz the power, 2012-04-10 21:03:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/10008015/21005

[commit-bot: I haz the power, 2012-04-10 21:03:51]

Presubmit check for 10008015-21005 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/public/browser

Presubmit checks took 2.6s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[nasko, 2012-04-10 21:11:40]

After proper reading of OWNERS, adding avi@ for review of content/public
changes.

[nasko, 2012-04-11 20:58:56]

Avi, would you be able to review this one line change today? I would like to
submit it to CQ tomorrow morning.
Thanks in advance,
Nasko

[Avi (use Gerrit), 2012-04-11 22:30:19]

lgtm

[commit-bot: I haz the power, 2012-04-12 15:19:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/10008015/21005

[commit-bot: I haz the power, 2012-04-12 15:19:57]

Can't apply patch for file
content/browser/tab_contents/render_view_host_manager.cc.
While running patch -p1 --forward --force;
patching file content/browser/tab_contents/render_view_host_manager.cc
Hunk #1 FAILED at 187.
1 out of 1 hunk FAILED -- saving rejects to file
content/browser/tab_contents/render_view_host_manager.cc.rej

[commit-bot: I haz the power, 2012-04-12 15:24:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/10008015/30001

[commit-bot: I haz the power, 2012-04-12 18:13:57]

Change committed as 132018