Bug 1188265 - No manual JS_ClearPendingException when StructuredCloneHelper is used, r=smaug
authorAndrea Marchesini <amarchesini@mozilla.com>
Fri, 31 Jul 2015 01:38:00 +0100
changeset 287231 639c7d0805bfd1cd079144284b55d480521288d6
parent 287230 d88da1f3c511ac78aa777f218900ee38bb118b03
child 287232 3f05126269cbbd385f6aa43263a2637cc78d7427
push id5067
push userraliiev@mozilla.com
push dateMon, 21 Sep 2015 14:04:52 +0000
treeherdermozilla-beta@14221ffe5b2f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1188265
milestone42.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 1188265 - No manual JS_ClearPendingException when StructuredCloneHelper is used, r=smaug
dom/base/PostMessageEvent.cpp
dom/base/StructuredCloneHelper.cpp
dom/base/StructuredCloneHelper.h
dom/base/nsGlobalWindow.cpp
dom/base/test/test_postMessages.html
dom/broadcastchannel/BroadcastChannel.cpp
dom/broadcastchannel/BroadcastChannelChild.cpp
dom/messagechannel/SharedMessagePortMessage.cpp
dom/workers/DataStore.cpp
--- a/dom/base/PostMessageEvent.cpp
+++ b/dom/base/PostMessageEvent.cpp
@@ -89,20 +89,23 @@ PostMessageEvent::Run()
     //       don't do that in other places it seems better to hold the line for
     //       now.  Long-term, we want HTML5 to address this so that we can
     //       be compliant while being safer.
     if (!targetPrin->Equals(mProvidedPrincipal)) {
       return NS_OK;
     }
   }
 
+  ErrorResult rv;
   JS::Rooted<JS::Value> messageData(cx);
   nsCOMPtr<nsPIDOMWindow> window = targetWindow.get();
-  if (!Read(window, cx, &messageData)) {
-    return NS_ERROR_DOM_DATA_CLONE_ERR;
+
+  Read(window, cx, &messageData, rv);
+  if (NS_WARN_IF(rv.Failed())) {
+    return rv.StealNSResult();
   }
 
   // Create the event
   nsCOMPtr<mozilla::dom::EventTarget> eventTarget =
     do_QueryInterface(static_cast<nsPIDOMWindow*>(targetWindow.get()));
   nsRefPtr<MessageEvent> event =
     new MessageEvent(eventTarget, nullptr, nullptr);
 
--- a/dom/base/StructuredCloneHelper.cpp
+++ b/dom/base/StructuredCloneHelper.cpp
@@ -206,62 +206,74 @@ StructuredCloneHelper::StructuredCloneHe
   , mParent(nullptr)
 {}
 
 StructuredCloneHelper::~StructuredCloneHelper()
 {
   Shutdown();
 }
 
-bool
+void
 StructuredCloneHelper::Write(JSContext* aCx,
-                             JS::Handle<JS::Value> aValue)
+                             JS::Handle<JS::Value> aValue,
+                             ErrorResult& aRv)
 {
-  return Write(aCx, aValue, JS::UndefinedHandleValue);
+  Write(aCx, aValue, JS::UndefinedHandleValue, aRv);
 }
 
-bool
+void
 StructuredCloneHelper::Write(JSContext* aCx,
                              JS::Handle<JS::Value> aValue,
-                             JS::Handle<JS::Value> aTransfer)
+                             JS::Handle<JS::Value> aTransfer,
+                             ErrorResult& aRv)
 {
-  bool ok = StructuredCloneHelperInternal::Write(aCx, aValue, aTransfer);
+  if (!StructuredCloneHelperInternal::Write(aCx, aValue, aTransfer)) {
+    aRv.Throw(NS_ERROR_DOM_DATA_CLONE_ERR);
+  }
+
   mTransferringPort.Clear();
-  return ok;
 }
 
-bool
+void
 StructuredCloneHelper::Read(nsISupports* aParent,
                             JSContext* aCx,
-                            JS::MutableHandle<JS::Value> aValue)
+                            JS::MutableHandle<JS::Value> aValue,
+                            ErrorResult& aRv)
 {
   mozilla::AutoRestore<nsISupports*> guard(mParent);
   mParent = aParent;
 
-  bool ok = StructuredCloneHelperInternal::Read(aCx, aValue);
+  if (!StructuredCloneHelperInternal::Read(aCx, aValue)) {
+    JS_ClearPendingException(aCx);
+    aRv.Throw(NS_ERROR_DOM_DATA_CLONE_ERR);
+  }
+
   mBlobImplArray.Clear();
-  return ok;
 }
 
-bool
+void
 StructuredCloneHelper::ReadFromBuffer(nsISupports* aParent,
                                       JSContext* aCx,
                                       uint64_t* aBuffer,
                                       size_t aBufferLength,
-                                      JS::MutableHandle<JS::Value> aValue)
+                                      JS::MutableHandle<JS::Value> aValue,
+                                      ErrorResult& aRv)
 {
   MOZ_ASSERT(!mBuffer, "ReadFromBuffer() must be called without a Write().");
   MOZ_ASSERT(aBuffer);
 
   mozilla::AutoRestore<nsISupports*> guard(mParent);
   mParent = aParent;
 
-  return JS_ReadStructuredClone(aCx, aBuffer, aBufferLength,
-                                JS_STRUCTURED_CLONE_VERSION, aValue,
-                                &gCallbacks, this);
+  if (!JS_ReadStructuredClone(aCx, aBuffer, aBufferLength,
+                              JS_STRUCTURED_CLONE_VERSION, aValue,
+                              &gCallbacks, this)) {
+    JS_ClearPendingException(aCx);
+    aRv.Throw(NS_ERROR_DOM_DATA_CLONE_ERR);
+  }
 }
 
 void
 StructuredCloneHelper::MoveBufferDataToArray(FallibleTArray<uint8_t>& aArray,
                                              ErrorResult& aRv)
 {
   MOZ_ASSERT(mBuffer, "MoveBuffer() cannot be called without a Write().");
 
--- a/dom/base/StructuredCloneHelper.h
+++ b/dom/base/StructuredCloneHelper.h
@@ -120,41 +120,45 @@ public:
   // If transferring is supported, we will transfer MessagePorts and in the
   // future other transferrable objects.
   explicit StructuredCloneHelper(CloningSupport aSupportsCloning,
                                  TransferringSupport aSupportsTransferring);
   virtual ~StructuredCloneHelper();
 
   // Normally you should just use Write() and Read().
 
-  bool Write(JSContext* aCx,
-             JS::Handle<JS::Value> aValue);
+  void Write(JSContext* aCx,
+             JS::Handle<JS::Value> aValue,
+             ErrorResult &aRv);
 
-  bool Write(JSContext* aCx,
+  void Write(JSContext* aCx,
              JS::Handle<JS::Value> aValue,
-             JS::Handle<JS::Value> aTransfer);
+             JS::Handle<JS::Value> aTransfer,
+             ErrorResult &aRv);
 
-  bool Read(nsISupports* aParent,
+  void Read(nsISupports* aParent,
             JSContext* aCx,
-            JS::MutableHandle<JS::Value> aValue);
+            JS::MutableHandle<JS::Value> aValue,
+            ErrorResult &aRv);
 
   // Sometimes, when IPC is involved, you must send a buffer after a Write().
   // This method 'steals' the internal data from this helper class.
   // You should free this buffer with FreeBuffer().
   void MoveBufferDataToArray(FallibleTArray<uint8_t>& aArray,
                              ErrorResult& aRv);
 
   // If you receive a buffer from IPC, you can use this method to retrieve a
   // JS::Value. It can happen that you want to pre-populate the array of Blobs
   // and/or the PortIdentifiers.
-  bool ReadFromBuffer(nsISupports* aParent,
+  void ReadFromBuffer(nsISupports* aParent,
                       JSContext* aCx,
                       uint64_t* aBuffer,
                       size_t aBufferLength,
-                      JS::MutableHandle<JS::Value> aValue);
+                      JS::MutableHandle<JS::Value> aValue,
+                      ErrorResult &aRv);
 
   // Use this method to free a buffer generated by MoveToBuffer().
   void FreeBuffer(uint64_t* aBuffer,
                   size_t aBufferLength);
 
   nsTArray<nsRefPtr<BlobImpl>>& BlobImpls()
   {
     MOZ_ASSERT(mSupportsCloning, "Blobs cannot be taken/set if cloning is not supported.");
--- a/dom/base/nsGlobalWindow.cpp
+++ b/dom/base/nsGlobalWindow.cpp
@@ -8596,18 +8596,18 @@ nsGlobalWindow::PostMessageMozOuter(JSCo
                          origin,
                          this,
                          providedPrincipal,
                          nsContentUtils::IsCallerChrome());
 
   JS::Rooted<JS::Value> message(aCx, aMessage);
   JS::Rooted<JS::Value> transfer(aCx, aTransfer);
 
-  if (!event->Write(aCx, message, transfer)) {
-    aError.Throw(NS_ERROR_DOM_DATA_CLONE_ERR);
+  event->Write(aCx, message, transfer, aError);
+  if (NS_WARN_IF(aError.Failed())) {
     return;
   }
 
   aError = NS_DispatchToCurrentThread(event);
 }
 
 void
 nsGlobalWindow::PostMessageMoz(JSContext* aCx, JS::Handle<JS::Value> aMessage,
--- a/dom/base/test/test_postMessages.html
+++ b/dom/base/test/test_postMessages.html
@@ -137,16 +137,34 @@ function runTests(obj) {
         received.ports[0].onmessage = function(e) {
           ok(e.data, "hello world", "Ports are connected!");
           r();
         }
       });
     });
   })
 
+  // non transfering tests
+  .then(function() {
+    if (obj.transferableObjects) {
+      return;
+    }
+
+    // MessagePort
+    return new Promise(function(r, rr) {
+      var mc = new MessageChannel();
+      obj.send(42, [mc.port1]).then(function(received) {
+        ok(false, "This object should not support port transferring");
+      }, function() {
+        ok(true, "This object should not support port transferring");
+      })
+      .then(r);
+    });
+  })
+
   // done.
   .then(function() {
     obj.finished();
   });
 }
 
 // PostMessage to the same window.
 function test_windowToWindow() {
@@ -165,17 +183,23 @@ function test_windowToWindow() {
   }
 
   runTests({
     clonableObjects: true,
     transferableObjects: true,
     send: function(what, ports) {
       return new Promise(function(r, rr) {
         resolve = r;
-        postMessage(what, '*', ports);
+
+        try {
+          postMessage(what, '*', ports);
+        } catch(e) {
+          resolve = null;
+          rr();
+        }
       });
     },
 
     finished: function() {
       onmessage = null;
       next();
     }
   });
@@ -212,17 +236,22 @@ function test_windowToIframeURL(url) {
   ifr.src = url;
   ifr.onload = function() {
     runTests({
       clonableObjects: true,
       transferableObjects: true,
       send: function(what, ports) {
         return new Promise(function(r, rr) {
           resolve = r;
-          ifr.contentWindow.postMessage(what, '*', ports);
+          try {
+            ifr.contentWindow.postMessage(what, '*', ports);
+          } catch(e) {
+            resolve = null;
+            rr();
+          }
         });
       },
 
       finished: function() {
         document.body.removeChild(ifr);
         onmessage = null;
         next();
       }
@@ -250,18 +279,22 @@ function test_broadcastChannel() {
     resolve = null;
     tmp({ data: e.data, ports: [] });
   }
 
   runTests({
     clonableObjects: true,
     transferableObjects: false,
     send: function(what, ports) {
-      is(ports.length, 0, "No ports for this test!");
       return new Promise(function(r, rr) {
+        if (ports.length) {
+          rr();
+          return;
+        }
+
         resolve = r;
         bc1.postMessage(what);
       });
     },
 
     finished: function() {
       onmessage = null;
       next();
@@ -288,17 +321,22 @@ function test_messagePort() {
   }
 
   runTests({
     clonableObjects: true,
     transferableObjects: true,
     send: function(what, ports) {
       return new Promise(function(r, rr) {
         resolve = r;
-        mc.port1.postMessage(what, ports);
+        try {
+          mc.port1.postMessage(what, ports);
+        } catch(e) {
+          resolve = null;
+          rr();
+        }
       });
     },
 
     finished: function() {
       onmessage = null;
       next();
     }
   });
--- a/dom/broadcastchannel/BroadcastChannel.cpp
+++ b/dom/broadcastchannel/BroadcastChannel.cpp
@@ -449,18 +449,18 @@ BroadcastChannel::PostMessage(JSContext*
 
 void
 BroadcastChannel::PostMessageInternal(JSContext* aCx,
                                       JS::Handle<JS::Value> aMessage,
                                       ErrorResult& aRv)
 {
   nsRefPtr<BroadcastChannelMessage> data = new BroadcastChannelMessage();
 
-  if (!data->Write(aCx, aMessage)) {
-    aRv.Throw(NS_ERROR_DOM_DATA_CLONE_ERR);
+  data->Write(aCx, aMessage, aRv);
+  if (NS_WARN_IF(aRv.Failed())) {
     return;
   }
 
   const nsTArray<nsRefPtr<BlobImpl>>& blobImpls = data->BlobImpls();
   for (uint32_t i = 0, len = blobImpls.Length(); i < len; ++i) {
     if (!blobImpls[i]->MayBeClonedToOtherThreads()) {
       aRv.Throw(NS_ERROR_DOM_DATA_CLONE_ERR);
       return;
--- a/dom/broadcastchannel/BroadcastChannelChild.cpp
+++ b/dom/broadcastchannel/BroadcastChannelChild.cpp
@@ -88,21 +88,23 @@ BroadcastChannelChild::RecvNotify(const 
   JSContext* cx = jsapi.cx();
   const SerializedStructuredCloneBuffer& buffer = aData.data();
   StructuredCloneHelper cloneHelper(StructuredCloneHelper::CloningSupported,
                                     StructuredCloneHelper::TransferringNotSupported);
 
   cloneHelper.BlobImpls().AppendElements(blobs);
 
   JS::Rooted<JS::Value> value(cx, JS::NullValue());
-  if (buffer.dataLength &&
-      !cloneHelper.ReadFromBuffer(mBC->GetParentObject(), cx,
-                                  buffer.data, buffer.dataLength, &value)) {
-    JS_ClearPendingException(cx);
-    return false;
+  if (buffer.dataLength) {
+    ErrorResult rv;
+    cloneHelper.ReadFromBuffer(mBC->GetParentObject(), cx,
+                               buffer.data, buffer.dataLength, &value, rv);
+    if (NS_WARN_IF(rv.Failed())) {
+      return true;
+    }
   }
 
   RootedDictionary<MessageEventInit> init(cx);
   init.mBubbles = false;
   init.mCancelable = false;
   init.mOrigin.Construct(mOrigin);
   init.mData = value;
 
--- a/dom/messagechannel/SharedMessagePortMessage.cpp
+++ b/dom/messagechannel/SharedMessagePortMessage.cpp
@@ -29,33 +29,30 @@ SharedMessagePortMessage::Read(nsISuppor
   if (mData.IsEmpty()) {
     return;
   }
 
   auto* data = reinterpret_cast<uint64_t*>(mData.Elements());
   size_t dataLen = mData.Length();
   MOZ_ASSERT(!(dataLen % sizeof(*data)));
 
-  bool ok = ReadFromBuffer(aParent, aCx, data, dataLen, aValue);
-  Free();
+  ReadFromBuffer(aParent, aCx, data, dataLen, aValue, aRv);
+  NS_WARN_IF(aRv.Failed());
 
-  if (NS_WARN_IF(!ok)) {
-    aRv.Throw(NS_ERROR_FAILURE);
-    return;
-  }
+  Free();
 }
 
 void
 SharedMessagePortMessage::Write(JSContext* aCx,
                                 JS::Handle<JS::Value> aValue,
                                 JS::Handle<JS::Value> aTransfer,
                                 ErrorResult& aRv)
 {
-  if (NS_WARN_IF(!StructuredCloneHelper::Write(aCx, aValue, aTransfer))) {
-    aRv.Throw(NS_ERROR_DOM_DATA_CLONE_ERR);
+  StructuredCloneHelper::Write(aCx, aValue, aTransfer, aRv);
+  if (NS_WARN_IF(aRv.Failed())) {
     return;
   }
 
   const nsTArray<nsRefPtr<BlobImpl>>& blobImpls = BlobImpls();
   for (uint32_t i = 0, len = blobImpls.Length(); i < len; ++i) {
     if (!blobImpls[i]->MayBeClonedToOtherThreads()) {
       aRv.Throw(NS_ERROR_DOM_DATA_CLONE_ERR);
       return;
--- a/dom/workers/DataStore.cpp
+++ b/dom/workers/DataStore.cpp
@@ -228,20 +228,18 @@ public:
     , mId(aId)
     , mRevisionId(aRevisionId)
     , mRv(aRv)
   {
     MOZ_ASSERT(aWorkerPrivate);
     aWorkerPrivate->AssertIsOnWorkerThread();
 
     // This needs to be structured cloned while it's still on the worker thread.
-    if (!Write(aCx, aObj)) {
-      JS_ClearPendingException(aCx);
-      mRv.Throw(NS_ERROR_DOM_DATA_CLONE_ERR);
-    }
+    Write(aCx, aObj, mRv);
+    NS_WARN_IF(mRv.Failed());
   }
 
 protected:
   virtual bool
   MainThreadRun() override
   {
     AssertIsOnMainThread();
 
@@ -249,19 +247,18 @@ protected:
     AutoJSAPI jsapi;
     if (NS_WARN_IF(!jsapi.Init(mBackingStore->GetParentObject()))) {
       mRv.Throw(NS_ERROR_UNEXPECTED);
       return true;
     }
     JSContext* cx = jsapi.cx();
 
     JS::Rooted<JS::Value> value(cx);
-    if (!Read(mBackingStore->GetParentObject(), cx, &value)) {
-      JS_ClearPendingException(cx);
-      mRv.Throw(NS_ERROR_DOM_DATA_CLONE_ERR);
+    Read(mBackingStore->GetParentObject(), cx, &value, mRv);
+    if (NS_WARN_IF(mRv.Failed())) {
       return true;
     }
 
     nsRefPtr<Promise> promise = mBackingStore->Put(cx,
                                                    value,
                                                    mId,
                                                    mRevisionId,
                                                    mRv);
@@ -292,20 +289,18 @@ public:
     , mId(aId)
     , mRevisionId(aRevisionId)
     , mRv(aRv)
   {
     MOZ_ASSERT(aWorkerPrivate);
     aWorkerPrivate->AssertIsOnWorkerThread();
 
     // This needs to be structured cloned while it's still on the worker thread.
-    if (!Write(aCx, aObj)) {
-      JS_ClearPendingException(aCx);
-      mRv.Throw(NS_ERROR_DOM_DATA_CLONE_ERR);
-    }
+    Write(aCx, aObj, mRv);
+    NS_WARN_IF(mRv.Failed());
   }
 
 protected:
   virtual bool
   MainThreadRun() override
   {
     AssertIsOnMainThread();
 
@@ -313,19 +308,18 @@ protected:
     AutoJSAPI jsapi;
     if (NS_WARN_IF(!jsapi.Init(mBackingStore->GetParentObject()))) {
       mRv.Throw(NS_ERROR_UNEXPECTED);
       return true;
     }
     JSContext* cx = jsapi.cx();
 
     JS::Rooted<JS::Value> value(cx);
-    if (!Read(mBackingStore->GetParentObject(), cx, &value)) {
-      JS_ClearPendingException(cx);
-      mRv.Throw(NS_ERROR_DOM_DATA_CLONE_ERR);
+    Read(mBackingStore->GetParentObject(), cx, &value, mRv);
+    if (NS_WARN_IF(mRv.Failed())) {
       return true;
     }
 
     nsRefPtr<Promise> promise = mBackingStore->Add(cx,
                                                    value,
                                                    mId,
                                                    mRevisionId,
                                                    mRv);