Ensure that scheduled tasks are executed during db thread shutdown

Ensure that scheduled tasks are executed during db thread shutdown

Since closing the databases on the database thread can schedule
new cleanup tasks, we need to postTask the notificatino that we're done.

BUG=333058
R=michaeln@chromium.org, abarth@chromium.org

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

[adamk, 2014-02-27 17:28:26]

https://codereview.chromium.org/183093002/diff/1/Source/core/workers/WorkerTh...
File Source/core/workers/WorkerThread.cpp (left):

https://codereview.chromium.org/183093002/diff/1/Source/core/workers/WorkerTh...
Source/core/workers/WorkerThread.cpp:232:
DatabaseManager::manager().interruptAllDatabasesForContext(m_workerGlobalScope.get());
I've been curious for awhile about why this line wasn't inside
WorkerThreadShutdownStartTask; I assumed there was some reason it had to run
before stopDatabases(). I hope michaeln can shed some light on this.

Anyway, if it works, this change makes me very happy, as it gets us a good bit
closer to being able to remove the dependency from core -> modules/webdatabase.

[michaeln, 2014-02-27 19:38:16]

There are cases where the workerthread can block on the dbthread (ala
taskSynchrnozer stuff). The call to 'interrupt' from the outside (as adamk put
it) kicks the dbthread to abort long running statements which will more quickly
unblock the workerthread to more quickly get started on the shutdown tasks.

(adam, does that answer your question)

Your moving the call to interrupt to the workerthread instead of having it poke
things from the mainthread. So it won't interrupt anything until after the
dbthread has done whatever amount of work is needed to unblock the workerthread
(if it had been waiting). Is that what you want? 

> since closing the databases on the database thread can schedule
> new cleanup tasks

What tasks get scheduled at that time?

[jochen (gone - plz use gerrit), 2014-02-27 19:40:55]

https://codereview.chromium.org/183093002/diff/1/Source/core/workers/WorkerTh...
File Source/core/workers/WorkerThread.cpp (left):

https://codereview.chromium.org/183093002/diff/1/Source/core/workers/WorkerTh...
Source/core/workers/WorkerThread.cpp:232:
DatabaseManager::manager().interruptAllDatabasesForContext(m_workerGlobalScope.get());
On 2014/02/27 17:28:26, adamk wrote:
> I've been curious for awhile about why this line wasn't inside
> WorkerThreadShutdownStartTask; I assumed there was some reason it had to run
> before stopDatabases(). I hope michaeln can shed some light on this.
> 
> Anyway, if it works, this change makes me very happy, as it gets us a good bit
> closer to being able to remove the dependency from core ->
modules/webdatabase.

There might be SQLTransaction steps in the message loop. By interrupt the
database, they will all abort the transaction. However, the transaction needs to
be aborted on the DB thread as well, so the SQLTransactions all postTask to the
DB thread.

By interrupting here and then postTasking the databaseClose call, we can ensure
that all the abort transactions are scheduled before we invoke databaseClose.

The actual UaF is because all other code paths neither invoke interruptAllDBs
nor then postTask.

After my CL, some of the cleanup transactions might get scheduled on the
database thread after the database thread notifies the context thread that it
has terminated. However, we drain the entire message loop before joining the DB
thread, and the cleanup transactions themselves don't postTask anywhere nor use
V8.

[jochen (gone - plz use gerrit), 2014-02-27 19:47:41]

On 2014/02/27 19:40:55, jochen (OOO from March 1) wrote:
>
https://codereview.chromium.org/183093002/diff/1/Source/core/workers/WorkerTh...
> File Source/core/workers/WorkerThread.cpp (left):
> 
>
https://codereview.chromium.org/183093002/diff/1/Source/core/workers/WorkerTh...
> Source/core/workers/WorkerThread.cpp:232:
>
DatabaseManager::manager().interruptAllDatabasesForContext(m_workerGlobalScope.get());
> On 2014/02/27 17:28:26, adamk wrote:
> > I've been curious for awhile about why this line wasn't inside
> > WorkerThreadShutdownStartTask; I assumed there was some reason it had to run
> > before stopDatabases(). I hope michaeln can shed some light on this.
> > 
> > Anyway, if it works, this change makes me very happy, as it gets us a good
bit
> > closer to being able to remove the dependency from core ->
> modules/webdatabase.
> 
> There might be SQLTransaction steps in the message loop. By interrupt the
> database, they will all abort the transaction. However, the transaction needs
to
> be aborted on the DB thread as well, so the SQLTransactions all postTask to
the
> DB thread.
> 
> By interrupting here and then postTasking the databaseClose call, we can
ensure
> that all the abort transactions are scheduled before we invoke databaseClose.
> 
> The actual UaF is because all other code paths neither invoke interruptAllDBs
> nor then postTask.

actually, that's just one class of UaF this CL fixes.

The other class is that the cleanupDatabaseThread task is not necessarily the
last task that does something. There could be (and in the test case in the bug
report, there are) transaction tasks in the message loop after the cleanup task
was executed. Those transaction will post cleanup tasks to the context thread.
By postTasking the notification, we notify the context thread after all those
transaction tasks postTasked their cleanup tasks to the context thread.

Even after the notification, new tasks might get scheduled, but they're all DB
cleanup tasks, so we can safely proceed with shutting down the worker w/o
waiting for them.

> After my CL, some of the cleanup transactions might get scheduled on the
> database thread after the database thread notifies the context thread that it
> has terminated. However, we drain the entire message loop before joining the
DB
> thread, and the cleanup transactions themselves don't postTask anywhere nor
use
> V8.

[jochen (gone - plz use gerrit), 2014-02-27 19:53:39]

On 2014/02/27 19:38:16, michaeln wrote:
> There are cases where the workerthread can block on the dbthread (ala
> taskSynchrnozer stuff). The call to 'interrupt' from the outside (as adamk put
> it) kicks the dbthread to abort long running statements which will more
quickly
> unblock the workerthread to more quickly get started on the shutdown tasks.

I don't think that's true. The worker thread only blocks on the db thread after
invoking interrupt.

> 
> (adam, does that answer your question)
> 
> Your moving the call to interrupt to the workerthread instead of having it
poke
> things from the mainthread. So it won't interrupt anything until after the
> dbthread has done whatever amount of work is needed to unblock the
workerthread
> (if it had been waiting). Is that what you want? 

The interrupt call is being made from the context thread both before and after
my cl.

> 
> > since closing the databases on the database thread can schedule
> > new cleanup tasks
> 
> What tasks get scheduled at that time?

If we abort a DatabaseTransactionTask, it invokes the
databaseThreadIsShuttingDown() which cleans up the transaction. If the
transaction has SQLCallbackWrappers, they postTask a SafeReleaseTask back to the
context thread.

[jochen (gone - plz use gerrit), 2014-02-27 20:10:42]

after chatting with Michael, I agree with his description. Esp. this means that
moving the interrupt call is wrong.

Updated the CL and description.

[michaeln, 2014-02-27 20:17:59]

lgtm (thnx!)

[jochen (gone - plz use gerrit), 2014-02-27 20:18:31]

The CQ bit was checked by jochen@chromium.org

[commit-bot: I haz the power, 2014-02-27 20:18:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jochen@chromium.org/183093002/10001

[jochen (gone - plz use gerrit), 2014-02-27 22:57:40]

Committed patchset #2 manually as r168059 (presubmit successful).

[michaeln, 2014-03-05 23:42:51]

Hi jochen, just saw this was merged so i looked again...

>> > since closing the databases on the database thread can schedule
>> > new cleanup tasks
>> 
>> What tasks get scheduled at that time?
>
> If we abort a DatabaseTransactionTask, it invokes the
> databaseThreadIsShuttingDown() which cleans up the transaction. If the
> transaction has SQLCallbackWrappers, they postTask a SafeReleaseTask back to
the
> context thread.

But something isn't quite adding up?

The SafeReleaseTask's are queued up for the context thread to process. You're
change pumps the message loop (so to speak) on the database thread prior to
waking the context thread. How does pumping the db thread affect those
SafeReleaseTasks (other than to defer their processing since the context thread
is waiting to be woken prior to processing them).

[jochen (gone - plz use gerrit), 2014-03-10 12:44:35]

On 2014/03/05 23:42:51, michaeln wrote:
> Hi jochen, just saw this was merged so i looked again...
> 
> >> > since closing the databases on the database thread can schedule
> >> > new cleanup tasks
> >> 
> >> What tasks get scheduled at that time?
> >
> > If we abort a DatabaseTransactionTask, it invokes the
> > databaseThreadIsShuttingDown() which cleans up the transaction. If the
> > transaction has SQLCallbackWrappers, they postTask a SafeReleaseTask back to
> the
> > context thread.
> 
> But something isn't quite adding up?
> 
> The SafeReleaseTask's are queued up for the context thread to process. You're
> change pumps the message loop (so to speak) on the database thread prior to
> waking the context thread. How does pumping the db thread affect those
> SafeReleaseTasks (other than to defer their processing since the context
thread
> is waiting to be woken prior to processing them).

pumping the DB message loop makes sure that all tasks that schedule stuff on the
context thread are executed.

The context thread blocks on the signal we send after pumping the DB message
loop, and only after this schedules the delete-v8-task on the context thread.

So by pumping once, I can make sure that all tasks that access v8 are on the
context thread's message loop before the context thread posts its delete-v8-task
task.

makes sense?

[michaeln, 2014-03-10 19:12:53]

> pumping the DB message loop makes sure that all tasks that schedule stuff on
the
> context thread are executed.
> 
> The context thread blocks on the signal we send after pumping the DB message
> loop, and only after this schedules the delete-v8-task on the context thread.
> 
> So by pumping once, I can make sure that all tasks that access v8 are on the
> context thread's message loop before the context thread posts its
delete-v8-task
> task.
> 
> makes sense?

yes, i think so, thnx for explaining. 

It helps by flushing any newly scheduled db-thread tasks, which can post
context-thread tasks. Doing so guarantees those  content-thread tasks run prior
to delete-v8 task post time (and that's useful for some reason :)