Added code so renderer would cleanly exit.

Added code so renderer would cleanly exit.

This code is only invoked when --renderer-clean-exit is passed to Chrome.

It does the following: 
1. Makes the browser process not send a SIGTERM to its children.
2. Makes the renderer process not call _exit() in OnChannelError().

Why is this needed? The renderer process in Chrome does not exit cleanly
currently so when Chrome is profiled for optimization we do not get
representative data and miss out on optimization opportunities.
This CL addresses that problem by ensuring that exit handlers including
profile dumpers get run before the renderer exits.

BUG=107584
TEST=Rebuilt Chrome with -fprofile-generate. Verified that the renderer
process' profile is included when Chrome is closed.


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

[asharif1, 2012-03-29 22:54:46]

Thanks to Markus' comments on my original CL:
http://codereview.chromium.org/9837053/, I was able to code up this CL quickly.

[asharif1, 2012-03-29 23:14:56]

+agl to the reviewers like the old CL. PTAL.

[Markus (顧孟勤), 2012-03-29 23:58:36]

Overall, this changelist makes me much happier. It is far less invasive and it
doesn't perform any dangerous operations from within signal handlers.

I am glad you found this solution!

My only concern is that you probably should add detailed comments. I bet, a year
from now, nobody remembers why we have these ifdef's or why we have the
seemingly random call to alarm() -- and somebody is inadvertently going to break
things.

That also means, if you don't have unittests yet, please add them. We don't want
to end up with performance regressions because PGO silently stopped working
correctly.

[asharif1, 2012-03-30 00:04:56]

On Thu, Mar 29, 2012 at 4:58 PM, <markus@chromium.org> wrote:

> Overall, this changelist makes me much happier. It is far less invasive
> and it
> doesn't perform any dangerous operations from within signal handlers.
>
> I am glad you found this solution!
>
> My only concern is that you probably should add detailed comments. I bet,
> a year
> from now, nobody remembers why we have these ifdef's or why we have the
> seemingly random call to alarm() -- and somebody is inadvertently going to
> break
> things.
>
> That also means, if you don't have unittests yet, please add them. We
> don't want
> to end up with performance regressions because PGO silently stopped working
> correctly.
>

PGO is enabled only for ChromeOS and we will detect regressions via the
autotest lab.

How do you think I should implement the unittest? I am thinking the
unittest should register an exit handler only in the renderer process and
send a kill to the browser process. Would that be possible using the
unittest framework Chrome uses?


>
>
http://codereview.chromium.**org/9936002/<http://codereview.chromium.org/9936...
>

[Markus (顧孟勤), 2012-03-30 00:28:00]

Ideally, I think, we would want a test that runs both browser and renderer
and then confirms that exit handlers have executed, when the browser kills
the renderer. But I am not very familiar with this part of the code and I
don't know if we have a good way to do this type of end-to-end test. I
normally would bug Evan as he has looked into startup and shutdown, but he
is not actually on the team any more -- so, he probably has other
priorities.

I cc'd Tony. Maybe he has an idea or maybe he can suggest who you should be
talking to.


Markus

On Thu, Mar 29, 2012 at 17:04, asharif <asharif@chromium.org> wrote:

>
>
> On Thu, Mar 29, 2012 at 4:58 PM, <markus@chromium.org> wrote:
>
>> Overall, this changelist makes me much happier. It is far less invasive
>> and it
>> doesn't perform any dangerous operations from within signal handlers.
>>
>> I am glad you found this solution!
>>
>> My only concern is that you probably should add detailed comments. I bet,
>> a year
>> from now, nobody remembers why we have these ifdef's or why we have the
>> seemingly random call to alarm() -- and somebody is inadvertently going
>> to break
>> things.
>>
>> That also means, if you don't have unittests yet, please add them. We
>> don't want
>> to end up with performance regressions because PGO silently stopped
>> working
>> correctly.
>>
>
> PGO is enabled only for ChromeOS and we will detect regressions via the
> autotest lab.
>
> How do you think I should implement the unittest? I am thinking the
> unittest should register an exit handler only in the renderer process and
> send a kill to the browser process. Would that be possible using the
> unittest framework Chrome uses?
>
>
>>
>>
http://codereview.chromium.**org/9936002/<http://codereview.chromium.org/9936...
>>
>
>

[tony, 2012-03-30 00:47:29]

Can you make this a command line flag rather than a compile time option?  Then
we could test it using a ui_test or maybe a browser_test and we don't have to
build a separate binary just to test.

John might be able to give more testing guidance.  We're trying to avoid writing
new ui tests, but that may be unavoidable.

[jam, 2012-03-30 03:50:02]

On 2012/03/30 00:47:29, tony wrote:
> Can you make this a command line flag rather than a compile time option?  Then
> we could test it using a ui_test or maybe a browser_test and we don't have to
> build a separate binary just to test.
> 
> John might be able to give more testing guidance.  We're trying to avoid
writing
> new ui tests, but that may be unavoidable.

we're trying to avoid ui_tests, but new browsr_tests are fine.

I agree that command line flag is better.

[asharif1, 2012-04-03 01:01:20]

I've converted it into a command line flag. However, I don't know how to write a
test for this.

How would I test whether all exit handlers are run correctly? Maybe I could log
a message before calling exit() and try finding that in the log?

The ideal test case would be registering an atexit handler and making sure it
gets hit. How can I do that? Ideas are welcome.

[asharif1, 2012-04-05 18:58:49]

On 2012/04/03 01:01:20, asharif1 wrote:
> I've converted it into a command line flag. However, I don't know how to write
a
> test for this.
> 
> How would I test whether all exit handlers are run correctly? Maybe I could
log
> a message before calling exit() and try finding that in the log?
> 
> The ideal test case would be registering an atexit handler and making sure it
> gets hit. How can I do that? Ideas are welcome.

Ping.

Can I implement this test in a platform-agnostic way?

[asharif1, 2012-04-05 20:03:02]

On 2012/04/05 18:58:49, asharif1 wrote:
> On 2012/04/03 01:01:20, asharif1 wrote:
> > I've converted it into a command line flag. However, I don't know how to
write
> a
> > test for this.
> > 
> > How would I test whether all exit handlers are run correctly? Maybe I could
> log
> > a message before calling exit() and try finding that in the log?
> > 
> > The ideal test case would be registering an atexit handler and making sure
it
> > gets hit. How can I do that? Ideas are welcome.
> 
> Ping.
> 
> Can I implement this test in a platform-agnostic way?

Some ideas I had were:

Using strace to make sure exit is called. Not sure what to do on Mac/Windows.

Launch Chrome as as subprocess, and find the renderer, create another thread in
that remote renderer process, ask it to register an atexit handler which
communicates back with the test process and then send a kill to the browser
process. Make sure that the communication happens.

I may be able to do that for Linux. What do you think?

[asharif1, 2012-04-05 23:00:29]

+reviewers: nirnimesh and dennisjeffrey

Nirnimesh, please read the discussion above.

I found found that a simple test that runs on posix may be the following:

temp_file=$(mktemp)
./out/Debug/chrome --renderer-clean-exit --no-sandbox
--renderer-cmd-prefix="strace -o $temp_file" news.google.com &
browser_pid=$!
sleep 10
kill $browser_pid
grep -q exit_group $temp_file

I noticed a bunch of python tests in src/chrome/test/functional. How would I go
about coding this one there? And how do I run those tests on a Linux box?

[dennis_jeffrey, 2012-04-06 00:15:29]

On 2012/04/05 23:00:29, asharif1 wrote:
> +reviewers: nirnimesh and dennisjeffrey
> 
> Nirnimesh, please read the discussion above.
> 
> I found found that a simple test that runs on posix may be the following:
> 
> temp_file=$(mktemp)
> ./out/Debug/chrome --renderer-clean-exit --no-sandbox
> --renderer-cmd-prefix="strace -o $temp_file" http://news.google.com &
> browser_pid=$!
> sleep 10
> kill $browser_pid
> grep -q exit_group $temp_file
> 
> I noticed a bunch of python tests in src/chrome/test/functional. How would I
go
> about coding this one there? And how do I run those tests on a Linux box?

The tests in chrome/test/functional are pyauto tests.  I think everything you
need in your sample test can potentially be done with a pyauto test, though I've
never killed the browser process (intentionally) with a pyauto test before, so
not sure off hand how to do that in pyauto.  Everything else should be possible:
you can use pyauto to pass extra command-line flags to Chrome, navigate to
news.google.com, sleep, and check the contents of an output file.

[Nirnimesh, 2012-04-06 01:06:30]

On 2012/04/06 00:15:29, dennis_jeffrey wrote:
> On 2012/04/05 23:00:29, asharif1 wrote:
> > +reviewers: nirnimesh and dennisjeffrey
> > 
> > Nirnimesh, please read the discussion above.
> > 
> > I found found that a simple test that runs on posix may be the following:
> > 
> > temp_file=$(mktemp)
> > ./out/Debug/chrome --renderer-clean-exit --no-sandbox
> > --renderer-cmd-prefix="strace -o $temp_file" http://news.google.com &
> > browser_pid=$!
> > sleep 10
> > kill $browser_pid
> > grep -q exit_group $temp_file
> > 
> > I noticed a bunch of python tests in src/chrome/test/functional. How would I
> go
> > about coding this one there? And how do I run those tests on a Linux box?
> 
> The tests in chrome/test/functional are pyauto tests.  I think everything you
> need in your sample test can potentially be done with a pyauto test, though
I've
> never killed the browser process (intentionally) with a pyauto test before, so
> not sure off hand how to do that in pyauto.  Everything else should be
possible:
> you can use pyauto to pass extra command-line flags to Chrome, navigate to
> http://news.google.com, sleep, and check the contents of an output file.

It's fine to kill chrome from a pyauto test.  Look at how
chrome/test/functional/indexeddb.py uses about:inducebrowsercrashforrealz to
kill chrome.
Instead you just os.kill().

Override ExtraChromeFlags() in your test to pass additional flags to chrome.
Then enable your test in the 'linux' section only in
chrome/test/functional/PYAUTO_TESTS so that it runs on linux only.

[Nirnimesh, 2012-04-06 01:06:41]

http://codereview.chromium.org/9936002/diff/10001/chrome/common/chrome_switch...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/9936002/diff/10001/chrome/common/chrome_switch...
chrome/common/chrome_switches.cc:566: // Enables clean exit of the renderer
process via calling exit();
nit: end with period, not ;

[asharif1, 2012-04-06 02:28:20]

On 2012/04/06 01:06:41, Nirnimesh wrote:
>
http://codereview.chromium.org/9936002/diff/10001/chrome/common/chrome_switch...
> File chrome/common/chrome_switches.cc (right):
> 
>
http://codereview.chromium.org/9936002/diff/10001/chrome/common/chrome_switch...
> chrome/common/chrome_switches.cc:566: // Enables clean exit of the renderer
> process via calling exit();
> nit: end with period, not ;

Thanks, dennis and nirnimesh for your input. I've added a test and verified that
it passes with my CL. If I remove the --renderer-clean-exit flag from it, it
fails.

Re-review please.

[Nirnimesh, 2012-04-06 04:34:10]

http://codereview.chromium.org/9936002/diff/18002/chrome/test/functional/test...
File chrome/test/functional/test_clean_exit.py (right):

http://codereview.chromium.org/9936002/diff/18002/chrome/test/functional/test...
chrome/test/functional/test_clean_exit.py:25: return ['--no-sandbox',
'--renderer-clean-exit',
This method should only ever append to the parent's list.
ie. return pyauto.PyUITest.ExtraChromeFlags(self) + [ '--no-sandbox', ....]

Or else weird things will happen on chromeos.

http://codereview.chromium.org/9936002/diff/18002/chrome/test/functional/test...
chrome/test/functional/test_clean_exit.py:31:
self.NavigateToURL('http://www.google.com')
You don't need to navigate to a live URL. Just use one of the files in the data
dir.
say: self.GetHttpURLForDataPath('title2.html')

http://codereview.chromium.org/9936002/diff/18002/chrome/test/functional/test...
chrome/test/functional/test_clean_exit.py:34: strace_contents =
open(self._strace_log).read()
Delete self._strace_log after this?

http://codereview.chromium.org/9936002/diff/18002/chrome/test/functional/test...
chrome/test/functional/test_clean_exit.py:35: self.assertTrue("exit_group" in
strace_contents)
use ' instead of " for consistency

[Nirnimesh, 2012-04-06 04:35:02]

Oh, and please add the test to chrome/test/functional/PYAUTO_TESTS ('linux'
section) or else it won't run.

[asharif1, 2012-04-06 18:45:58]

http://codereview.chromium.org/9936002/diff/18002/chrome/test/functional/test...
File chrome/test/functional/test_clean_exit.py (right):

http://codereview.chromium.org/9936002/diff/18002/chrome/test/functional/test...
chrome/test/functional/test_clean_exit.py:25: return ['--no-sandbox',
'--renderer-clean-exit',
On 2012/04/06 04:34:10, Nirnimesh wrote:
> This method should only ever append to the parent's list.
> ie. return pyauto.PyUITest.ExtraChromeFlags(self) + [ '--no-sandbox', ....]
> 
> Or else weird things will happen on chromeos.

Done.

http://codereview.chromium.org/9936002/diff/18002/chrome/test/functional/test...
chrome/test/functional/test_clean_exit.py:31:
self.NavigateToURL('http://www.google.com')
On 2012/04/06 04:34:10, Nirnimesh wrote:
> You don't need to navigate to a live URL. Just use one of the files in the
data
> dir.
> say: self.GetHttpURLForDataPath('title2.html')

Done.

http://codereview.chromium.org/9936002/diff/18002/chrome/test/functional/test...
chrome/test/functional/test_clean_exit.py:34: strace_contents =
open(self._strace_log).read()
On 2012/04/06 04:34:10, Nirnimesh wrote:
> Delete self._strace_log after this?

Done.

http://codereview.chromium.org/9936002/diff/18002/chrome/test/functional/test...
chrome/test/functional/test_clean_exit.py:35: self.assertTrue("exit_group" in
strace_contents)
On 2012/04/06 04:34:10, Nirnimesh wrote:
> use ' instead of " for consistency

Done.

[Nirnimesh, 2012-04-06 18:51:43]

chrome/test/functional LGMT

[Nirnimesh, 2012-04-06 18:51:59]

On 2012/04/06 18:51:43, Nirnimesh wrote:
> chrome/test/functional LGMT

chrome/test/functional LGTM

[asharif1, 2012-04-06 21:19:53]

markus, jam and evan, ping.

[Markus (顧孟勤), 2012-04-06 21:41:58]

lgtm

http://codereview.chromium.org/9936002/diff/21006/chrome/common/chrome_switch...
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/9936002/diff/21006/chrome/common/chrome_switch...
chrome/common/chrome_switches.h:165: extern const char
kEnableRendererCleanExit[];
You might want to make this switch conditional (#ifdef) on just the platforms
that it affects (POSIX?).

I think, in practice, this doesn't change Chrome's behavior. But it is good
practice to document that there are platforms where we don't even expect the
switch to have any effect.

http://codereview.chromium.org/9936002/diff/21006/chrome/renderer/chrome_rend...
File chrome/renderer/chrome_render_process_observer.cc (right):

http://codereview.chromium.org/9936002/diff/21006/chrome/renderer/chrome_rend...
chrome/renderer/chrome_render_process_observer.cc:167: alarm(30);
I think I mentioned this before. A comment would be great. It's not entirely
obvious to a casual reader that we are relying on the fact that SIGALRM has a
default disposition of terminating the application.

In fact, if we wanted to be thorough, we'd reset things to SIG_DFL.

[jam, 2012-04-09 16:02:37]

http://codereview.chromium.org/9936002/diff/21006/chrome/renderer/chrome_rend...
File chrome/renderer/chrome_render_process_observer.cc (right):

http://codereview.chromium.org/9936002/diff/21006/chrome/renderer/chrome_rend...
chrome/renderer/chrome_render_process_observer.cc:148: class
SuicideOnChannelErrorFilter : public IPC::ChannelProxy::MessageFilter {
I just realized I missed moving this class to content. This belongs in content
and not chrome because we don't want every embedder of content to have to do
this.

if you don't mind, can you move this to
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/renderer/render_th...
?

http://codereview.chromium.org/9936002/diff/21006/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.cc (right):

http://codereview.chromium.org/9936002/diff/21006/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:41: #include
"chrome/common/chrome_switches.h"
content can't include files from chrome, you'd see this if you ran trybots
before sending the review (which I think is useful as it catches things like
this)

this swithc needs to be in content_switches

http://codereview.chromium.org/9936002/diff/21006/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:646:
command_line->AppendSwitch(switches::kEnableRendererCleanExit);
see the list a few lines down of switches to be copied

[jam, 2012-04-09 22:29:29]

content lgtm

[asharif1, 2012-04-09 23:26:33]

markus, PTAL.

http://codereview.chromium.org/9936002/diff/21006/chrome/common/chrome_switch...
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/9936002/diff/21006/chrome/common/chrome_switch...
chrome/common/chrome_switches.h:165: extern const char
kEnableRendererCleanExit[];
On 2012/04/06 21:41:59, Markus (顧孟勤) wrote:
> You might want to make this switch conditional (#ifdef) on just the platforms
> that it affects (POSIX?).
> 
> I think, in practice, this doesn't change Chrome's behavior. But it is good
> practice to document that there are platforms where we don't even expect the
> switch to have any effect.

Done.

http://codereview.chromium.org/9936002/diff/21006/chrome/renderer/chrome_rend...
File chrome/renderer/chrome_render_process_observer.cc (right):

http://codereview.chromium.org/9936002/diff/21006/chrome/renderer/chrome_rend...
chrome/renderer/chrome_render_process_observer.cc:148: class
SuicideOnChannelErrorFilter : public IPC::ChannelProxy::MessageFilter {
On 2012/04/09 16:02:37, John Abd-El-Malek wrote:
> I just realized I missed moving this class to content. This belongs in content
> and not chrome because we don't want every embedder of content to have to do
> this.
> 
> if you don't mind, can you move this to
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/renderer/render_th...
> ?

Separate CL uploaded here: https://chromiumcodereview.appspot.com/10027027/

http://codereview.chromium.org/9936002/diff/21006/chrome/renderer/chrome_rend...
chrome/renderer/chrome_render_process_observer.cc:167: alarm(30);
On 2012/04/06 21:41:59, Markus (顧孟勤) wrote:
> I think I mentioned this before. A comment would be great. It's not entirely
> obvious to a casual reader that we are relying on the fact that SIGALRM has a
> default disposition of terminating the application.
> 
> In fact, if we wanted to be thorough, we'd reset things to SIG_DFL.

Done.

http://codereview.chromium.org/9936002/diff/21006/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.cc (right):

http://codereview.chromium.org/9936002/diff/21006/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:41: #include
"chrome/common/chrome_switches.h"
On 2012/04/09 16:02:37, John Abd-El-Malek wrote:
> content can't include files from chrome, you'd see this if you ran trybots
> before sending the review (which I think is useful as it catches things like
> this)
> 
> this swithc needs to be in content_switches

Done.

http://codereview.chromium.org/9936002/diff/21006/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:646:
command_line->AppendSwitch(switches::kEnableRendererCleanExit);
On 2012/04/09 16:02:37, John Abd-El-Malek wrote:
> see the list a few lines down of switches to be copied

Done.

[Markus (顧孟勤), 2012-04-09 23:45:46]

http://codereview.chromium.org/9936002/diff/30003/content/public/common/conte...
File content/public/common/content_switches.cc (right):

http://codereview.chromium.org/9936002/diff/30003/content/public/common/conte...
content/public/common/content_switches.cc:522: const char kRendererCleanExit[] =
"renderer-clean-exit";
Maybe fix the indentation of the "=". All the other ones in this fix appear to
be aligned.

http://codereview.chromium.org/9936002/diff/30003/content/renderer/render_thr...
File content/renderer/render_thread_impl.cc (right):

http://codereview.chromium.org/9936002/diff/30003/content/renderer/render_thr...
content/renderer/render_thread_impl.cc:204:
HasSwitch(switches::kRendererCleanExit))
I am sure, I am just confused here. Shouldn't we be looking at the
terminate_child_on_shutdown variable that you set in the other object? Is this
variable ever used anywhere?

[asharif1, 2012-04-10 03:07:59]

http://codereview.chromium.org/9936002/diff/30003/content/public/common/conte...
File content/public/common/content_switches.cc (right):

http://codereview.chromium.org/9936002/diff/30003/content/public/common/conte...
content/public/common/content_switches.cc:522: const char kRendererCleanExit[] =
"renderer-clean-exit";
On 2012/04/09 23:45:46, Markus (顧孟勤) wrote:
> Maybe fix the indentation of the "=". All the other ones in this fix appear to
> be aligned.

Done.

http://codereview.chromium.org/9936002/diff/30003/content/renderer/render_thr...
File content/renderer/render_thread_impl.cc (right):

http://codereview.chromium.org/9936002/diff/30003/content/renderer/render_thr...
content/renderer/render_thread_impl.cc:204:
HasSwitch(switches::kRendererCleanExit))
On 2012/04/09 23:45:46, Markus (顧孟勤) wrote:
> I am sure, I am just confused here. Shouldn't we be looking at the
> terminate_child_on_shutdown variable that you set in the other object? Is this
> variable ever used anywhere?

As far as I understand, that variable is set in the parent browser process and
is unset in the case the command line flag is given. That causes the parent
process not to send a SIGTERM signal to the renderer.

In addition I need this here so _exit() is not called and the renderer has time
to actually shut down properly and call exit() along with the handlers.

I am not sure whether the renderer process uses the same ChildProcessLauncher
class. If so, I may be able to use it here...

[asharif1, 2012-04-10 03:10:25]

On 2012/04/10 03:07:59, asharif1 wrote:
>
http://codereview.chromium.org/9936002/diff/30003/content/public/common/conte...
> File content/public/common/content_switches.cc (right):
> 
>
http://codereview.chromium.org/9936002/diff/30003/content/public/common/conte...
> content/public/common/content_switches.cc:522: const char kRendererCleanExit[]
=
> "renderer-clean-exit";
> On 2012/04/09 23:45:46, Markus (顧孟勤) wrote:
> > Maybe fix the indentation of the "=". All the other ones in this fix appear
to
> > be aligned.
> 
> Done.
> 
>
http://codereview.chromium.org/9936002/diff/30003/content/renderer/render_thr...
> File content/renderer/render_thread_impl.cc (right):
> 
>
http://codereview.chromium.org/9936002/diff/30003/content/renderer/render_thr...
> content/renderer/render_thread_impl.cc:204:
> HasSwitch(switches::kRendererCleanExit))
> On 2012/04/09 23:45:46, Markus (顧孟勤) wrote:
> > I am sure, I am just confused here. Shouldn't we be looking at the
> > terminate_child_on_shutdown variable that you set in the other object? Is
this
> > variable ever used anywhere?
> 
> As far as I understand, that variable is set in the parent browser process and
> is unset in the case the command line flag is given. That causes the parent
> process not to send a SIGTERM signal to the renderer.

Forgot to mention the line # and file where that variable is used. Take a look
at: src/content/browser/child_process_launcher.cc around line 238 in void
Terminate(). Note that when the bool is unset the SIGTERM is not sent to the
renderer.

> 
> In addition I need this here so _exit() is not called and the renderer has
time
> to actually shut down properly and call exit() along with the handlers.
> 
> I am not sure whether the renderer process uses the same ChildProcessLauncher
> class. If so, I may be able to use it here...

[Markus (顧孟勤), 2012-04-10 03:31:49]

lgtm

OK. In that case, your code seems to be doing the right thing. Thanks for
checking.

[commit-bot: I haz the power, 2012-04-10 19:02:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asharif@chromium.org/9936002/8031

[commit-bot: I haz the power, 2012-04-10 20:37:36]

Change committed as 131625