Bug 1436784 - Use WorkerRef in FileReader, r=smaug

dom/file/FileReader.cpp

@@ -16,7 +16,8 @@
 #include "mozilla/dom/File.h"
 #include "mozilla/dom/FileReaderBinding.h"
 #include "mozilla/dom/ProgressEvent.h"
-#include "mozilla/dom/WorkerPrivate.h"
+#include "mozilla/dom/WorkerCommon.h"
+#include "mozilla/dom/WorkerRef.h"
 #include "mozilla/dom/WorkerScope.h"
 #include "mozilla/Encoding.h"
 #include "nsAlgorithm.h"
@@ -93,9 +94,8 @@ FileReader::RootResultArrayBuffer()
 //FileReader constructors/initializers
 
 FileReader::FileReader(nsIGlobalObject* aGlobal,
-                       WorkerPrivate* aWorkerPrivate)
+                       WeakWorkerRef* aWorkerRef)
   : DOMEventTargetHelper(aGlobal)
-  , WorkerHolder("FileReader")
   , mFileData(nullptr)
   , mDataLen(0)
   , mDataFormat(FILE_AS_BINARY)
@@ -106,10 +106,10 @@ FileReader::FileReader(nsIGlobalObject* aGlobal,
   , mTotal(0)
   , mTransferred(0)
   , mBusyCount(0)
-  , mWorkerPrivate(aWorkerPrivate)
+  , mWeakWorkerRef(aWorkerRef)
 {
   MOZ_ASSERT(aGlobal);
-  MOZ_ASSERT(NS_IsMainThread() == !mWorkerPrivate);
+  MOZ_ASSERT_IF(NS_IsMainThread(), !mWeakWorkerRef);
 
   if (NS_IsMainThread()) {
     mTarget = aGlobal->EventTargetFor(TaskCategory::Other);
@@ -130,15 +130,16 @@ FileReader::~FileReader()
 FileReader::Constructor(const GlobalObject& aGlobal, ErrorResult& aRv)
 {
   nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(aGlobal.GetAsSupports());
-  WorkerPrivate* workerPrivate = nullptr;
+  RefPtr<WeakWorkerRef> workerRef;
 
   if (!NS_IsMainThread()) {
     JSContext* cx = aGlobal.Context();
-    workerPrivate = GetWorkerPrivateFromContext(cx);
-    MOZ_ASSERT(workerPrivate);
+    WorkerPrivate* workerPrivate = GetWorkerPrivateFromContext(cx);
+
+    workerRef = WeakWorkerRef::Create(workerPrivate);
   }
 
-  RefPtr<FileReader> fileReader = new FileReader(global, workerPrivate);
+  RefPtr<FileReader> fileReader = new FileReader(global, workerRef);
 
   return fileReader.forget();
 }
@@ -384,6 +385,11 @@ FileReader::ReadFileContent(Blob& aBlob,
                             eDataFormat aDataFormat,
                             ErrorResult& aRv)
 {
+  if (IsCurrentThreadRunningWorker() && !mWeakWorkerRef) {
+    // The worker is already shutting down.
+    return;
+  }
+
   if (mReadyState == LOADING) {
     aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
     return;
@@ -647,7 +653,7 @@ FileReader::OnInputStreamReady(nsIAsyncInputStream* aStream)
 
   // We use this class to decrease the busy counter at the end of this method.
   // In theory we can do it immediatelly but, for debugging reasons, we want to
-  // be 100% sure we have a workerHolder when OnLoadEnd() is called.
+  // be 100% sure we have a workerRef when OnLoadEnd() is called.
   FileReaderDecreaseBusyCounter RAII(this);
 
   uint64_t count;
@@ -778,9 +784,21 @@ FileReader::Abort()
 nsresult
 FileReader::IncreaseBusyCounter()
 {
-  if (mWorkerPrivate && mBusyCount++ == 0 &&
-      !HoldWorker(mWorkerPrivate, Closing)) {
-    return NS_ERROR_FAILURE;
+  if (mWeakWorkerRef && mBusyCount++ == 0) {
+    if (NS_WARN_IF(!mWeakWorkerRef->GetPrivate())) {
+      return NS_ERROR_FAILURE;
+    }
+
+    RefPtr<FileReader> self = this;
+
+    RefPtr<StrongWorkerRef> ref =
+      StrongWorkerRef::Create(mWeakWorkerRef->GetPrivate(), "FileReader",
+                              [self]() { self->Shutdown(); });
+    if (NS_WARN_IF(!ref)) {
+      return NS_ERROR_FAILURE;
+    }
+
+    mStrongWorkerRef = ref;
   }
 
   return NS_OK;
@@ -789,25 +807,12 @@ FileReader::IncreaseBusyCounter()
 void
 FileReader::DecreaseBusyCounter()
 {
-  MOZ_ASSERT_IF(mWorkerPrivate, mBusyCount);
-  if (mWorkerPrivate && --mBusyCount == 0) {
-    ReleaseWorker();
+  MOZ_ASSERT_IF(mStrongWorkerRef, mBusyCount);
+  if (mStrongWorkerRef && --mBusyCount == 0) {
+    mStrongWorkerRef = nullptr;
   }
 }
 
-bool
-FileReader::Notify(WorkerStatus aStatus)
-{
-  MOZ_ASSERT(mWorkerPrivate);
-  mWorkerPrivate->AssertIsOnWorkerThread();
-
-  if (aStatus > Running) {
-    Shutdown();
-  }
-
-  return true;
-}
-
 void
 FileReader::Shutdown()
 {
@@ -821,9 +826,9 @@ FileReader::Shutdown()
   FreeFileData();
   mResultArrayBuffer = nullptr;
 
-  if (mWorkerPrivate && mBusyCount != 0) {
-    ReleaseWorker();
-    mWorkerPrivate = nullptr;
+  if (mWeakWorkerRef && mBusyCount != 0) {
+    mStrongWorkerRef = nullptr;
+    mWeakWorkerRef = nullptr;
     mBusyCount = 0;
   }
 }

dom/file/FileReader.h

@@ -9,7 +9,6 @@
 
 #include "mozilla/Attributes.h"
 #include "mozilla/DOMEventTargetHelper.h"
-#include "mozilla/dom/WorkerHolder.h"
 
 #include "nsIAsyncInputStream.h"
 #include "nsIInterfaceRequestor.h"
@@ -28,7 +27,8 @@ namespace dom {
 
 class Blob;
 class DOMException;
-class WorkerPrivate;
+class StrongWorkerRef;
+class WeakWorkerRef;
 
 extern const uint64_t kUnknownSize;
 
@@ -39,14 +39,13 @@ class FileReader final : public DOMEventTargetHelper,
                          public nsSupportsWeakReference,
                          public nsIInputStreamCallback,
                          public nsITimerCallback,
-                         public nsINamed,
-                         public WorkerHolder
+                         public nsINamed
 {
   friend class FileReaderDecreaseBusyCounter;
 
 public:
   FileReader(nsIGlobalObject* aGlobal,
-             WorkerPrivate* aWorkerPrivate);
+             WeakWorkerRef* aWorkerRef);
 
   NS_DECL_ISUPPORTS_INHERITED
 
@@ -111,9 +110,6 @@ public:
     ReadFileContent(aBlob, EmptyString(), FILE_AS_BINARY, aRv);
   }
 
-  // WorkerHolder
-  bool Notify(WorkerStatus) override;
-
 private:
   virtual ~FileReader();
 
@@ -197,8 +193,14 @@ private:
 
   uint64_t mBusyCount;
 
-  // Kept alive with a WorkerHolder.
-  WorkerPrivate* mWorkerPrivate;
+  // This is set if FileReader is created on workers, but it is null if the
+  // worker is shutting down. The null value is checked in ReadFileContent()
+  // before starting any reading.
+  RefPtr<WeakWorkerRef> mWeakWorkerRef;
+
+  // This value is set when the reading starts in order to keep the worker alive
+  // during the process.
+  RefPtr<StrongWorkerRef> mStrongWorkerRef;
 };
 
 } // dom namespace

testing/web-platform/meta/MANIFEST.json

@@ -300238,6 +300238,12 @@
      {}
     ]
    ],
+   "FileAPI/FileReader/workers.html": [
+    [
+     "/FileAPI/FileReader/workers.html",
+     {}
+    ]
+   ],
    "FileAPI/FileReaderSync.worker.js": [
     [
      "/FileAPI/FileReaderSync.worker.html",
@@ -390111,6 +390117,10 @@
    "d352d5c6b5d1f2ada419d51bcfd9ecd9100bf892",
    "manual"
   ],
+  "FileAPI/FileReader/workers.html": [
+   "7e9f00c9af5bb982a1b549ebc9f5a3d0b5cf4387",
+   "testharness"
+  ],
   "FileAPI/FileReaderSync.worker.js": [
    "19741fbd0498bf9135408ceb6128221cbeb4e2f3",
    "testharness"

testing/web-platform/tests/FileAPI/FileReader/workers.html

@@ -0,0 +1,27 @@
+<!DOCTYPE html>
+<meta charset=utf-8>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script>
+
+async_test(t => {
+  function workerCode() {
+    close();
+    var blob = new Blob([123]);
+    var fr = new FileReader();
+    fr.readAsText(blob);
+    fr.abort()
+    fr.readAsArrayBuffer(blob);
+    postMessage(true);
+  }
+
+  var workerBlob = new Blob([workerCode.toSource() + ";workerCode();"], {type:"application/javascript"});
+
+  var w = new Worker(URL.createObjectURL(workerBlob));
+  w.onmessage = function(e) {
+    assert_true(e.data, "FileReader created during worker shutdown.");
+    t.done();
+  }
+}, 'FileReader created after a worker self.close()');
+
+</script>