Don't terminate SharedWorker while loading the script.

Don't terminate SharedWorker while loading the script.

This path will fix the heap-use-after-free bug.

BUG=343661, 344750
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252010

[horo, 2014-02-19 03:47:33]

kinuko@
Please review this CL

[kinuko, 2014-02-19 04:09:35]

lgtm

[horo, 2014-02-19 04:11:30]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-02-19 04:11:53]

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

[kinuko, 2014-02-19 04:24:08]

https://codereview.chromium.org/171943002/diff/1/content/worker/websharedwork...
File content/worker/websharedworker_stub.cc (right):

https://codereview.chromium.org/171943002/diff/1/content/worker/websharedwork...
content/worker/websharedworker_stub.cc:107: impl_->terminateWorkerContext();
It feels like we should always nullify the impl_ after calling this?

Do we want to call this only when running_ is true?

If we call this before WorkerScriptLoaded is called WorkerScriptLoadFailed will
be called because m_askedToTerminate is set to true in WebSharedWorkerImpl...
was that not the case?

[horo, 2014-02-19 04:27:36]

The CQ bit was unchecked by horo@chromium.org

[horo, 2014-02-19 05:29:09]

https://codereview.chromium.org/171943002/diff/1/content/worker/websharedwork...
File content/worker/websharedworker_stub.cc (right):

https://codereview.chromium.org/171943002/diff/1/content/worker/websharedwork...
content/worker/websharedworker_stub.cc:107: impl_->terminateWorkerContext();
On 2014/02/19 04:24:08, kinuko wrote:
> It feels like we should always nullify the impl_ after calling this?
> 
> Do we want to call this only when running_ is true?
> 
> If we call this before WorkerScriptLoaded is called WorkerScriptLoadFailed
will
> be called because m_askedToTerminate is set to true in WebSharedWorkerImpl...
> was that not the case?

The bug is caused by "delete this" in Shutdown() which may be called via
WorkerScriptLoaded() in the current stack.

I changed this cl to just move the position of
"impl_->terminateWorkerContext();".

[kinuko, 2014-02-19 05:34:14]

On 2014/02/19 05:29:09, horo wrote:
>
https://codereview.chromium.org/171943002/diff/1/content/worker/websharedwork...
> File content/worker/websharedworker_stub.cc (right):
> 
>
https://codereview.chromium.org/171943002/diff/1/content/worker/websharedwork...
> content/worker/websharedworker_stub.cc:107: impl_->terminateWorkerContext();
> On 2014/02/19 04:24:08, kinuko wrote:
> > It feels like we should always nullify the impl_ after calling this?
> > 
> > Do we want to call this only when running_ is true?
> > 
> > If we call this before WorkerScriptLoaded is called WorkerScriptLoadFailed
> will
> > be called because m_askedToTerminate is set to true in
WebSharedWorkerImpl...
> > was that not the case?
> 
> The bug is caused by "delete this" in Shutdown() which may be called via
> WorkerScriptLoaded() in the current stack.
> 
> I changed this cl to just move the position of
> "impl_->terminateWorkerContext();".

Cool, thanks for further investigation. LGTM if this passes the ASAN repro test

[horo, 2014-02-19 05:37:57]

The CQ bit was checked by horo@chromium.org

[horo, 2014-02-19 05:38:01]

The CQ bit was unchecked by horo@chromium.org

[horo, 2014-02-19 05:38:06]

The CQ bit was checked by horo@chromium.org

[horo, 2014-02-19 05:38:10]

The CQ bit was unchecked by horo@chromium.org

[horo, 2014-02-19 06:37:18]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-02-19 06:37:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/horo@chromium.org/171943002/150002

[commit-bot: I haz the power, 2014-02-19 08:41:03]

Change committed as 252010

[horo, 2014-02-19 16:44:39]

I forgot to change the title.

This patch only changes the order of calling terminateWorkerContext() to avoid
use-after-free crash.