Make WorkerThread lifetime much more predictable.
authorbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 30 Nov 2017 21:01:12 +0000 (21:01 +0000)
committerbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 30 Nov 2017 21:01:12 +0000 (21:01 +0000)
https://bugs.webkit.org/show_bug.cgi?id=180203

Reviewed by Chris Dumez.

No new tests (Fixes flakiness in existing and future tests).

The family of classes related to Workers has a complicated ownership model.

For Dedicated Workers, the WorkerThread object is owned by the WorkerMessagingProxy,
which manages its own lifetime. Additionally, other object(s) have raw C++ references
to it, and the expected lifetimes are described in comments scattered through a few files.

What it boils down to is that the "Worker" DOM object - which lives on the main thread -
is the key to the proper destruction of all of these objects.

For ServiceWorkers running in their own context process, there is no "Worker" on the main thread.

As a result, ServiceWorkers can get into a situation where their WorkerThread can be destroyed before
their ServiceWorkerGlobalScope is destroyed on the running background thread.

There's no reason to not have WorkerThread guarantee its own lifetime until its background thread
has actually completed.

* workers/WorkerThread.cpp:
(WebCore::WorkerThread::workerThread): Protect the WorkerThread object during the entire runtime
  of the background thread itself, and release that protection on the main thread.
* workers/WorkerThread.h:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@225343 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/workers/WorkerThread.cpp
Source/WebCore/workers/WorkerThread.h

index 92a8fdc..7a92802 100644 (file)
@@ -1,3 +1,34 @@
+2017-11-30  Brady Eidson  <beidson@apple.com>
+
+        Make WorkerThread lifetime much more predictable.
+        https://bugs.webkit.org/show_bug.cgi?id=180203
+
+        Reviewed by Chris Dumez.
+
+        No new tests (Fixes flakiness in existing and future tests).
+
+        The family of classes related to Workers has a complicated ownership model.
+
+        For Dedicated Workers, the WorkerThread object is owned by the WorkerMessagingProxy,
+        which manages its own lifetime. Additionally, other object(s) have raw C++ references
+        to it, and the expected lifetimes are described in comments scattered through a few files.
+
+        What it boils down to is that the "Worker" DOM object - which lives on the main thread - 
+        is the key to the proper destruction of all of these objects.
+
+        For ServiceWorkers running in their own context process, there is no "Worker" on the main thread.
+
+        As a result, ServiceWorkers can get into a situation where their WorkerThread can be destroyed before
+        their ServiceWorkerGlobalScope is destroyed on the running background thread.
+
+        There's no reason to not have WorkerThread guarantee its own lifetime until its background thread
+        has actually completed.
+
+        * workers/WorkerThread.cpp:
+        (WebCore::WorkerThread::workerThread): Protect the WorkerThread object during the entire runtime
+          of the background thread itself, and release that protection on the main thread.
+        * workers/WorkerThread.h:
+
 2017-11-30  Chris Dumez  <cdumez@apple.com>
 
         Populate self.registration.installing/waiting/active inside service workers
index 5e0e462..f455588 100644 (file)
@@ -149,6 +149,8 @@ bool WorkerThread::start(WTF::Function<void(const String&)>&& evaluateCallback)
 
 void WorkerThread::workerThread()
 {
+    auto protectedThis = makeRef(*this);
+
     // Propagate the mainThread's fenv to workers.
 #if PLATFORM(IOS)
     FloatingPointEnvironment::singleton().propagateMainThreadEnvironment();
@@ -231,6 +233,9 @@ void WorkerThread::workerThread()
     // Clean up WebCore::ThreadGlobalData before WTF::Thread goes away!
     threadGlobalData().destroy();
 
+    // Send the last WorkerThread Ref to be Deref'ed on the main thread.
+    callOnMainThread([protectedThis = WTFMove(protectedThis)] { });
+
     // The thread object may be already destroyed from notification now, don't try to access "this".
     protector->detach();
 }
index cc92469..49e5c4b 100644 (file)
@@ -59,7 +59,7 @@ class IDBConnectionProxy;
 
 struct WorkerThreadStartupData;
 
-class WorkerThread : public RefCounted<WorkerThread> {
+class WorkerThread : public ThreadSafeRefCounted<WorkerThread> {
 public:
     virtual ~WorkerThread();