Bug 1800470, make it possible to extend WritableStream with another c…

…ycle collectable class, r=saschanaz Differential Revision: https://phabricator.services.mozilla.com/D162287
dothq · Nov 17, 2022 · 7042f24 · 7042f24
1 parent caf1ed9
commit 7042f24
Show file tree

Hide file tree

Showing 7 changed files with 72 additions and 13 deletions.
diff --git a/dom/fs/api/FileSystemWritableFileStream.cpp b/dom/fs/api/FileSystemWritableFileStream.cpp
@@ -116,7 +116,7 @@ FileSystemWritableFileStream::FileSystemWritableFileStream(
     RefPtr<FileSystemWritableFileStreamChild> aActor,
     const ::mozilla::ipc::FileDescriptor& aFileDescriptor,
     const fs::FileSystemEntryMetadata& aMetadata)
-    : WritableStream(aGlobal),
+    : WritableStream(aGlobal, HoldDropJSObjectsCaller::Explicit),
       mManager(aManager),
       mActor(std::move(aActor)),
       mFileDesc(nullptr),
@@ -125,12 +125,16 @@ FileSystemWritableFileStream::FileSystemWritableFileStream(
   auto rawFD = aFileDescriptor.ClonePlatformHandle();
   mFileDesc = PR_ImportFile(PROsfd(rawFD.release()));
 
+  mozilla::HoldJSObjects(this);
+
   LOG(("Created WritableFileStream %p for fd %p", this, mFileDesc));
 }
 
 FileSystemWritableFileStream::~FileSystemWritableFileStream() {
   MOZ_ASSERT(!mActor);
   MOZ_ASSERT(mClosed);
+
+  mozilla::DropJSObjects(this);
 }
 
 // https://streams.spec.whatwg.org/#writablestream-set-up

diff --git a/dom/fs/test/crashtests/1800470.html b/dom/fs/test/crashtests/1800470.html
@@ -0,0 +1,27 @@
+<!DOCTYPE html>
+<html class="reftest-wait">
+<head>
+  <script id="worker1" type="javascript/worker">
+    self.onmessage = async function (e) {
+      const directory = await navigator.storage.getDirectory()
+      const file = await directory.getFileHandle("500014c3-f683-4551-bb26-08025c9be332", {
+        create: true,
+      });
+      const stream = await file.createWritable({});
+      const regex = new RegExp(".*");
+      await stream.abort(regex);
+      self.postMessage("done");
+      self.close();
+    }
+  </script>
+  <script>
+    document.addEventListener('DOMContentLoaded', () => {
+      const buffer = new ArrayBuffer(1);
+      const blob = new Blob([document.querySelector('#worker1').textContent], { type: 'text/javascript' });
+      const worker = new Worker(window.URL.createObjectURL(blob));
+      worker.postMessage([buffer], [buffer]);
+      worker.onmessage = function() {document.documentElement.removeAttribute("class"); }
+    });
+  </script>
+</head>
+</html>
diff --git a/dom/fs/test/crashtests/crashtests.list b/dom/fs/test/crashtests/crashtests.list
@@ -0,0 +1,2 @@
+# StorageManager isn't enabled on Android
+skip-if(Android) pref(dom.fs.enabled,true) load 1800470.html
diff --git a/dom/streams/Transferable.cpp b/dom/streams/Transferable.cpp
@@ -833,7 +833,8 @@ bool ReadableStream::Transfer(JSContext* aCx, UniqueMessagePortId& aPortId) {
   }
 
   // Step 5: Let writable be a new WritableStream in the current Realm.
-  RefPtr<WritableStream> writable = new WritableStream(mGlobal);
+  RefPtr<WritableStream> writable = new WritableStream(
+      mGlobal, WritableStream::HoldDropJSObjectsCaller::Implicit);
 
   // Step 6: Perform ! SetUpCrossRealmTransformWritable(writable, port1).
   // MOZ_KnownLive because Port1 never changes before CC
@@ -957,7 +958,8 @@ WritableStreamTransferReceivingStepsImpl(JSContext* aCx,
   // Step 2: Let port be a deserializedRecord.[[Deserialized]].
 
   // Step 3: Perform ! SetUpCrossRealmTransformWritable(value, port).
-  auto writable = MakeRefPtr<WritableStream>(aGlobal);
+  auto writable = MakeRefPtr<WritableStream>(
+      aGlobal, WritableStream::HoldDropJSObjectsCaller::Implicit);
   ErrorResult rv;
   SetUpCrossRealmTransformWritable(writable, &aPort, rv);
   if (rv.MaybeSetPendingException(aCx)) {

diff --git a/dom/streams/WritableStream.cpp b/dom/streams/WritableStream.cpp
@@ -49,16 +49,28 @@ NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(WritableStream)
   NS_INTERFACE_MAP_ENTRY(nsISupports)
 NS_INTERFACE_MAP_END
 
-WritableStream::WritableStream(nsIGlobalObject* aGlobal) : mGlobal(aGlobal) {
-  mozilla::HoldJSObjects(this);
+WritableStream::WritableStream(nsIGlobalObject* aGlobal,
+                               HoldDropJSObjectsCaller aHoldDropCaller)
+    : mGlobal(aGlobal), mHoldDropCaller(aHoldDropCaller) {
+  if (mHoldDropCaller == HoldDropJSObjectsCaller::Implicit) {
+    mozilla::HoldJSObjects(this);
+  }
 }
 
-WritableStream::WritableStream(const GlobalObject& aGlobal)
-    : mGlobal(do_QueryInterface(aGlobal.GetAsSupports())) {
-  mozilla::HoldJSObjects(this);
+WritableStream::WritableStream(const GlobalObject& aGlobal,
+                               HoldDropJSObjectsCaller aHoldDropCaller)
+    : mGlobal(do_QueryInterface(aGlobal.GetAsSupports())),
+      mHoldDropCaller(aHoldDropCaller) {
+  if (mHoldDropCaller == HoldDropJSObjectsCaller::Implicit) {
+    mozilla::HoldJSObjects(this);
+  }
 }
 
-WritableStream::~WritableStream() { mozilla::DropJSObjects(this); }
+WritableStream::~WritableStream() {
+  if (mHoldDropCaller == HoldDropJSObjectsCaller::Implicit) {
+    mozilla::DropJSObjects(this);
+  }
+}
 
 JSObject* WritableStream::WrapObject(JSContext* aCx,
                                      JS::Handle<JSObject*> aGivenProto) {
@@ -458,7 +470,8 @@ already_AddRefed<WritableStream> WritableStream::Constructor(
   }
 
   // Step 4. Perform ! InitializeWritableStream(this).
-  RefPtr<WritableStream> writableStream = new WritableStream(aGlobal);
+  RefPtr<WritableStream> writableStream =
+      new WritableStream(aGlobal, HoldDropJSObjectsCaller::Implicit);
 
   // Step 5. Let sizeAlgorithm be ! ExtractSizeAlgorithm(strategy).
   //
@@ -693,7 +706,8 @@ already_AddRefed<WritableStream> CreateWritableStream(
 
   // Step 2: Let stream be a new WritableStream.
   // Step 3: Perform ! InitializeWritableStream(stream).
-  auto stream = MakeRefPtr<WritableStream>(aGlobal);
+  auto stream = MakeRefPtr<WritableStream>(
+      aGlobal, WritableStream::HoldDropJSObjectsCaller::Implicit);
 
   // Step 4: Let controller be a new WritableStreamDefaultController.
   auto controller =

diff --git a/dom/streams/WritableStream.h b/dom/streams/WritableStream.h
@@ -39,8 +39,16 @@ class WritableStream : public nsISupports, public nsWrapperCache {
   virtual void LastRelease() {}
 
  public:
-  explicit WritableStream(const GlobalObject& aGlobal);
-  explicit WritableStream(nsIGlobalObject* aGlobal);
+  // If one extends WritableStream with another cycle collectable class,
+  // calling HoldJSObjects and DropJSObjects should happen using 'this' of
+  // that extending class. And in that case Explicit should be passed to the
+  // constructor of WriteableStream so that it doesn't make those calls.
+  // See also https://bugzilla.mozilla.org/show_bug.cgi?id=1801214.
+  enum class HoldDropJSObjectsCaller { Implicit, Explicit };
+  explicit WritableStream(const GlobalObject& aGlobal,
+                          HoldDropJSObjectsCaller aHoldDropCaller);
+  explicit WritableStream(nsIGlobalObject* aGlobal,
+                          HoldDropJSObjectsCaller aHoldDropCaller);
 
   enum class WriterState { Writable, Closed, Erroring, Errored };
 
@@ -188,6 +196,7 @@ class WritableStream : public nsISupports, public nsWrapperCache {
   nsTArray<RefPtr<Promise>> mWriteRequests;
 
   nsCOMPtr<nsIGlobalObject> mGlobal;
+  HoldDropJSObjectsCaller mHoldDropCaller;
 };
 
 MOZ_CAN_RUN_SCRIPT already_AddRefed<WritableStream> CreateWritableStream(

diff --git a/testing/crashtest/crashtests.list b/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
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		# StorageManager isn't enabled on Android
		skip-if(Android) pref(dom.fs.enabled,true) load 1800470.html