Fix IndexedDB race condition during shutdown.

Fix IndexedDB race condition during shutdown.

Make sure IndexedDBContext is deleted before returning from
~WebKitContext. Otherwise, UtilityProcessHost may still be in batch mode
when the IO thread is shut down.

BUG=67422
TEST=browser_tests --gtest_filter=IndexedDBBrowserTest.*

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

[hans, 2011-01-12 14:52:40]

On 2011/01/12 12:57:14, hans wrote:

Looks like we break ExtensionServiceTest.KilledExtensions.. looking into that.

[jorlow, 2011-01-12 17:08:21]

http://codereview.chromium.org/6209005/diff/1/chrome/browser/in_process_webki...
File chrome/browser/in_process_webkit/webkit_context.cc (right):

http://codereview.chromium.org/6209005/diff/1/chrome/browser/in_process_webki...
chrome/browser/in_process_webkit/webkit_context.cc:30: base::WaitableEvent&
event_;
Why not a pointer?  That seems more typical.

http://codereview.chromium.org/6209005/diff/1/chrome/browser/in_process_webki...
chrome/browser/in_process_webkit/webkit_context.cc:34: void
DeleteOnThread(BrowserThread::ID thread_id, T* object) {
This is doing more than deleting on a particular thread.  Maybe the name should
somehow mention that it can block.

http://codereview.chromium.org/6209005/diff/1/chrome/browser/in_process_webki...
chrome/browser/in_process_webkit/webkit_context.cc:38: if
(BrowserThread::PostTask(thread_id, FROM_HERE, delete_task)) {
no {}'s

http://codereview.chromium.org/6209005/diff/1/chrome/browser/in_process_webki...
chrome/browser/in_process_webkit/webkit_context.cc:73:
DeleteOnThread(BrowserThread::WEBKIT, indexed_db_context_.release());
Hm...so we're going to block the io thread on the WebKit?  Is there any way to
avoid this?  Could we at least do the wait later or something?

[hans, 2011-01-13 17:05:45]

On 2011/01/12 17:08:21, jorlow wrote:
>
http://codereview.chromium.org/6209005/diff/1/chrome/browser/in_process_webki...
> File chrome/browser/in_process_webkit/webkit_context.cc (right):
> 
>
http://codereview.chromium.org/6209005/diff/1/chrome/browser/in_process_webki...
> chrome/browser/in_process_webkit/webkit_context.cc:30: base::WaitableEvent&
> event_;
> Why not a pointer?  That seems more typical.
Done.


>
http://codereview.chromium.org/6209005/diff/1/chrome/browser/in_process_webki...
> chrome/browser/in_process_webkit/webkit_context.cc:34: void
> DeleteOnThread(BrowserThread::ID thread_id, T* object) {
> This is doing more than deleting on a particular thread.  Maybe the name
should
> somehow mention that it can block.
Done.

> 
>
http://codereview.chromium.org/6209005/diff/1/chrome/browser/in_process_webki...
> chrome/browser/in_process_webkit/webkit_context.cc:38: if
> (BrowserThread::PostTask(thread_id, FROM_HERE, delete_task)) {
> no {}'s
Removed that if statement.

> 
>
http://codereview.chromium.org/6209005/diff/1/chrome/browser/in_process_webki...
> chrome/browser/in_process_webkit/webkit_context.cc:73:
> DeleteOnThread(BrowserThread::WEBKIT, indexed_db_context_.release());
> Hm...so we're going to block the io thread on the WebKit?  Is there any way to
> avoid this?  Could we at least do the wait later or something?
Yes, it would be desirable to do the wait later. Any ideas you guys can think of
are welcome.

Ideally, we could post the task here, and defer the waiting to a later stage.

Marcus and I discussed using a MessageLoop::DestructionObserver to detect the IO
thread shutdown, but I don't think that's designed to wait for the destruction
of *another* thread's MessageLoop.

The point at which it becomes important that the IndexedDBContext has been
destructed is when IOThread::Cleanup runs, i.e. at io_thread.cc:375. It would be
nice to defer the waiting to this point, but I don't see how to do that
code-wise without things getting ugly.

[hans, 2011-01-14 16:13:44]

Trying out a new approach after discussing this with Jeremy:

* Scrap the WebKitClient::idbShutdown() method as it didn't really work anyway

* Make IndexedDBKeyUtilityClient Singleton-ish, as there is only the need for
one instance

* Have IOThread::Cleanup() call IndexedDBKeyUtilityClient::Shutdown()
explicitly, assuring that shutdown happens at the right time.

Your comments are welcome.

[bulach, 2011-01-14 17:55:08]

hi hans,

the overall approach seems fine, just some suggestions that may make it clear.
let me know your thoughts:

http://codereview.chromium.org/6209005/diff/9001/chrome/browser/in_process_we...
File chrome/browser/in_process_webkit/browser_webkitclient_impl.cc (right):

http://codereview.chromium.org/6209005/diff/9001/chrome/browser/in_process_we...
chrome/browser/in_process_webkit/browser_webkitclient_impl.cc:154: if
(!indexed_db_key_utility_client_.get())
I think it no longer needs to be a member, right?
afaict, this would hold a ref for longer than needed. just use a local here.

http://codereview.chromium.org/6209005/diff/9001/chrome/browser/in_process_we...
File chrome/browser/in_process_webkit/indexed_db_key_utility_client.h (right):

http://codereview.chromium.org/6209005/diff/9001/chrome/browser/in_process_we...
chrome/browser/in_process_webkit/indexed_db_key_utility_client.h:61: static
scoped_refptr<IndexedDBKeyUtilityClient> instance_;
iirc, it's not safe / advisable to have non-POD static, and valgrind may
complain about them too..
would it be possible to wrap this all up in base::Singleton instead? (as in,
have a singleton that owns this class, possibly move it to the cc file, and
expose just GetInstance, CreateIDBKeys and Shutdown..)

[hans, 2011-01-20 14:30:17]

On 2011/01/14 17:55:08, bulach wrote:
> hi hans,
> 
> the overall approach seems fine, just some suggestions that may make it clear.
> let me know your thoughts:
> 
>
http://codereview.chromium.org/6209005/diff/9001/chrome/browser/in_process_we...
> File chrome/browser/in_process_webkit/browser_webkitclient_impl.cc (right):
> 
>
http://codereview.chromium.org/6209005/diff/9001/chrome/browser/in_process_we...
> chrome/browser/in_process_webkit/browser_webkitclient_impl.cc:154: if
> (!indexed_db_key_utility_client_.get())
> I think it no longer needs to be a member, right?
> afaict, this would hold a ref for longer than needed. just use a local here.
> 
>
http://codereview.chromium.org/6209005/diff/9001/chrome/browser/in_process_we...
> File chrome/browser/in_process_webkit/indexed_db_key_utility_client.h (right):
> 
>
http://codereview.chromium.org/6209005/diff/9001/chrome/browser/in_process_we...
> chrome/browser/in_process_webkit/indexed_db_key_utility_client.h:61: static
> scoped_refptr<IndexedDBKeyUtilityClient> instance_;
> iirc, it's not safe / advisable to have non-POD static, and valgrind may
> complain about them too..
> would it be possible to wrap this all up in base::Singleton instead? (as in,
> have a singleton that owns this class, possibly move it to the cc file, and
> expose just GetInstance, CreateIDBKeys and Shutdown..)
Doing this.

I think this solves the problem nicely. The interface becomes very simple, but
the .cc file got a bit messier, so any ideas towards making stuff cleaner are
welcome.

[bulach, 2011-01-20 16:49:15]

LGTM

a few nits / suggestions, and win bot doesn't seem too happy:

http://codereview.chromium.org/6209005/diff/16001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/browser_webkitclient_impl.cc (right):

http://codereview.chromium.org/6209005/diff/16001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/browser_webkitclient_impl.cc:155:
IndexedDBKeyUtilityClient::GetInstance();
no need for this local, just call directly
IndexedDBKeyUtilityClient::GetInstance()->Create...
below:

http://codereview.chromium.org/6209005/diff/16001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/indexed_db_key_utility_client.cc (right):

http://codereview.chromium.org/6209005/diff/16001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_key_utility_client.cc:46: friend
class IndexedDBKeyUtilityClient;
perhaps remove the friendship and just publish the relevant methods? the only
public method is called by this class anyways..

http://codereview.chromium.org/6209005/diff/16001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_key_utility_client.cc:103:
is_shutdown_ = true;
maybe swap the order?

http://codereview.chromium.org/6209005/diff/16001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/indexed_db_key_utility_client.h (right):

http://codereview.chromium.org/6209005/diff/16001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_key_utility_client.h:27: // Shut
down the underlying implementation.
add a note about which thread this must be called?

http://codereview.chromium.org/6209005/diff/16001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_key_utility_client.h:41: // The real
client; laziliy instantiated.
move the comment one down

http://codereview.chromium.org/6209005/diff/16001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/6209005/diff/16001/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:367:
IndexedDBKeyUtilityClient::GetInstance()->Shutdown();
hmm, this would always create a new instance.. maybe add a note in the ctor?

[hans, 2011-01-20 17:24:58]

> and win bot doesn't seem too happy

Forgot to provide a recent -r option.

Jeremy: any thoughts?

http://codereview.chromium.org/6209005/diff/16001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/browser_webkitclient_impl.cc (right):

http://codereview.chromium.org/6209005/diff/16001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/browser_webkitclient_impl.cc:155:
IndexedDBKeyUtilityClient::GetInstance();
On 2011/01/20 16:49:15, bulach wrote:
> no need for this local, just call directly
> IndexedDBKeyUtilityClient::GetInstance()->Create...
> below:

Done.

http://codereview.chromium.org/6209005/diff/16001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/indexed_db_key_utility_client.cc (right):

http://codereview.chromium.org/6209005/diff/16001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_key_utility_client.cc:46: friend
class IndexedDBKeyUtilityClient;
On 2011/01/20 16:49:15, bulach wrote:
> perhaps remove the friendship and just publish the relevant methods? the only
> public method is called by this class anyways..

Done.

http://codereview.chromium.org/6209005/diff/16001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_key_utility_client.cc:103:
is_shutdown_ = true;
On 2011/01/20 16:49:15, bulach wrote:
> maybe swap the order?

Done.

http://codereview.chromium.org/6209005/diff/16001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/indexed_db_key_utility_client.h (right):

http://codereview.chromium.org/6209005/diff/16001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_key_utility_client.h:27: // Shut
down the underlying implementation.
On 2011/01/20 16:49:15, bulach wrote:
> add a note about which thread this must be called?

Done.

http://codereview.chromium.org/6209005/diff/16001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_key_utility_client.h:41: // The real
client; laziliy instantiated.
On 2011/01/20 16:49:15, bulach wrote:
> move the comment one down

Whoops. Done.

http://codereview.chromium.org/6209005/diff/16001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/6209005/diff/16001/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:367:
IndexedDBKeyUtilityClient::GetInstance()->Shutdown();
On 2011/01/20 16:49:15, bulach wrote:
> hmm, this would always create a new instance.. maybe add a note in the ctor?

Yup, that's why the singleton is a thin wrapper: to make construction cheap in
case it's not really needed.

[jorlow1, 2011-01-21 09:33:00]

http://codereview.chromium.org/6209005/diff/24001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/browser_webkitclient_impl.cc (right):

http://codereview.chromium.org/6209005/diff/24001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/browser_webkitclient_impl.cc:163:
GetInstance()->CreateIDBKeysFromSerializedValuesAndKeyPath(std_values,
Isn't this what
IndexedDBKeyUtilityClient::CreateIDBKeysFromSerializedValuesAndKeyPath is for?

Should you even have a GetInstance()?  It seems too easy to shoot yourself in
the foot.

http://codereview.chromium.org/6209005/diff/24001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/indexed_db_key_utility_client.cc (right):

http://codereview.chromium.org/6209005/diff/24001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_key_utility_client.cc:13: // This
class is used to obtain IndexedDBKeys from the SerializedScriptValues
no the

http://codereview.chromium.org/6209005/diff/24001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_key_utility_client.cc:53: 
extra space?

http://codereview.chromium.org/6209005/diff/24001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_key_utility_client.cc:55: 
extra space

http://codereview.chromium.org/6209005/diff/24001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_key_utility_client.cc:95:
IndexedDBKeyUtilityClient::~IndexedDBKeyUtilityClient() { }
Assert we've been shutdown?

http://codereview.chromium.org/6209005/diff/24001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_key_utility_client.cc:98: return
Singleton<IndexedDBKeyUtilityClient>::get();
What if we've been shutdown...should we allow this?

http://codereview.chromium.org/6209005/diff/24001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/6209005/diff/24001/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:367:
IndexedDBKeyUtilityClient::GetInstance()->Shutdown();
This should be ::Shutdown() right?

[hans, 2011-01-21 11:27:55]

Thanks for the review!

http://codereview.chromium.org/6209005/diff/24001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/browser_webkitclient_impl.cc (right):

http://codereview.chromium.org/6209005/diff/24001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/browser_webkitclient_impl.cc:163:
GetInstance()->CreateIDBKeysFromSerializedValuesAndKeyPath(std_values,
On 2011/01/21 09:33:00, jorlow1 wrote:
> Isn't this what
> IndexedDBKeyUtilityClient::CreateIDBKeysFromSerializedValuesAndKeyPath is for?
> 
> Should you even have a GetInstance()?  It seems too easy to shoot yourself in
> the foot.

You're right. Removing GetInstance() and making the public members static.

After discussing with Satish, this means we probably want to use LazyInstance<>
rather than Singleton<>.

http://codereview.chromium.org/6209005/diff/24001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/indexed_db_key_utility_client.cc (right):

http://codereview.chromium.org/6209005/diff/24001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_key_utility_client.cc:13: // This
class is used to obtain IndexedDBKeys from the SerializedScriptValues
On 2011/01/21 09:33:00, jorlow1 wrote:
> no the

Done.

http://codereview.chromium.org/6209005/diff/24001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_key_utility_client.cc:53: 
On 2011/01/21 09:33:00, jorlow1 wrote:
> extra space?

Done.

http://codereview.chromium.org/6209005/diff/24001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_key_utility_client.cc:55: 
On 2011/01/21 09:33:00, jorlow1 wrote:
> extra space

Done.

http://codereview.chromium.org/6209005/diff/24001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_key_utility_client.cc:95:
IndexedDBKeyUtilityClient::~IndexedDBKeyUtilityClient() { }
On 2011/01/21 09:33:00, jorlow1 wrote:
> Assert we've been shutdown?

Done.

http://codereview.chromium.org/6209005/diff/24001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_key_utility_client.cc:98: return
Singleton<IndexedDBKeyUtilityClient>::get();
On 2011/01/21 09:33:00, jorlow1 wrote:
> What if we've been shutdown...should we allow this?

Well, we need to somehow get to the instance to check is_shutdown_?

I also think that it's important that we allow
CreateIDBKeysFromSerializedValuesAndKeyPath to be called even after shutdown, in
case there are outstanding tasks on the WebKit thread.

http://codereview.chromium.org/6209005/diff/24001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/6209005/diff/24001/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:367:
IndexedDBKeyUtilityClient::GetInstance()->Shutdown();
On 2011/01/21 09:33:00, jorlow1 wrote:
> This should be ::Shutdown() right?
Done.

[jorlow1, 2011-01-21 11:39:34]

http://codereview.chromium.org/6209005/diff/24001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/indexed_db_key_utility_client.cc (right):

http://codereview.chromium.org/6209005/diff/24001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/indexed_db_key_utility_client.cc:98: return
Singleton<IndexedDBKeyUtilityClient>::get();
On 2011/01/21 11:27:55, hans wrote:
> On 2011/01/21 09:33:00, jorlow1 wrote:
> > What if we've been shutdown...should we allow this?
> 
> Well, we need to somehow get to the instance to check is_shutdown_?
> 
> I also think that it's important that we allow
> CreateIDBKeysFromSerializedValuesAndKeyPath to be called even after shutdown,
in
> case there are outstanding tasks on the WebKit thread.

If the io thread is done, the transaction will be aborted (or at least should
be).  Returning the error condition should be fine.

Can we make it static?  Would that mess up any tests?

[hans, 2011-01-21 15:11:15]

On 2011/01/21 11:39:34, jorlow1 wrote:
>
http://codereview.chromium.org/6209005/diff/24001/chrome/browser/in_process_w...
> File chrome/browser/in_process_webkit/indexed_db_key_utility_client.cc
(right):
> 
>
http://codereview.chromium.org/6209005/diff/24001/chrome/browser/in_process_w...
> chrome/browser/in_process_webkit/indexed_db_key_utility_client.cc:98: return
> Singleton<IndexedDBKeyUtilityClient>::get();
> On 2011/01/21 11:27:55, hans wrote:
> > On 2011/01/21 09:33:00, jorlow1 wrote:
> > > What if we've been shutdown...should we allow this?
> > 
> > Well, we need to somehow get to the instance to check is_shutdown_?
> > 
> > I also think that it's important that we allow
> > CreateIDBKeysFromSerializedValuesAndKeyPath to be called even after
shutdown,
> in
> > case there are outstanding tasks on the WebKit thread.
> 
> If the io thread is done, the transaction will be aborted (or at least should
> be).  Returning the error condition should be fine.
> 
> Can we make it static?  Would that mess up any tests?

We also need to check the impl_ member to see if the implementation class was
ever instantiated.

The separation of this into one singleton and one actual implementation class is
made with the idea that even though we need to instantiate the singleton during
shutdown, at least it should be as cheap as possible, and only instantiate the
actual implementation class when we need it.

[jorlow1, 2011-01-21 15:14:12]

GTMitL

On Fri, Jan 21, 2011 at 3:11 PM, <hans@chromium.org> wrote:

> On 2011/01/21 11:39:34, jorlow1 wrote:
>
>
>
http://codereview.chromium.org/6209005/diff/24001/chrome/browser/in_process_w...
>
>> File chrome/browser/in_process_webkit/indexed_db_key_utility_client.cc
>>
> (right):
>
>
>
>
http://codereview.chromium.org/6209005/diff/24001/chrome/browser/in_process_w...
>
>> chrome/browser/in_process_webkit/indexed_db_key_utility_client.cc:98:
>> return
>> Singleton<IndexedDBKeyUtilityClient>::get();
>> On 2011/01/21 11:27:55, hans wrote:
>> > On 2011/01/21 09:33:00, jorlow1 wrote:
>> > > What if we've been shutdown...should we allow this?
>> >
>> > Well, we need to somehow get to the instance to check is_shutdown_?
>> >
>> > I also think that it's important that we allow
>> > CreateIDBKeysFromSerializedValuesAndKeyPath to be called even after
>>
> shutdown,
>
>> in
>> > case there are outstanding tasks on the WebKit thread.
>>
>
>  If the io thread is done, the transaction will be aborted (or at least
>> should
>> be).  Returning the error condition should be fine.
>>
>
>  Can we make it static?  Would that mess up any tests?
>>
>
> We also need to check the impl_ member to see if the implementation class
> was
> ever instantiated.
>
> The separation of this into one singleton and one actual implementation
> class is
> made with the idea that even though we need to instantiate the singleton
> during
> shutdown, at least it should be as cheap as possible, and only instantiate
> the
> actual implementation class when we need it.
>
>
> http://codereview.chromium.org/6209005/
>