DOMDataStore should be cleared when shutting down worker threads

DOMDataStore should be cleared when shutting down worker threads

- The essence of this CL is to call DOMWrapperWorld::dispose() when shutting down WorkerScriptController. This makes sure that DOMDataStore is cleared when shutting down the worker thread.

- This CL also fixes misused RefPtr<DOMWrapperWorld> and DOMWrapperWorld*. In general, we should use RefPtr<DOMWrapperWorld> when we store DOMWrapperWorld in heap-allocated objects and when we want to pass ownership around. Otherwise, we should use DOMWrapperWorld*.

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

[haraken, 2014-03-10 04:41:09]

PTAL

[kouhei (in TOK), 2014-03-10 04:49:26]

https://codereview.chromium.org/189513012/diff/1/Source/bindings/v8/WorkerScr...
File Source/bindings/v8/WorkerScriptController.cpp (right):

https://codereview.chromium.org/189513012/diff/1/Source/bindings/v8/WorkerScr...
Source/bindings/v8/WorkerScriptController.cpp:110: m_world->dispose();
Can't we just delete the world instead? Why do we need dispose?

[haraken, 2014-03-10 04:58:59]

https://codereview.chromium.org/189513012/diff/1/Source/bindings/v8/WorkerScr...
File Source/bindings/v8/WorkerScriptController.cpp (right):

https://codereview.chromium.org/189513012/diff/1/Source/bindings/v8/WorkerScr...
Source/bindings/v8/WorkerScriptController.cpp:110: m_world->dispose();
On 2014/03/10 04:49:26, kouhei wrote:
> Can't we just delete the world instead? Why do we need dispose?

Because other objects can retain RefPtr<DOMWrapperWorld>. For example,
V8MutationCallback, V8PerContextDataHolder etc retains RefPtr<DOMWrapperWorld>.
Rather than manually clearing these retainers, it's better to let RefPtr decide
when the DOMWrapperWorld is destructed.

[dcarney, 2014-03-10 08:12:27]

https://codereview.chromium.org/189513012/diff/1/Source/bindings/v8/DOMWrappe...
File Source/bindings/v8/DOMWrapperWorld.h (right):

https://codereview.chromium.org/189513012/diff/1/Source/bindings/v8/DOMWrappe...
Source/bindings/v8/DOMWrapperWorld.h:69: static void
allWorldsInMainThread(Vector<DOMWrapperWorld*>& worlds);
i don't see what ensures the safety of this.  I think it's better just to leave
as RefPtr here, and if the user of this call needs a pointer, it can deref there

[haraken, 2014-03-10 08:23:56]

PTAL

https://codereview.chromium.org/189513012/diff/1/Source/bindings/v8/DOMWrappe...
File Source/bindings/v8/DOMWrapperWorld.h (right):

https://codereview.chromium.org/189513012/diff/1/Source/bindings/v8/DOMWrappe...
Source/bindings/v8/DOMWrapperWorld.h:69: static void
allWorldsInMainThread(Vector<DOMWrapperWorld*>& worlds);
On 2014/03/10 08:12:28, dcarney wrote:
> i don't see what ensures the safety of this.  I think it's better just to
leave
> as RefPtr here, and if the user of this call needs a pointer, it can deref
there

Fixed.

(Either is fine with me. Since current(), mainWorld() etc return raw pointers,
it won't matter whether allWorldsInMainThread returns raw pointers or RefPtrs.)

[dcarney, 2014-03-10 08:33:40]

> (Either is fine with me. Since current(), mainWorld() etc return raw pointers,
> it won't matter whether allWorldsInMainThread returns raw pointers or
RefPtrs.)

yeah, i guess that's true.  anway, lgtm

[sof, 2014-03-10 10:18:26]

Might this also address some of the Worker-specific leaks that have been
reported? (e.g., http://crbug.com/332328 )

[haraken, 2014-03-10 10:21:25]

On 2014/03/10 10:18:26, sof wrote:
> Might this also address some of the Worker-specific leaks that have been
> reported? (e.g., http://crbug.com/332328 )

Hopefully.

However, if I apply the CL, a couple of IndexedDB tests start crashing so I'm
investigating.

[haraken, 2014-03-10 10:43:41]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-03-10 10:43:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/189513012/40001

[haraken, 2014-03-10 10:47:32]

https://codereview.chromium.org/189513012/diff/20001/Source/bindings/v8/Worke...
File Source/bindings/v8/WorkerScriptController.cpp (right):

https://codereview.chromium.org/189513012/diff/20001/Source/bindings/v8/Worke...
Source/bindings/v8/WorkerScriptController.cpp:110: m_world->dispose();

I noticed that m_world->dispose() must be executed before
didStopWorkerRunLoop(). Otherwise, we crash a couple of IndexedDB tests
because...

- m_world->dispose() destructs IDBRequests.
- didStopWorkerRunLoop() destructs IndexedDBDispatcher.
- IDBRequests::deref() requires IndexedDBDispatcher.

I fixed the issue in the latest CL. Landing.

[commit-bot: I haz the power, 2014-03-10 10:49:39]

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

[commit-bot: I haz the power, 2014-03-10 10:49:40]

Try jobs failed on following builders: blink_presubmit

[haraken, 2014-03-10 10:50:36]

jochen@: would you rubberstamp to web/?

[adamk, 2014-03-10 21:05:24]

lgtm for Source/web/

[haraken, 2014-03-10 22:50:06]

The CQ bit was checked by haraken@chromium.org

[haraken, 2014-03-10 22:50:12]

On 2014/03/10 21:05:24, adamk wrote:
> lgtm for Source/web/

Thanks!

[commit-bot: I haz the power, 2014-03-10 22:51:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/189513012/40001

[commit-bot: I haz the power, 2014-03-10 23:08:12]

Change committed as 168874