Oilpan: Fix worker thread termination finalization order issues.

Oilpan: Fix worker thread termination finalization order issues.

We need to carry out the nullptr assignment to m_workerGlobalScope
before notifying the proxy that the worker thread is going away. 
Otherwise, the main thread could destroy the WorkerThread object 
which owns the m_workerGlobalScope persistent before the assignment 
and that would lead to a use-after-free.

It turns out that there are objects that keep the execution context
alive in order to be able to determine if they are being destructed
on the right thread. Therefore, clearing out the thread member
in WorkerGlobalScope cannot be done before the WorkerGlobalScope
is actually dead. Therefore, the larger change that Sigbjorn did
will not work. (One example of this kind of dependency is
SQLCallbackWrapper.)

We need to just keep the thread member intact and let objects
die normally. The call to detach needs to happen before we
actually kill the worker thread because finalizers need to
be able to access thread local data.

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

[sof, 2014-03-18 13:51:27]

lgtm 

Drop the initial para on the bug description?

[tkent, 2014-03-18 13:53:56]

lgtm

[haraken, 2014-03-18 13:55:45]

LGTM

https://codereview.chromium.org/203233004/diff/1/Source/core/workers/WorkerGl...
File Source/core/workers/WorkerGlobalScope.cpp (left):

https://codereview.chromium.org/203233004/diff/1/Source/core/workers/WorkerGl...
Source/core/workers/WorkerGlobalScope.cpp:208: // they're finalized.

Shall we keep this comment? It's a non-trivial fact that we cannot clear
m_thread here.

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

Thanks guys. I reformatted the first paragraph. I left it in here because this
change moves the notification which fixes that finalization order issue.

https://codereview.chromium.org/203233004/diff/1/Source/core/workers/WorkerGl...
File Source/core/workers/WorkerGlobalScope.cpp (left):

https://codereview.chromium.org/203233004/diff/1/Source/core/workers/WorkerGl...
Source/core/workers/WorkerGlobalScope.cpp:208: // they're finalized.
On 2014/03/18 13:55:45, haraken wrote:
> 
> Shall we keep this comment? It's a non-trivial fact that we cannot clear
> m_thread here.

Added back a comment explaining that we cannot clear the thread field before all
references to the worker global scope are gone.

[Mads Ager (chromium), 2014-03-18 14:08:16]

The CQ bit was checked by ager@chromium.org

[commit-bot: I haz the power, 2014-03-18 14:08:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ager@chromium.org/203233004/20001

[commit-bot: I haz the power, 2014-03-18 14:11:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-18 14:12:00]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_rel

[Mads Ager (chromium), 2014-03-18 14:12:28]

The CQ bit was checked by ager@chromium.org

[commit-bot: I haz the power, 2014-03-18 14:12:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ager@chromium.org/203233004/20001

[Vyacheslav Egorov (Google), 2014-03-18 14:17:46]

LGTM!

https://codereview.chromium.org/203233004/diff/20001/Source/core/workers/Work...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/203233004/diff/20001/Source/core/workers/Work...
Source/core/workers/WorkerThread.cpp:151: // otherwise leak and never be
destructed. It is important to
This comment is a bit confusing. 

It is correct that objects will not be destructed but they will be flushed down
the drain instead as underlying heap will be destroyed. So heap allocated
objects themselves will not leak, instead we are in danger of having dangling
pointers to them. That is the motivation for the assert.

It is also true that off-heap objects owned by heap objects will leak if heap
objects will not be finalized, and those potentially will leak. But dangling
pointers are a bigger danger here.

[Mads Ager (chromium), 2014-03-18 14:19:20]

The CQ bit was unchecked by ager@chromium.org

[Mads Ager (chromium), 2014-03-18 14:23:54]

https://codereview.chromium.org/203233004/diff/20001/Source/core/workers/Work...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/203233004/diff/20001/Source/core/workers/Work...
Source/core/workers/WorkerThread.cpp:151: // otherwise leak and never be
destructed. It is important to
On 2014/03/18 14:17:46, Vyacheslav Egorov (Google) wrote:
> This comment is a bit confusing. 
> 
> It is correct that objects will not be destructed but they will be flushed
down
> the drain instead as underlying heap will be destroyed. So heap allocated
> objects themselves will not leak, instead we are in danger of having dangling
> pointers to them. That is the motivation for the assert.
> 
> It is also true that off-heap objects owned by heap objects will leak if heap
> objects will not be finalized, and those potentially will leak. But dangling
> pointers are a bigger danger here.

Yes, thanks Slava. Updated the comment.

[Mads Ager (chromium), 2014-03-18 14:23:57]

The CQ bit was checked by ager@chromium.org

[commit-bot: I haz the power, 2014-03-18 14:24:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ager@chromium.org/203233004/30001

[commit-bot: I haz the power, 2014-03-18 18:58:22]

Change committed as 169460