Make sudden termination into a counter....

Make sudden termination into a counter.

We were incorrectly interpreting enableSuddenTermination to mean "enable sudden termination". Really it should just decrement a counter, and we should only enable sudden termination when that counter reaches 0.

Related to, but not the fix for, http://crbug.com/5638

This may have been causing memory corruption due to shutting down the renderer process while writing to a storage area or icon database (although honestly I don't actually know if we even use those parts of webkit).

Also, add a ui test that fails before this patch and succeeds with this patch.

BUG=none
TEST=sudden termination still works on a simple page


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

[jam, 2009-09-29 02:07:41]

http://codereview.chromium.org/245036/diff/2001/3001
File chrome/renderer/renderer_webkitclient_impl.cc (right):

http://codereview.chromium.org/245036/diff/2001/3001#newcode136
Line 136: sudden_termination_disables_ =
std::max(--sudden_termination_disables_, 0);
why is the std::max needed, i.e. under what scenarios can the calls be
unmatched?

[jam, 2009-09-29 02:08:12]

also, can you write a regression test for this?

[Evan Stade, 2009-09-29 03:42:02]

There's no easy way to write a test for this; the only test I would be able to
write would depend on implementation details of Webkit. I have sent a changelist
to WebKit that adds documentation to make this mistake less likely in the
future.

http://codereview.chromium.org/245036/diff/2001/3001
File chrome/renderer/renderer_webkitclient_impl.cc (right):

http://codereview.chromium.org/245036/diff/2001/3001#newcode136
Line 136: sudden_termination_disables_ =
std::max(--sudden_termination_disables_, 0);
On 2009/09/29 02:07:41, John Abd-El-Malek wrote:
> why is the std::max needed, i.e. under what scenarios can the calls be
> unmatched?

I got this from reading the documentation for the macosx version of these
functions. You are allowed to call enableSuddenTermination too many times,
although in practice I suppose it won't happen with webkit.

Also, it's sort of unclear to me whether the counter should start at 0 or 1, as
the mac docs say there are two ways to enable sudden termination: either call
enableSuddenTermination at program startup or set a key/value on your
application. From reading WebKit code it appears we should start at 0, hence
Safari must do the latter.

[darin (slow to review), 2009-09-29 06:10:49]

On 2009/09/29 03:42:02, Evan Stade wrote:
> There's no easy way to write a test for this

Really?  Can't you create a UI test that opens two tabs, the second spawned from
the first so that they are in the same renderer process?  Then play games with
the unload and beforeunload handlers existing / not-existing to trigger the
interesting cases.  I think there is already support for writing UI tests that
know how to test for fast termination of renderers.

[jam, 2009-09-29 17:17:55]

Darin's suggestion about how to test this is reasonable, I think we really need
tests here to avoid future regressions.

http://codereview.chromium.org/245036/diff/2001/3001
File chrome/renderer/renderer_webkitclient_impl.cc (right):

http://codereview.chromium.org/245036/diff/2001/3001#newcode136
Line 136: sudden_termination_disables_ =
std::max(--sudden_termination_disables_, 0);
On 2009/09/29 03:42:02, Evan Stade wrote:
> On 2009/09/29 02:07:41, John Abd-El-Malek wrote:
> > why is the std::max needed, i.e. under what scenarios can the calls be
> > unmatched?
> 
> I got this from reading the documentation for the macosx version of these
> functions. You are allowed to call enableSuddenTermination too many times,
> although in practice I suppose it won't happen with webkit.
> 
> Also, it's sort of unclear to me whether the counter should start at 0 or 1,
as
> the mac docs say there are two ways to enable sudden termination: either call
> enableSuddenTermination at program startup or set a key/value on your
> application. From reading WebKit code it appears we should start at 0, hence
> Safari must do the latter.
> 

It seems better to be more conservative first, and see if it brings up any
errors.  std::max can end up hiding bugs in our own binding code.

[Evan Stade, 2009-09-29 18:22:05]

> Really?  Can't you create a UI test that opens two tabs, the second spawned
from
> the first so that they are in the same renderer process?  Then play games with
> the unload and beforeunload handlers existing / not-existing to trigger the
> interesting cases.  I think there is already support for writing UI tests that
> know how to test for fast termination of renderers.

right. So my point was that if Webkit changed slightly and we somehow regressed
on this the test would fail to catch it. But I will look into it. 

> It seems better to be more conservative first, and see if it brings up any
> errors.  std::max can end up hiding bugs in our own binding code.

I will add a DCHECK, but I think the max should stay.

[Evan Stade, 2009-09-29 22:04:26]

ui test added

[darin (slow to review), 2009-09-29 22:44:43]

LG other than:

http://codereview.chromium.org/245036/diff/5002/5007
File chrome/test/ui/fast_shutdown_uitest.cc (right):

http://codereview.chromium.org/245036/diff/5002/5007#newcode21
Line 21: TEST_F(FastShutdownBrowserTest, FastTermination) {
nit: it seems like this test should be named SlowTermination or something to
indicate that it is testing for a condition that should make it not be fast.

[jam, 2009-09-29 22:50:02]

http://codereview.chromium.org/245036/diff/5002/5007
File chrome/test/ui/fast_shutdown_uitest.cc (right):

http://codereview.chromium.org/245036/diff/5002/5007#newcode1
Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.
are line endings not set for this file (and the other html files as well)?

http://codereview.chromium.org/245036/diff/5002/5007#newcode18
Line 18: // This tests for a previous error where uninstalling an onBeforeUnload
nits:

"previous error" doesn't give much information, so either add the bug number or
take it out

onBeforeUnload -> onbeforeunload

http://codereview.chromium.org/245036/diff/5002/5007#newcode28
Line 28: GURL url = GetTestUrl(L"fast_shutdown", L"OnUnloader.html");
our filenames are lower case

[Evan Stade, 2009-09-29 23:12:27]

http://codereview.chromium.org/245036/diff/5002/5007
File chrome/test/ui/fast_shutdown_uitest.cc (right):

http://codereview.chromium.org/245036/diff/5002/5007#newcode18
Line 18: // This tests for a previous error where uninstalling an onBeforeUnload
On 2009/09/29 22:50:02, John Abd-El-Malek wrote:
> nits:
> 
> "previous error" doesn't give much information, so either add the bug number
or
> take it out

there is no bug number, but I think it's a helpful comment for understanding the
point of the test.

> onBeforeUnload -> onbeforeunload

done.

http://codereview.chromium.org/245036/diff/5002/5007#newcode21
Line 21: TEST_F(FastShutdownBrowserTest, FastTermination) {
On 2009/09/29 22:44:43, darin wrote:
> nit: it seems like this test should be named SlowTermination or something to
> indicate that it is testing for a condition that should make it not be fast.

Done.

http://codereview.chromium.org/245036/diff/5002/5007#newcode28
Line 28: GURL url = GetTestUrl(L"fast_shutdown", L"OnUnloader.html");
On 2009/09/29 22:50:02, John Abd-El-Malek wrote:
> our filenames are lower case

Done.

[Evan Stade, 2009-09-30 21:00:45]

> http://codereview.chromium.org/245036/diff/5002/5007#newcode1
> Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.
> are line endings not set for this file (and the other html files as well)?

fixed, but I wonder why the presubmit check didn't catch it. This doesn't seem
to have solved the lint error though.

[Evan Stade, 2009-10-01 00:30:36]

ping.

[darin (slow to review), 2009-10-01 04:20:03]

LGTM