ServiceWorker: Fix data race in shutdown sequence

ServiceWorker: Fix data race in shutdown sequence

EmbeddedWorkerDispatcher should be deleted after blink::shutdown(),
where all worker threads join with main thread. The worker termination
code needs resources owned by EmbeddedWorkerDispatcher.

BUG=426332

[Kunihiko Sakamoto, 2014-10-27 08:52:38]

ksakamoto@chromium.org changed reviewers:
+ horo@chromium.org, jamesr@chromium.org

[horo, 2014-10-27 13:38:25]

https://codereview.chromium.org/675423002/diff/1/content/renderer/render_thre...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/675423002/diff/1/content/renderer/render_thre...
content/renderer/render_thread_impl.cc:664: embedded_worker_dispatcher_.reset();
Could you please write the comments about the race here?

[Kunihiko Sakamoto, 2014-10-28 01:12:27]

https://codereview.chromium.org/675423002/diff/1/content/renderer/render_thre...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/675423002/diff/1/content/renderer/render_thre...
content/renderer/render_thread_impl.cc:664: embedded_worker_dispatcher_.reset();
On 2014/10/27 13:38:25, horo wrote:
> Could you please write the comments about the race here?

Done.

[horo, 2014-10-28 01:14:52]

lgtm

[jamesr, 2014-10-28 04:41:24]

https://codereview.chromium.org/675423002/diff/20001/content/renderer/render_...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/675423002/diff/20001/content/renderer/render_...
content/renderer/render_thread_impl.cc:665: embedded_worker_dispatcher_.reset();
will all workers be deleted and removed from embedder_worker_dispatcher's
workers_ map before this call is made? can we DCHECK that?

[Kunihiko Sakamoto, 2014-10-28 06:41:17]

https://codereview.chromium.org/675423002/diff/20001/content/renderer/render_...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/675423002/diff/20001/content/renderer/render_...
content/renderer/render_thread_impl.cc:665: embedded_worker_dispatcher_.reset();
On 2014/10/28 04:41:24, jamesr wrote:
> will all workers be deleted and removed from embedder_worker_dispatcher's
> workers_ map before this call is made? can we DCHECK that?

Unfortunately workers_ is not guaranteed to be empty at this point.
Worker's shutdown code posts a task to remove workers_ entry in main thread but
it may not be handled yet.

[jamesr, 2014-10-28 06:44:08]

OK, then I don't think this'll work - destroying entries in the workers_ map
will invoke the destructor on blink::WebEmbeddedWorker which will call into
blink.  Calling into blink after blink::shutdown() can crash since basic things
like the heap may be invalid.  Maybe you want to join the worker threads, then
destroy all the workers_, then call blink::shutdown()?

[Kunihiko Sakamoto, 2014-10-30 01:55:41]

On 2014/10/28 06:44:08, jamesr wrote:
> OK, then I don't think this'll work - destroying entries in the workers_ map
> will invoke the destructor on blink::WebEmbeddedWorker which will call into
> blink.  Calling into blink after blink::shutdown() can crash since basic
things
> like the heap may be invalid.  Maybe you want to join the worker threads, then
> destroy all the workers_, then call blink::shutdown()?

Ah right, thank you for pointing it out.
Then we should join the worker thread in the WebEmbeddedWorker destructor.
Created a blink patch: https://codereview.chromium.org/684223004/
Closing this review. Thanks!