Oilpan: DatabaseThread::m_thread should not get destructed during sweeping

Oilpan: DatabaseThread::m_thread should not get destructed during sweeping

WebThreadSupportingGC's destructor has a safe point scope
because the destructor can block until all the tasks in the WebThread are processed.
This means that the WebThreadSupportingGC shouldn't get destruction during sweeping
where we cannot enter a safe point scope.

Currently DatabaseThread::m_thread is destructed during sweeping and causes the above issue.
To avoid the issue, this CL destructs the DatabaseThread::m_thread in DatabaseThread::terminate()
(which is called when DatabaseContext stops the database thread). That way we can prevent
the DatabaseThread::m_thread from getting destructed during sweeping.

BUG=416772
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182650

[haraken, 2014-09-23 08:16:04]

haraken@chromium.org changed reviewers:
+ ager@chromium.org, oilpan-reviews@chromium.org, tkent@chromium.org

[haraken, 2014-09-23 08:16:04]

PTAL

[wibling-chromium, 2014-09-23 08:40:59]

wibling@chromium.org changed reviewers:
+ wibling@chromium.org

[wibling-chromium, 2014-09-23 08:40:59]

https://codereview.chromium.org/589363002/diff/1/Source/modules/webdatabase/D...
File Source/modules/webdatabase/DatabaseContext.cpp (right):

https://codereview.chromium.org/589363002/diff/1/Source/modules/webdatabase/D...
Source/modules/webdatabase/DatabaseContext.cpp:185: m_databaseThread->dispose();
It would be nice with a comment explaining why it is safe to dispose of the
webthread at this point. Ie. something like the sync.waitForTaskCompletion will
not return until the cleanupDatabaseThreadCompleted method has been called at
which point the webthread should no longer be reused.

That said I am not that familiar with the code to say there cannot be a new
scheduleTask call before the context is destructed?

[zerny-chromium, 2014-09-23 08:46:29]

zerny@chromium.org changed reviewers:
+ zerny@chromium.org

[zerny-chromium, 2014-09-23 08:46:29]

https://codereview.chromium.org/589363002/diff/1/Source/modules/webdatabase/D...
File Source/modules/webdatabase/DatabaseContext.cpp (right):

https://codereview.chromium.org/589363002/diff/1/Source/modules/webdatabase/D...
Source/modules/webdatabase/DatabaseContext.cpp:185: m_databaseThread->dispose();
Alternatively we could encapsulate this in the database thread abstraction, ala:

DatabaseThread::terminate(bool* terminating)
{
    TaskSynchronizer sync;
    requestTermination(&sync);
    *terminating = true;
    sync.waitForTaskCompletion();
    // Oilpan: comment about waiting before clearing goes here.
    m_thread.clear();
}

That would avoid a public dispose and some out-of-context reasoning.

[haraken, 2014-09-23 13:34:24]

Thanks for review. I removed TaskSynchronizer and simplified the code as
follows:

- Now the DatabaseContext just calls m_databaseThread->terminate().
- The m_databaseThread->terminate() destructs the WebThread. It blocks until the
cleanup task is processed in the database thread.
- No WebThread destruction takes place in the sweeping phase.

Would you take another look?

[Mads Ager (chromium), 2014-09-23 13:44:37]

https://codereview.chromium.org/589363002/diff/40001/Source/modules/webdataba...
File Source/modules/webdatabase/DatabaseThread.cpp (right):

https://codereview.chromium.org/589363002/diff/40001/Source/modules/webdataba...
Source/modules/webdatabase/DatabaseThread.cpp:79: MutexLocker
lock(m_terminationRequestedMutex);
I think we should limit the scope of this lock so it is only held while updating
terminationRequested and posting the task. We don't want to hold the lock while
waiting for the thread to terminate.

{
    MutexLocker lock(...);
    ...
    m_thread->postTask(...);
}
m_thread->clear();

?

[haraken, 2014-09-23 13:45:56]

On 2014/09/23 13:44:37, Mads Ager (chromium) wrote:
>
https://codereview.chromium.org/589363002/diff/40001/Source/modules/webdataba...
> File Source/modules/webdatabase/DatabaseThread.cpp (right):
> 
>
https://codereview.chromium.org/589363002/diff/40001/Source/modules/webdataba...
> Source/modules/webdatabase/DatabaseThread.cpp:79: MutexLocker
> lock(m_terminationRequestedMutex);
> I think we should limit the scope of this lock so it is only held while
updating
> terminationRequested and posting the task. We don't want to hold the lock
while
> waiting for the thread to terminate.
> 
> {
>     MutexLocker lock(...);
>     ...
>     m_thread->postTask(...);
> }
> m_thread->clear();
> 
> ?

Nice catch, done.

[Mads Ager (chromium), 2014-09-23 14:09:08]

https://codereview.chromium.org/589363002/diff/60001/Source/modules/webdataba...
File Source/modules/webdatabase/DatabaseThread.cpp (right):

https://codereview.chromium.org/589363002/diff/60001/Source/modules/webdataba...
Source/modules/webdatabase/DatabaseThread.cpp:87: m_thread.clear();
Hmm, how does this work. You post the cleanupDatabaseThread task and then you
set m_thread to 0. However, the cleanupDatabaseThread will then try to enqueue a
new task using m_thread. That sounds like something that could/will lead to a
null ptr crash?

[haraken, 2014-09-23 14:33:12]

On 2014/09/23 14:09:08, Mads Ager (chromium) wrote:
>
https://codereview.chromium.org/589363002/diff/60001/Source/modules/webdataba...
> File Source/modules/webdatabase/DatabaseThread.cpp (right):
> 
>
https://codereview.chromium.org/589363002/diff/60001/Source/modules/webdataba...
> Source/modules/webdatabase/DatabaseThread.cpp:87: m_thread.clear();
> Hmm, how does this work. You post the cleanupDatabaseThread task and then you
> set m_thread to 0. However, the cleanupDatabaseThread will then try to enqueue
a
> new task using m_thread. That sounds like something that could/will lead to a
> null ptr crash?

Hmm, you're right.

I uploaded a new CL, but it also crashes a couple of tests in websql/ (so not
ready for review). I'd like to take a closer look tomorrow, so let me revert the
WebThreadSupportingGC CL at the moment.

[haraken, 2014-09-24 01:08:45]

On 2014/09/23 14:33:12, haraken wrote:
> On 2014/09/23 14:09:08, Mads Ager (chromium) wrote:
> >
>
https://codereview.chromium.org/589363002/diff/60001/Source/modules/webdataba...
> > File Source/modules/webdatabase/DatabaseThread.cpp (right):
> > 
> >
>
https://codereview.chromium.org/589363002/diff/60001/Source/modules/webdataba...
> > Source/modules/webdatabase/DatabaseThread.cpp:87: m_thread.clear();
> > Hmm, how does this work. You post the cleanupDatabaseThread task and then
you
> > set m_thread to 0. However, the cleanupDatabaseThread will then try to
enqueue
> a
> > new task using m_thread. That sounds like something that could/will lead to
a
> > null ptr crash?
> 
> Hmm, you're right.
> 
> I uploaded a new CL, but it also crashes a couple of tests in websql/ (so not
> ready for review). I'd like to take a closer look tomorrow, so let me revert
the
> WebThreadSupportingGC CL at the moment.

PTAL at the patch set 5. This CL depends on another bug fix
(https://codereview.chromium.org/596083005/). With that fix applied, the patch
set 5 passes all tests in websql/.

After landing this CL, I think I can reland the WebThreadSupportingGC CL.

[zerny-chromium, 2014-09-24 10:47:58]

lgtm

[haraken, 2014-09-25 05:46:06]

Now that https://codereview.chromium.org/596083005/ landed successfully, I think
it's safe to land this CL.

Mads: Would you take a look when you have a cycle?

[Mads Ager (chromium), 2014-09-25 05:47:52]

LGTM, thanks Haraken!

[haraken, 2014-09-25 05:48:25]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-09-25 05:48:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/589363002/120001

[commit-bot: I haz the power, 2014-09-25 05:53:48]

Committed patchset #7 (id:120001) as 182650