Bug 1128959 - Implement the WHATWG Streams spec - part 17 - Creating FetchStream as a out param in order to avoid JS hazards, r=bz
authorAndrea Marchesini <amarchesini@mozilla.com>
Thu, 10 Aug 2017 18:04:56 -0700
changeset 374218 e5a9999d401ada37fd105bdf66219f038166e18e
parent 374217 7fe06385f5ed97cc4610d2993f289e0e9e5f43ee
child 374219 d33b4593b4452413d7bc2c8ccbf7b873b2bece70
push id32318
push userkwierso@gmail.com
push dateFri, 11 Aug 2017 20:16:01 +0000
treeherdermozilla-central@80ff3f300e05 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbz
bugs1128959
milestone57.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1128959 - Implement the WHATWG Streams spec - part 17 - Creating FetchStream as a out param in order to avoid JS hazards, r=bz
dom/fetch/Fetch.cpp
dom/fetch/FetchStream.cpp
dom/fetch/FetchStream.h
--- a/dom/fetch/Fetch.cpp
+++ b/dom/fetch/Fetch.cpp
@@ -1111,22 +1111,19 @@ FetchBody<Derived>::GetBody(JSContext* a
   nsCOMPtr<nsIInputStream> inputStream;
   DerivedClass()->GetBody(getter_AddRefs(inputStream));
 
   if (!inputStream) {
     aBodyOut.set(nullptr);
     return;
   }
 
-  JS::Rooted<JSObject*> body(aCx,
-                             FetchStream::Create(aCx,
-                                                 this,
-                                                 DerivedClass()->GetParentObject(),
-                                                 inputStream,
-                                                 aRv));
+  JS::Rooted<JSObject*> body(aCx);
+  FetchStream::Create(aCx, this, DerivedClass()->GetParentObject(),
+                      inputStream, &body, aRv);
   if (NS_WARN_IF(aRv.Failed())) {
     return;
   }
 
   MOZ_ASSERT(body);
 
   // If the body has been already consumed, we lock the stream.
   if (BodyUsed()) {
--- a/dom/fetch/FetchStream.cpp
+++ b/dom/fetch/FetchStream.cpp
@@ -96,49 +96,49 @@ private:
   RefPtr<FetchStreamHolder> mStreamHolder;
 };
 
 } // anonymous
 
 NS_IMPL_ISUPPORTS(FetchStream, nsIInputStreamCallback, nsIObserver,
                   nsISupportsWeakReference)
 
-/* static */ JSObject*
+/* static */ void
 FetchStream::Create(JSContext* aCx, FetchStreamHolder* aStreamHolder,
                     nsIGlobalObject* aGlobal, nsIInputStream* aInputStream,
-                    ErrorResult& aRv)
+                    JS::MutableHandle<JSObject*> aStream, ErrorResult& aRv)
 {
   MOZ_DIAGNOSTIC_ASSERT(aCx);
   MOZ_DIAGNOSTIC_ASSERT(aStreamHolder);
   MOZ_DIAGNOSTIC_ASSERT(aInputStream);
 
   RefPtr<FetchStream> stream =
     new FetchStream(aGlobal, aStreamHolder, aInputStream);
 
   if (NS_IsMainThread()) {
     nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
     if (NS_WARN_IF(!os)) {
       aRv.Throw(NS_ERROR_FAILURE);
-      return nullptr;
+      return;
     }
 
     aRv = os->AddObserver(stream, DOM_WINDOW_DESTROYED_TOPIC, true);
     if (NS_WARN_IF(aRv.Failed())) {
-      return nullptr;
+      return;
     }
 
   } else {
     WorkerPrivate* workerPrivate = GetWorkerPrivateFromContext(aCx);
     MOZ_ASSERT(workerPrivate);
 
     UniquePtr<FetchStreamWorkerHolder> holder(
       new FetchStreamWorkerHolder(stream));
     if (NS_WARN_IF(!holder->HoldWorker(workerPrivate, Closing))) {
       aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
-      return nullptr;
+      return;
     }
 
     // Note, this will create a ref-cycle between the holder and the stream.
     // The cycle is broken when the stream is closed or the worker begins
     // shutting down.
     stream->mWorkerHolder = Move(holder);
   }
 
@@ -151,24 +151,27 @@ FetchStream::Create(JSContext* aCx, Fetc
                                    &FetchStream::ErroredCallback,
                                    &FetchStream::FinalizeCallback);
   }
 
   JS::Rooted<JSObject*> body(aCx,
     JS::NewReadableExternalSourceStreamObject(aCx, stream, FETCH_STREAM_FLAG));
   if (!body) {
     aRv.StealExceptionFromJSContext(aCx);
-    return nullptr;
+    return;
   }
 
   stream->mReadableStream = body;
 
-  // JS engine will call the finalize callback.
+  // This will be released in FetchStream::FinalizeCallback().  We are
+  // guaranteed the jsapi will call FinalizeCallback when ReadableStream
+  // js object is finalized.
   NS_ADDREF(stream.get());
-  return body;
+
+  aStream.set(body);
 }
 
 /* static */ void
 FetchStream::RequestDataCallback(JSContext* aCx,
                                  JS::HandleObject aStream,
                                  void* aUnderlyingSource,
                                  uint8_t aFlags,
                                  size_t aDesiredSize)
@@ -302,17 +305,21 @@ FetchStream::WriteIntoReadRequestCallbac
 /* static */ JS::Value
 FetchStream::CancelCallback(JSContext* aCx, JS::HandleObject aStream,
                             void* aUnderlyingSource, uint8_t aFlags,
                             JS::HandleValue aReason)
 {
   MOZ_DIAGNOSTIC_ASSERT(aUnderlyingSource);
   MOZ_DIAGNOSTIC_ASSERT(aFlags == FETCH_STREAM_FLAG);
 
-  RefPtr<FetchStream> stream = static_cast<FetchStream*>(aUnderlyingSource);
+  // This is safe because we created an extra reference in FetchStream::Create()
+  // that won't be released until FetchStream::FinalizeCallback() is called.
+  // We are guaranteed that won't happen until the js ReadableStream object
+  // is finalized.
+  FetchStream* stream = static_cast<FetchStream*>(aUnderlyingSource);
 
   if (stream->mInputStream) {
     stream->mInputStream->CloseWithStatus(NS_BASE_STREAM_CLOSED);
   }
 
   stream->ReleaseObjects();
   return JS::UndefinedValue();
 }
@@ -337,16 +344,17 @@ FetchStream::ErroredCallback(JSContext* 
 void
 FetchStream::FinalizeCallback(void* aUnderlyingSource, uint8_t aFlags)
 {
   MOZ_DIAGNOSTIC_ASSERT(aUnderlyingSource);
   MOZ_DIAGNOSTIC_ASSERT(aFlags == FETCH_STREAM_FLAG);
 
   // This can be called in any thread.
 
+  // This takes ownership of the ref created in FetchStream::Create().
   RefPtr<FetchStream> stream =
     dont_AddRef(static_cast<FetchStream*>(aUnderlyingSource));
 
   stream->ReleaseObjects();
 }
 
 FetchStream::FetchStream(nsIGlobalObject* aGlobal,
                          FetchStreamHolder* aStreamHolder,
--- a/dom/fetch/FetchStream.h
+++ b/dom/fetch/FetchStream.h
@@ -31,20 +31,20 @@ class FetchStream final : public nsIInpu
                         , public nsIObserver
                         , public nsSupportsWeakReference
 {
 public:
   NS_DECL_THREADSAFE_ISUPPORTS
   NS_DECL_NSIINPUTSTREAMCALLBACK
   NS_DECL_NSIOBSERVER
 
-  static JSObject*
+  static void
   Create(JSContext* aCx, FetchStreamHolder* aStreamHolder,
          nsIGlobalObject* aGlobal, nsIInputStream* aInputStream,
-         ErrorResult& aRv);
+         JS::MutableHandle<JSObject*> aStream, ErrorResult& aRv);
 
   void
   Close();
 
   static nsresult
   RetrieveInputStream(void* aUnderlyingReadableStreamSource,
                       nsIInputStream** aInputStream);