ServiceWorker: Fix data race in shutdown sequence

Source/web/WebEmbeddedWorkerImpl.cpp

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 33 matching lines @@
 #include "core/workers/WorkerLoaderProxy.h"
 #include "core/workers/WorkerScriptLoader.h"
 #include "core/workers/WorkerScriptLoaderClient.h"
 #include "core/workers/WorkerThreadStartupData.h"
 #include "modules/serviceworkers/ServiceWorkerThread.h"
 #include "platform/SharedBuffer.h"
 #include "platform/heap/Handle.h"
 #include "platform/network/ContentSecurityPolicyParsers.h"
 #include "public/platform/Platform.h"
 #include "public/platform/WebURLRequest.h"
+#include "public/platform/WebWaitableEvent.h"
 #include "public/web/WebDevToolsAgent.h"
 #include "public/web/WebServiceWorkerContextClient.h"
 #include "public/web/WebServiceWorkerNetworkProvider.h"
 #include "public/web/WebSettings.h"
 #include "public/web/WebView.h"
 #include "public/web/WebWorkerPermissionClientProxy.h"
 #include "web/ServiceWorkerGlobalScopeClientImpl.h"
 #include "web/ServiceWorkerGlobalScopeProxy.h"
 #include "web/WebDataSourceImpl.h"
 #include "web/WebLocalFrameImpl.h"
@@ skipping 100 matching lines @@
     , m_mainFrame(0)
     , m_askedToTerminate(false)
     , m_pauseAfterDownloadState(DontPauseAfterDownload)
     , m_waitingForDebuggerState(NotWaitingForDebugger)
 {
     runningWorkerInstances().add(this);
 }
 
 WebEmbeddedWorkerImpl::~WebEmbeddedWorkerImpl()
 {
+    if (m_workerThread) {
+        m_workerThread->stop();
+        m_workerThread->terminationEvent()->wait();

[haraken, 2014/10/30 02:40:24]

Can we create a helper method like WorkerThread::terminateAndWait() and use it
here?

[Kunihiko Sakamoto, 2014/10/30 04:50:15]

Done.

+    }

[haraken, 2014/10/30 02:40:24]

Don't you need to call m_workerInspectorProxy->workerThreadTerminated() ?

[Kunihiko Sakamoto, 2014/10/30 04:50:15]

No - WebEmbeddedWorkerImpl::terminateWorkerContext() should be called
beforehand.
(For the same reason, m_workerThread->stop() above isn't really needed. I've put
it just in case.)

+
     ASSERT(runningWorkerInstances().contains(this));
     runningWorkerInstances().remove(this);
     ASSERT(m_webView);
 
     // Detach the client before closing the view to avoid getting called back.
     toWebLocalFrameImpl(m_mainFrame)->setClient(0);
 
     m_webView->close();
     m_mainFrame->close();
 }
@@ skipping 29 matching lines @@
         // This may delete 'this'.
         m_workerContextClient->workerContextFailedToStart();
         return;
     }
     if (m_pauseAfterDownloadState == IsPausedAfterDownload) {
         // This may delete 'this'.
         m_workerContextClient->workerContextFailedToStart();
         return;
     }
     if (m_workerThread)
         m_workerThread->stop();

[haraken, 2014/10/30 02:40:24]

Not related to your CL, I wonder why you don't need to wait for the worker
thread termination here.

[Kunihiko Sakamoto, 2014/10/30 04:50:15]

Worker thread can do its pending and cleanup tasks while this
WebEmbeddedWorkerImpl is alive.
Waiting its termination here might make things too sequential.

     m_workerInspectorProxy->workerThreadTerminated();
 }
 
 void WebEmbeddedWorkerImpl::resumeAfterDownload()
 {
     ASSERT(!m_askedToTerminate);
     bool wasPaused = (m_pauseAfterDownloadState == IsPausedAfterDownload);
     m_pauseAfterDownloadState = DontPauseAfterDownload;
 
     // If we were asked to wait for debugger while updating service worker version then it is good time now.
@@ skipping 186 matching lines @@
     m_mainScriptLoader.clear();
 
     m_workerGlobalScopeProxy = ServiceWorkerGlobalScopeProxy::create(*this, *document, *m_workerContextClient);
     m_loaderProxy = LoaderProxy::create(*this);
     m_workerThread = ServiceWorkerThread::create(*m_loaderProxy, *m_workerGlobalScopeProxy, startupData.release());
     m_workerThread->start();
     m_workerInspectorProxy->workerThreadCreated(document, m_workerThread.get(), scriptURL);
 }
 
 } // namespace blink