Bug 1798773 - Always shutdown FileSystemManager; r=dom-worker-reviewers,smaug

Differential Revision: https://phabricator.services.mozilla.com/D162200

dom/fs/api/FileSystemHandle.cpp

@@ -278,6 +278,11 @@ already_AddRefed<FileSystemFileHandle> FileSystemHandle::ConstructFileHandle(
     return nullptr;
   }
 
+  // We used to create a FileSystemManager which is not connected to the chain
+  // of Navigator->StorageManager->FileSystemManager. That isn't possible
+  // anymore because FileSystemManager must always be properly shutdown before
+  // it's destroyed.
+#if 0
   // XXX Get the manager from Navigator!
   // Note that the actor may not exist or may not be connected yet.
   auto fileSystemManager = MakeRefPtr<FileSystemManager>(aGlobal, nullptr);
@@ -286,6 +291,9 @@ already_AddRefed<FileSystemFileHandle> FileSystemHandle::ConstructFileHandle(
       new FileSystemFileHandle(aGlobal, fileSystemManager, metadata);
 
   return fsHandle.forget();
+#else
+  return nullptr;
+#endif
 }
 
 // static
@@ -301,6 +309,11 @@ FileSystemHandle::ConstructDirectoryHandle(JSContext* aCx,
     return nullptr;
   }
 
+  // We used to create a FileSystemManager which is not connected to the chain
+  // of Navigator->StorageManager->FileSystemManager. That isn't possible
+  // anymore because FileSystemManager must always be properly shutdown before
+  // it's destroyed.
+#if 0
   // XXX Get the manager from Navigator!
   // Note that the actor may not exist or may not be connected yet.
   auto fileSystemManager = MakeRefPtr<FileSystemManager>(aGlobal, nullptr);
@@ -309,6 +322,9 @@ FileSystemHandle::ConstructDirectoryHandle(JSContext* aCx,
       new FileSystemDirectoryHandle(aGlobal, fileSystemManager, metadata);
 
   return fsHandle.forget();
+#else
+  return nullptr;
+#endif
 }
 
 }  // namespace mozilla::dom

dom/fs/api/FileSystemManager.cpp

@@ -30,7 +30,7 @@ FileSystemManager::FileSystemManager(nsIGlobalObject* aGlobal,
     : FileSystemManager(aGlobal, std::move(aStorageManager),
                         MakeRefPtr<FileSystemBackgroundRequestHandler>()) {}
 
-FileSystemManager::~FileSystemManager() = default;
+FileSystemManager::~FileSystemManager() { MOZ_ASSERT(mShutdown); }
 
 NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(FileSystemManager)
   NS_INTERFACE_MAP_ENTRY(nsISupports)

dom/fs/child/FileSystemBackgroundRequestHandler.cpp

@@ -49,6 +49,11 @@ void FileSystemBackgroundRequestHandler::Shutdown() {
     mCreateFileSystemManagerParentPromiseRequestHolder.Disconnect();
 
     mCreatingFileSystemManagerChild = false;
+
+    // We must either resolve/reject the promise or steal the internal promise
+    // before the holder is destroyed. The former isn't possible during
+    // shutdown.
+    Unused << mCreateFileSystemManagerChildPromiseHolder.Steal();
   }
 }
 

dom/fs/test/crashtests/1798773.html

@@ -0,0 +1,19 @@
+<script id="worker1" type="javascript/worker">
+self.onmessage = async function () {
+  const xhr = new XMLHttpRequest()
+  self.onerror = () => {
+    xhr.open("POST", "FOOBAR", false)
+    xhr.send()
+  }
+  self.reportError(undefined)
+  self.dir = await self.navigator.storage.getDirectory()
+}
+</script>
+<script>
+window.addEventListener('load', async () => {
+  const blob = new Blob([document.querySelector('#worker1').textContent], { type: "text/javascript" })
+  let worker = new Worker(window.URL.createObjectURL(blob))
+  worker.postMessage([], [])
+  setTimeout(() => {window.location.reload(true)})
+})
+</script>

dom/fs/test/crashtests/crashtests.list

@@ -0,0 +1,3 @@
+defaults pref(dom.fs.enabled,true)
+
+load 1798773.html

dom/fs/test/gtest/api/TestFileSystemDirectoryHandle.cpp

@@ -29,6 +29,12 @@ class TestFileSystemDirectoryHandle : public ::testing::Test {
     mManager = MakeAndAddRef<FileSystemManager>(mGlobal, nullptr);
   }
 
+  void TearDown() override {
+    if (!mManager->IsShutdown()) {
+      mManager->Shutdown();
+    }
+  }
+
   nsIGlobalObject* mGlobal = GetGlobal();
   const IterableIteratorBase::IteratorType mIteratorType =
       IterableIteratorBase::IteratorType::Keys;

dom/fs/test/gtest/api/TestFileSystemFileHandle.cpp

@@ -28,6 +28,12 @@ class TestFileSystemFileHandle : public ::testing::Test {
     mManager = MakeAndAddRef<FileSystemManager>(mGlobal, nullptr);
   }
 
+  void TearDown() override {
+    if (!mManager->IsShutdown()) {
+      mManager->Shutdown();
+    }
+  }
+
   nsIGlobalObject* mGlobal = GetGlobal();
   UniquePtr<MockFileSystemRequestHandler> mRequestHandler;
   FileSystemEntryMetadata mMetadata;

dom/fs/test/gtest/api/TestFileSystemHandle.cpp

@@ -27,6 +27,12 @@ class TestFileSystemHandle : public ::testing::Test {
     mManager = MakeAndAddRef<FileSystemManager>(mGlobal, nullptr);
   }
 
+  void TearDown() override {
+    if (!mManager->IsShutdown()) {
+      mManager->Shutdown();
+    }
+  }
+
   nsIGlobalObject* mGlobal = GetGlobal();
   FileSystemEntryMetadata mDirMetadata;
   FileSystemEntryMetadata mFileMetadata;

dom/fs/test/gtest/child/TestFileSystemRequestHandler.cpp

@@ -32,6 +32,18 @@ namespace mozilla::dom::fs::test {
 
 class TestFileSystemRequestHandler : public ::testing::Test {
  protected:
+  struct FileSystemManagerChildShutdown {
+    explicit FileSystemManagerChildShutdown(
+        TestFileSystemManagerChild* aFileSystemManagerChild)
+        : mFileSystemManagerChild(aFileSystemManagerChild) {}
+
+    void operator()() const {
+      mFileSystemManagerChild->FileSystemManagerChild::Shutdown();
+    }
+
+    TestFileSystemManagerChild* mFileSystemManagerChild;
+  };
+
   void SetUp() override {
     mListener = MakeAndAddRef<ExpectResolveCalled>();
 
@@ -46,6 +58,18 @@ class TestFileSystemRequestHandler : public ::testing::Test {
             mFileSystemManagerChild));
   }
 
+  void TearDown() override {
+    if (!mManager->IsShutdown()) {
+      EXPECT_CALL(*mFileSystemManagerChild, Shutdown())
+          .WillOnce(FileSystemManagerChildShutdown(mFileSystemManagerChild));
+
+      mManager->Shutdown();
+    }
+
+    EXPECT_CALL(*mFileSystemManagerChild, Shutdown())
+        .WillOnce(FileSystemManagerChildShutdown(mFileSystemManagerChild));
+  }
+
   already_AddRefed<Promise> GetDefaultPromise() {
     IgnoredErrorResult rv;
     RefPtr<Promise> result = Promise::Create(mGlobal, rv);
@@ -86,12 +110,6 @@ TEST_F(TestFileSystemRequestHandler, isGetRootHandleSuccessful) {
   EXPECT_CALL(mListener->GetSuccessHandler(), InvokeMe());
   EXPECT_CALL(*mFileSystemManagerChild, SendGetRootHandle(_, _))
       .WillOnce(Invoke(fakeResponse));
-  EXPECT_CALL(*mFileSystemManagerChild, Shutdown())
-      .WillOnce([fileSystemManagerChild =
-                     static_cast<void*>(mFileSystemManagerChild.get())]() {
-        static_cast<TestFileSystemManagerChild*>(fileSystemManagerChild)
-            ->FileSystemManagerChild::Shutdown();
-      });
 
   RefPtr<Promise> promise = GetDefaultPromise();
   auto testable = GetFileSystemRequestHandler();
@@ -101,6 +119,9 @@ TEST_F(TestFileSystemRequestHandler, isGetRootHandleSuccessful) {
 }
 
 TEST_F(TestFileSystemRequestHandler, isGetRootHandleBlockedAfterShutdown) {
+  EXPECT_CALL(*mFileSystemManagerChild, Shutdown())
+      .WillOnce(FileSystemManagerChildShutdown(mFileSystemManagerChild));
+
   mManager->Shutdown();
 
   IgnoredErrorResult error;
@@ -122,12 +143,6 @@ TEST_F(TestFileSystemRequestHandler, isGetDirectoryHandleSuccessful) {
   EXPECT_CALL(mListener->GetSuccessHandler(), InvokeMe());
   EXPECT_CALL(*mFileSystemManagerChild, SendGetDirectoryHandle(_, _, _))
       .WillOnce(Invoke(fakeResponse));
-  EXPECT_CALL(*mFileSystemManagerChild, Shutdown())
-      .WillOnce([fileSystemManagerChild =
-                     static_cast<void*>(mFileSystemManagerChild.get())]() {
-        static_cast<TestFileSystemManagerChild*>(fileSystemManagerChild)
-            ->FileSystemManagerChild::Shutdown();
-      });
 
   RefPtr<Promise> promise = GetDefaultPromise();
   auto testable = GetFileSystemRequestHandler();
@@ -139,6 +154,9 @@ TEST_F(TestFileSystemRequestHandler, isGetDirectoryHandleSuccessful) {
 }
 
 TEST_F(TestFileSystemRequestHandler, isGetDirectoryHandleBlockedAfterShutdown) {
+  EXPECT_CALL(*mFileSystemManagerChild, Shutdown())
+      .WillOnce(FileSystemManagerChildShutdown(mFileSystemManagerChild));
+
   mManager->Shutdown();
 
   IgnoredErrorResult error;
@@ -160,12 +178,6 @@ TEST_F(TestFileSystemRequestHandler, isGetFileHandleSuccessful) {
   EXPECT_CALL(mListener->GetSuccessHandler(), InvokeMe());
   EXPECT_CALL(*mFileSystemManagerChild, SendGetFileHandle(_, _, _))
       .WillOnce(Invoke(fakeResponse));
-  EXPECT_CALL(*mFileSystemManagerChild, Shutdown())
-      .WillOnce([fileSystemManagerChild =
-                     static_cast<void*>(mFileSystemManagerChild.get())]() {
-        static_cast<TestFileSystemManagerChild*>(fileSystemManagerChild)
-            ->FileSystemManagerChild::Shutdown();
-      });
 
   RefPtr<Promise> promise = GetDefaultPromise();
   auto testable = GetFileSystemRequestHandler();
@@ -176,6 +188,9 @@ TEST_F(TestFileSystemRequestHandler, isGetFileHandleSuccessful) {
 }
 
 TEST_F(TestFileSystemRequestHandler, isGetFileHandleBlockedAfterShutdown) {
+  EXPECT_CALL(*mFileSystemManagerChild, Shutdown())
+      .WillOnce(FileSystemManagerChildShutdown(mFileSystemManagerChild));
+
   mManager->Shutdown();
 
   IgnoredErrorResult error;
@@ -220,12 +235,6 @@ TEST_F(TestFileSystemRequestHandler, isGetFileSuccessful) {
   EXPECT_CALL(mListener->GetSuccessHandler(), InvokeMe());
   EXPECT_CALL(*mFileSystemManagerChild, SendGetFile(_, _, _))
       .WillOnce(Invoke(fakeResponse));
-  EXPECT_CALL(*mFileSystemManagerChild, Shutdown())
-      .WillOnce([fileSystemManagerChild =
-                     static_cast<void*>(mFileSystemManagerChild.get())]() {
-        static_cast<TestFileSystemManagerChild*>(fileSystemManagerChild)
-            ->FileSystemManagerChild::Shutdown();
-      });
 
   RefPtr<Promise> promise = GetDefaultPromise();
   auto testable = GetFileSystemRequestHandler();
@@ -235,6 +244,9 @@ TEST_F(TestFileSystemRequestHandler, isGetFileSuccessful) {
 }
 
 TEST_F(TestFileSystemRequestHandler, isGetFileBlockedAfterShutdown) {
+  EXPECT_CALL(*mFileSystemManagerChild, Shutdown())
+      .WillOnce(FileSystemManagerChildShutdown(mFileSystemManagerChild));
+
   mManager->Shutdown();
 
   IgnoredErrorResult error;
@@ -246,6 +258,9 @@ TEST_F(TestFileSystemRequestHandler, isGetFileBlockedAfterShutdown) {
 }
 
 TEST_F(TestFileSystemRequestHandler, isGetAccessHandleBlockedAfterShutdown) {
+  EXPECT_CALL(*mFileSystemManagerChild, Shutdown())
+      .WillOnce(FileSystemManagerChildShutdown(mFileSystemManagerChild));
+
   mManager->Shutdown();
 
   IgnoredErrorResult error;
@@ -257,6 +272,9 @@ TEST_F(TestFileSystemRequestHandler, isGetAccessHandleBlockedAfterShutdown) {
 }
 
 TEST_F(TestFileSystemRequestHandler, isGetWritableBlockedAfterShutdown) {
+  EXPECT_CALL(*mFileSystemManagerChild, Shutdown())
+      .WillOnce(FileSystemManagerChildShutdown(mFileSystemManagerChild));
+
   mManager->Shutdown();
 
   IgnoredErrorResult error;
@@ -293,12 +311,6 @@ TEST_F(TestFileSystemRequestHandler, isGetEntriesSuccessful) {
 
   EXPECT_CALL(*mFileSystemManagerChild, SendGetEntries(_, _, _))
       .WillOnce(Invoke(fakeResponse));
-  EXPECT_CALL(*mFileSystemManagerChild, Shutdown())
-      .WillOnce([fileSystemManagerChild =
-                     static_cast<void*>(mFileSystemManagerChild.get())]() {
-        static_cast<TestFileSystemManagerChild*>(fileSystemManagerChild)
-            ->FileSystemManagerChild::Shutdown();
-      });
 
   auto testable = GetFileSystemRequestHandler();
   RefPtr<FileSystemEntryMetadataArray> sink;
@@ -310,6 +322,9 @@ TEST_F(TestFileSystemRequestHandler, isGetEntriesSuccessful) {
 }
 
 TEST_F(TestFileSystemRequestHandler, isGetEntriesBlockedAfterShutdown) {
+  EXPECT_CALL(*mFileSystemManagerChild, Shutdown())
+      .WillOnce(FileSystemManagerChildShutdown(mFileSystemManagerChild));
+
   mManager->Shutdown();
 
   RefPtr<FileSystemEntryMetadataArray> sink;
@@ -333,12 +348,6 @@ TEST_F(TestFileSystemRequestHandler, isRemoveEntrySuccessful) {
   EXPECT_CALL(mListener->GetSuccessHandler(), InvokeMe());
   EXPECT_CALL(*mFileSystemManagerChild, SendRemoveEntry(_, _, _))
       .WillOnce(Invoke(fakeResponse));
-  EXPECT_CALL(*mFileSystemManagerChild, Shutdown())
-      .WillOnce([fileSystemManagerChild =
-                     static_cast<void*>(mFileSystemManagerChild.get())]() {
-        static_cast<TestFileSystemManagerChild*>(fileSystemManagerChild)
-            ->FileSystemManagerChild::Shutdown();
-      });
 
   auto testable = GetFileSystemRequestHandler();
   RefPtr<Promise> promise = GetDefaultPromise();
@@ -349,6 +358,9 @@ TEST_F(TestFileSystemRequestHandler, isRemoveEntrySuccessful) {
 }
 
 TEST_F(TestFileSystemRequestHandler, isRemoveEntryBlockedAfterShutdown) {
+  EXPECT_CALL(*mFileSystemManagerChild, Shutdown())
+      .WillOnce(FileSystemManagerChildShutdown(mFileSystemManagerChild));
+
   mManager->Shutdown();
 
   IgnoredErrorResult error;

dom/workers/WorkerPrivate.cpp

@@ -4625,7 +4625,11 @@ bool WorkerPrivate::NotifyInternal(WorkerStatus aStatus) {
     if (aStatus >= Canceling) {
       MutexAutoUnlock unlock(mMutex);
       if (data->mScope) {
-        data->mScope->NoteTerminating();
+        if (aStatus == Canceling) {
+          data->mScope->NoteTerminating();
+        } else {
+          data->mScope->NoteShuttingDown();
+        }
       }
     }
 

dom/workers/WorkerScope.cpp

@@ -436,6 +436,10 @@ void WorkerGlobalScope::NoteTerminating() {
   }
 
   StartDying();
+}
+
+void WorkerGlobalScope::NoteShuttingDown() {
+  MOZ_ASSERT(IsDying());
 
   if (mNavigator) {
     mNavigator->Invalidate();

dom/workers/WorkerScope.h

@@ -214,6 +214,8 @@ class WorkerGlobalScope : public WorkerGlobalScopeBase {
 
   void NoteTerminating();
 
+  void NoteShuttingDown();
+
   // nsIGlobalObject implementation
   RefPtr<ServiceWorkerRegistration> GetServiceWorkerRegistration(
       const ServiceWorkerRegistrationDescriptor& aDescriptor) const final;

testing/crashtest/crashtests.list

@@ -15,6 +15,7 @@ include ../../dom/canvas/crashtests/crashtests.list
 include ../../dom/events/crashtests/crashtests.list
 include ../../dom/fetch/tests/crashtests/crashtests.list
 include ../../dom/file/tests/crashtests/crashtests.list
+include ../../dom/fs/test/crashtests/crashtests.list
 include ../../dom/html/crashtests/crashtests.list
 include ../../dom/indexedDB/crashtests/crashtests.list
 include ../../dom/jsurl/crashtests/crashtests.list

testing/web-platform/meta/fs/FileSystemBaseHandle-IndexedDB.https.any.js.ini

@@ -29,22 +29,27 @@
     if (os == "win") and not debug and (processor == "x86"): [OK, TIMEOUT]
   [Store handles and blobs in IndexedDB.]
     expected:
-      if (os == "win") and not debug and (processor == "x86"): [PASS, TIMEOUT, NOTRUN]
-      if (os == "win") and not debug and (processor == "x86_64"): [PASS, NOTRUN]
+      if (os == "win") and not debug and (processor == "x86"): [FAIL, TIMEOUT, NOTRUN]
+      if (os == "win") and not debug and (processor == "x86_64"): [FAIL, NOTRUN]
+      FAIL
 
   [Store handle in IndexedDB and read using a cursor.]
     expected:
-      if (os == "win") and not debug: [PASS, NOTRUN]
+      if (os == "win") and not debug: [FAIL, NOTRUN]
+      FAIL
 
   [Store handle in IndexedDB using inline keys.]
     expected:
-      if (os == "win") and not debug and (processor == "x86"): [PASS, TIMEOUT, NOTRUN]
-      if (os == "win") and not debug and (processor == "x86_64"): [PASS, TIMEOUT, NOTRUN]
+      if (os == "win") and not debug and (processor == "x86"): [FAIL, TIMEOUT, NOTRUN]
+      if (os == "win") and not debug and (processor == "x86_64"): [FAIL, TIMEOUT, NOTRUN]
+      FAIL
 
   [Store handle in IndexedDB and read from new transaction.]
     expected:
-      if (os == "win") and not debug and (processor == "x86_64"): [PASS, NOTRUN]
+      if (os == "win") and not debug and (processor == "x86_64"): [FAIL, NOTRUN]
+      FAIL
 
   [Store handle in IndexedDB and read from pending transaction.]
     expected:
-      if (os == "win") and not debug and (processor == "x86_64"): [PASS, TIMEOUT]
+      if (os == "win") and not debug and (processor == "x86_64"): [FAIL, TIMEOUT]
+      FAIL