Fix race condition in utility process clients

A few clients of the utility process had a race condition that could lead to a browser crash if the utility process crashed.  IndexedDB was the worst offender.  WebstoreInstallHelper, the profile importer, and posix plugin loader were also affected.

As a side effect, NaClProcessHost and GpuProcessHost are now notified when their respective processes are killed and treat such an occurrence as a crash.

BUG=108871
TEST=


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

[dgrogan, 2012-01-26 03:47:02]

jsbell, could you review the indexeddb parts?

jam, could you let me know if you have opposition to the ProcessCrashed ->
ProcessCrashedOrWasKilled change?

[jam, 2012-01-26 03:56:18]

http://codereview.chromium.org/9235052/diff/1/content/browser/browser_child_p...
File content/browser/browser_child_process_host_impl.cc (right):

http://codereview.chromium.org/9235052/diff/1/content/browser/browser_child_p...
content/browser/browser_child_process_host_impl.cc:232:
delegate_->OnProcessCrashedOrWasKilled(exit_code);
it seems that consumers don't care if it crashed because it crashes or was
killed. so why not just leave it as OnProcessCrashed here?

http://codereview.chromium.org/9235052/diff/1/content/browser/utility_process...
File content/browser/utility_process_host.h (right):

http://codereview.chromium.org/9235052/diff/1/content/browser/utility_process...
content/browser/utility_process_host.h:33: public
base::SupportsWeakPtr<UtilityProcessHost> {
i dont think this is necessary. you can already get notified through the
Client::OnProcessCrashed callback.

[jsbell, 2012-01-26 04:12:48]

What does the behavior in IDB end up being after the utility process crashes,
both (1) in the call that induces the crash and (2) subsequent calls?

(1) It looks like the current call will Send, then:
waitable_event_.Wait();
DCHECK(state_ == STATE_INITIALIZED);
Will we come back from the Wait() after the crash? And would that DCHECK fail?

(2) Subsequent calls should exit early (since state_ == STATE_SHUTDOWN), with an
empty keys list (in the IDBKeyFromValue... case at least). Do the callers do the
right thing in that case (I recall seeing that we turn a length of 0 into a NULL
result, which is the same as if the key lookup failed due to the data present)


On 2012/01/26 03:56:18, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/9235052/diff/1/content/browser/browser_child_p...
> File content/browser/browser_child_process_host_impl.cc (right):
> 
>
http://codereview.chromium.org/9235052/diff/1/content/browser/browser_child_p...
> content/browser/browser_child_process_host_impl.cc:232:
> delegate_->OnProcessCrashedOrWasKilled(exit_code);
> it seems that consumers don't care if it crashed because it crashes or was
> killed. so why not just leave it as OnProcessCrashed here?

Agreed. But if the name is changed, the
UtilityProcessHost::Client::OnProcessCrashed name (and overrides) should be
changed as well to match.


>
http://codereview.chromium.org/9235052/diff/1/content/browser/utility_process...
> File content/browser/utility_process_host.h (right):
> 
>
http://codereview.chromium.org/9235052/diff/1/content/browser/utility_process...
> content/browser/utility_process_host.h:33: public
> base::SupportsWeakPtr<UtilityProcessHost> {
> i dont think this is necessary. you can already get notified through the
> Client::OnProcessCrashed callback.

Agreed -

[dgrogan, 2012-01-26 04:17:39]

http://codereview.chromium.org/9235052/diff/1/content/browser/browser_child_p...
File content/browser/browser_child_process_host_impl.cc (right):

http://codereview.chromium.org/9235052/diff/1/content/browser/browser_child_p...
content/browser/browser_child_process_host_impl.cc:232:
delegate_->OnProcessCrashedOrWasKilled(exit_code);
On 2012/01/26 03:56:18, John Abd-El-Malek wrote:
> it seems that consumers don't care if it crashed because it crashes or was
> killed. so why not just leave it as OnProcessCrashed here?

I didn't want to blur the line between crashed and killed that is made here. But
you're right, that distinction doesn't matter to the consumers and keeping the
name makes this change smaller, so: changed back to OnProcessCrashed.

http://codereview.chromium.org/9235052/diff/1/content/browser/utility_process...
File content/browser/utility_process_host.h (right):

http://codereview.chromium.org/9235052/diff/1/content/browser/utility_process...
content/browser/utility_process_host.h:33: public
base::SupportsWeakPtr<UtilityProcessHost> {
On 2012/01/26 03:56:18, John Abd-El-Malek wrote:
> i dont think this is necessary. you can already get notified through the
> Client::OnProcessCrashed callback.

Client::OnProcessCrashed is posted to the IO thread asynchronously but
UtilityProcessHost is deleted synchronously, so there's a tiny race condition
that could be triggered if a task is already queued up on the IO thread that
uses KeyUtilityClientImpl::utility_process_host_.

[jam, 2012-01-26 05:17:00]

http://codereview.chromium.org/9235052/diff/1/content/browser/utility_process...
File content/browser/utility_process_host.h (right):

http://codereview.chromium.org/9235052/diff/1/content/browser/utility_process...
content/browser/utility_process_host.h:33: public
base::SupportsWeakPtr<UtilityProcessHost> {
On 2012/01/26 04:17:39, dgrogan wrote:
> On 2012/01/26 03:56:18, John Abd-El-Malek wrote:
> > i dont think this is necessary. you can already get notified through the
> > Client::OnProcessCrashed callback.
> 
> Client::OnProcessCrashed is posted to the IO thread asynchronously but
> UtilityProcessHost is deleted synchronously, so there's a tiny race condition
> that could be triggered if a task is already queued up on the IO thread that
> uses KeyUtilityClientImpl::utility_process_host_.

ah, good point. There are other places that hang on to UtilityProcessHost. Can
you either fix them or file bugs against those owners? And also add a warning in
the header file about this issue so others don't repeat it?

http://codereview.chromium.org/9235052/diff/5001/content/browser/in_process_w...
File content/browser/in_process_webkit/indexed_db_key_utility_client.cc (right):

http://codereview.chromium.org/9235052/diff/5001/content/browser/in_process_w...
content/browser/in_process_webkit/indexed_db_key_utility_client.cc:316: if
(utility_process_host_)
nit: need brackets

[dgrogan, 2012-01-26 23:17:58]

jam, could you take another look?

http://codereview.chromium.org/9235052/diff/1/content/browser/utility_process...
File content/browser/utility_process_host.h (right):

http://codereview.chromium.org/9235052/diff/1/content/browser/utility_process...
content/browser/utility_process_host.h:33: public
base::SupportsWeakPtr<UtilityProcessHost> {
On 2012/01/26 05:17:00, John Abd-El-Malek wrote:
> On 2012/01/26 04:17:39, dgrogan wrote:
> > On 2012/01/26 03:56:18, John Abd-El-Malek wrote:
> > > i dont think this is necessary. you can already get notified through the
> > > Client::OnProcessCrashed callback.
> > 
> > Client::OnProcessCrashed is posted to the IO thread asynchronously but
> > UtilityProcessHost is deleted synchronously, so there's a tiny race
condition
> > that could be triggered if a task is already queued up on the IO thread that
> > uses KeyUtilityClientImpl::utility_process_host_.
> 
> ah, good point. There are other places that hang on to UtilityProcessHost. Can
> you either fix them or file bugs against those owners? And also add a warning
in
> the header file about this issue so others don't repeat it?

Done.

http://codereview.chromium.org/9235052/diff/5001/content/browser/in_process_w...
File content/browser/in_process_webkit/indexed_db_key_utility_client.cc (right):

http://codereview.chromium.org/9235052/diff/5001/content/browser/in_process_w...
content/browser/in_process_webkit/indexed_db_key_utility_client.cc:316: if
(utility_process_host_)
On 2012/01/26 05:17:00, John Abd-El-Malek wrote:
> nit: need brackets

Done.

[jam, 2012-01-26 23:32:15]

lgtm, thanks!

http://codereview.chromium.org/9235052/diff/3008/content/browser/utility_proc...
File content/browser/utility_process_host.h (right):

http://codereview.chromium.org/9235052/diff/3008/content/browser/utility_proc...
content/browser/utility_process_host.h:31: // Note: If you keep a ptr to an
object of this class that is used in a task
nit: i think what you want to say here is that if you keep a ptr period. the
pointer has to be used only on the IO thread. if their client is on the UI
thread, for ex, they're obviously hosed. but they're also hosed if it's on the
IO thread, there's still a PostTask to the thread as well (maybe that should be
a straight function call if it's IO thread).

[dgrogan, 2012-01-26 23:32:16]

On 2012/01/26 04:12:48, jsbell wrote:
> What does the behavior in IDB end up being after the utility process crashes,
> both (1) in the call that induces the crash and (2) subsequent calls?
> 
> (1) It looks like the current call will Send, then:
> waitable_event_.Wait();
> DCHECK(state_ == STATE_INITIALIZED);
> Will we come back from the Wait() after the crash? And would that DCHECK fail?

This will be ok.  When OnProcessCrashed is called, it calls
parent_->FinishCreatingKeys(), which sets the state to STATE_INITIALIZED and
signals.

If there's an error duing injection, we might hang the renderer.  Given the
recent security escalation of this fix (see traffic on the bug), I'd prefer to
leave investigating that for another patch.

> (2) Subsequent calls should exit early (since state_ == STATE_SHUTDOWN), with
an
> empty keys list (in the IDBKeyFromValue... case at least). Do the callers do
the
> right thing in that case (I recall seeing that we turn a length of 0 into a
NULL
> result, which is the same as if the key lookup failed due to the data present)

I think subsequent calls fail, the pages I was testing on threw javascript
errors.

Again, I'd like to investigate recovering more gracefully in a later patch,
after the security issue is taken care of.

[jsbell, 2012-01-26 23:34:51]

On 2012/01/26 23:32:16, dgrogan wrote:
> 
> If there's an error duing injection, we might hang the renderer.  Given the
> recent security escalation of this fix (see traffic on the bug), I'd prefer to
> leave investigating that for another patch.

Agreed. lgtm

[commit-bot: I haz the power, 2012-01-26 23:48:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgrogan@chromium.org/9235052/2018

[commit-bot: I haz the power, 2012-01-27 02:04:51]

Change committed as 119340