Oilpan: Fix worker thread termination use-after-free found by the asan build.

Oilpan: Fix worker thread termination use-after-free found by the asan build.

R=haraken@chromium.org, oilpan-reviews@chromium.org, vegorov@chromium.org
BUG=

[haraken, 2014-03-14 15:15:36]

LGTM as a short-term fix, but I'm not confident that this is a right fix in long
term.

Currently we're not clearing WorkerGlobalScope::m_thread in
WorkerGlobalScope::dispose(), and we have the following comment in
WorkerGlobalScope::dispose():

  // The thread reference isn't currently cleared, as the execution
  // context's thread is accessed by GCed lifetime objects when
  // they're finalized.
  // FIXME: oilpan: avoid by explicitly removing the WorkerGlobalScope
  // as an observable context right here.

(Although I haven't yet considered in details) my gut feeling is that something
is mis-designed around the lifetime of WorkerGlobalScope and WorkerThread.

[sof, 2014-03-14 15:54:56]

On 2014/03/14 15:15:36, haraken wrote:
> LGTM as a short-term fix, but I'm not confident that this is a right fix in
long
> term.
> 
> Currently we're not clearing WorkerGlobalScope::m_thread in
> WorkerGlobalScope::dispose(), and we have the following comment in
> WorkerGlobalScope::dispose():
> 
>   // The thread reference isn't currently cleared, as the execution
>   // context's thread is accessed by GCed lifetime objects when
>   // they're finalized.
>   // FIXME: oilpan: avoid by explicitly removing the WorkerGlobalScope
>   // as an observable context right here.
> 
> (Although I haven't yet considered in details) my gut feeling is that
something
> is mis-designed around the lifetime of WorkerGlobalScope and WorkerThread.

Don't forget about https://codereview.chromium.org/194923007/

[haraken, 2014-03-14 15:56:18]

> Don't forget about https://codereview.chromium.org/194923007/

oh, sorry, I missed that CL. Let me take a look on Monday :)

[sof, 2014-03-14 16:34:25]

On 2014/03/14 15:56:18, haraken wrote:
> > Don't forget about https://codereview.chromium.org/194923007/
> 
> oh, sorry, I missed that CL. Let me take a look on Monday :)

Good thing this was caught, I ran into something quite similar with Oilpan
disabled (see comment on that CL.)

When to notify the proxy that the WorkerGlobalScope has been destroyed, seems
relevant.

[sof, 2014-03-16 14:24:25]

On 2014/03/14 16:34:25, sof wrote:
> On 2014/03/14 15:56:18, haraken wrote:
> > > Don't forget about https://codereview.chromium.org/194923007/
> > 
> > oh, sorry, I missed that CL. Let me take a look on Monday :)
> 
> Good thing this was caught, I ran into something quite similar with Oilpan
> disabled (see comment on that CL.)
> 
> When to notify the proxy that the WorkerGlobalScope has been destroyed, seems
> relevant.

I've adjusted that via https://codereview.chromium.org/194923007/ , delaying the
proxy notification until it has been disposed of and the final GC has run.

The tryruns done so far hasn't showed up anything.

[Mads Ager (chromium), 2014-03-17 06:24:07]

The CQ bit was checked by ager@chromium.org

[commit-bot: I haz the power, 2014-03-17 06:24:13]

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

[Mads Ager (chromium), 2014-03-17 06:30:07]

The CQ bit was unchecked by ager@chromium.org

[Mads Ager (chromium), 2014-03-17 06:48:55]

Thanks Sigbjørn! This made almost all worker tests fail with the asan build.
I'll rerun my asan build with your patch later today and verify that it doesn't
show up something else. I'll abandon this change in favor of the bigger cleanup
in your change. Thanks for doing that!

[sof, 2014-03-17 08:15:09]

On 2014/03/17 06:48:55, Mads Ager (chromium) wrote:
> Thanks Sigbjørn! This made almost all worker tests fail with the asan build.
> I'll rerun my asan build with your patch later today and verify that it
doesn't
> show up something else. I'll abandon this change in favor of the bigger
cleanup
> in your change. Thanks for doing that!

I'm a bit surprised why most worker tests fail (=> need to read up on ASan :) ),
but sounds like it will pass a solid verdict on whether the real problem has
been addressed.

[Mads Ager (chromium), 2014-03-17 08:25:16]

On 2014/03/17 08:15:09, sof wrote:
> On 2014/03/17 06:48:55, Mads Ager (chromium) wrote:
> > Thanks Sigbjørn! This made almost all worker tests fail with the asan build.
> > I'll rerun my asan build with your patch later today and verify that it
> doesn't
> > show up something else. I'll abandon this change in favor of the bigger
> cleanup
> > in your change. Thanks for doing that!
> 
> I'm a bit surprised why most worker tests fail (=> need to read up on ASan :)
),
> but sounds like it will pass a solid verdict on whether the real problem has
> been addressed.

ASAN poisons memory that has been freed and traps on access to memory that has
been freed. It also delays reuse of memory so that memory can stay poisoned for
a long time. The assignment to m_workerGlobalScope after the proxy has been
notified is a use after free if the main thread gets to destroy the worker
thread object before the assignment. It manages to do that in a lot of the
worker thread tests on shutdown and ASAN catches the use-after-free. :)