Bug 1541557: Part 2 - Stop sending uninitialized StructuredCloneData objects. r=nika
☠☠ backed out by f9bf5e4b0b4f ☠ ☠
authorKris Maglione <maglione.k@gmail.com>
Thu, 13 Jun 2019 15:01:05 -0700
changeset 540536 158a4000c44b9b17a7935340db79431d544fb556
parent 540535 61fa2745733f3631488a3ecccc144823683b7b6d
child 540537 b4ed40bea2698802ef562a0931c0b560737fb89d
push id11529
push userarchaeopteryx@coole-files.de
push dateThu, 04 Jul 2019 15:22:33 +0000
treeherdermozilla-beta@ebb510a784b8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnika
bugs1541557
milestone69.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 1541557: Part 2 - Stop sending uninitialized StructuredCloneData objects. r=nika There are a couple of places where JSWindowActor currently sends uninitialized StructuredCloneData objects in places where it wants the message data to be undefined. The problem with this is that it causes an error when trying to read the data, which leads to odd behavior like `sendQuery` promises never being settled if the message handler throws, or `sendAsyncMessage` and `sendQuery` failing to decode the message if the caller doesn't pass a second argument. The errors reported for these problems also have no context, which means it's very hard for a developer to figure out the source of the problem. And, to make matters worse, the errors look the same as structured clone encoding errors, so there isn't even the clue that the error is happening on decode. This patch updates the offending code to always explicitly initialize the structured clone data with `undefined` when that's what it wants, and adds assertions to make it more obvious where the decode errors are actually happening. Differential Revision: https://phabricator.services.mozilla.com/D35052
dom/ipc/JSWindowActor.cpp
--- a/dom/ipc/JSWindowActor.cpp
+++ b/dom/ipc/JSWindowActor.cpp
@@ -4,16 +4,17 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "mozilla/dom/JSWindowActor.h"
 #include "mozilla/dom/JSWindowActorBinding.h"
 #include "mozilla/dom/MessageManagerBinding.h"
 #include "mozilla/dom/PWindowGlobal.h"
 #include "mozilla/dom/Promise.h"
+#include "js/Promise.h"
 
 namespace mozilla {
 namespace dom {
 
 NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(JSWindowActor)
   NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
   NS_INTERFACE_MAP_ENTRY(nsISupports)
 NS_INTERFACE_MAP_END
@@ -40,18 +41,17 @@ void JSWindowActor::StartDestroy() {
   DestroyCallback(DestroyCallbackFunction::WillDestroy);
 }
 
 void JSWindowActor::AfterDestroy() {
   DestroyCallback(DestroyCallbackFunction::DidDestroy);
 }
 
 void JSWindowActor::DestroyCallback(DestroyCallbackFunction callback) {
-  AutoEntryScript aes(GetParentObject(),
-                      "JSWindowActor destroy callback");
+  AutoEntryScript aes(GetParentObject(), "JSWindowActor destroy callback");
   JSContext* cx = aes.cx();
   MozActorDestroyCallbacks callbacksHolder;
   NS_ENSURE_TRUE_VOID(GetWrapper());
   JS::Rooted<JS::Value> val(cx, JS::ObjectValue(*GetWrapper()));
   if (NS_WARN_IF(!callbacksHolder.Init(cx, val))) {
     return;
   }
 
@@ -83,36 +83,36 @@ void JSWindowActor::SetName(const nsAStr
 }
 
 void JSWindowActor::SendAsyncMessage(JSContext* aCx,
                                      const nsAString& aMessageName,
                                      JS::Handle<JS::Value> aObj,
                                      JS::Handle<JS::Value> aTransfers,
                                      ErrorResult& aRv) {
   ipc::StructuredCloneData data;
-  if (!aObj.isUndefined() && !nsFrameMessageManager::GetParamsForMessage(
-                                 aCx, aObj, aTransfers, data)) {
+  if (!nsFrameMessageManager::GetParamsForMessage(aCx, aObj, aTransfers,
+                                                  data)) {
     aRv.Throw(NS_ERROR_DOM_DATA_CLONE_ERR);
     return;
   }
 
   JSWindowActorMessageMeta meta;
   meta.actorName() = mName;
   meta.messageName() = aMessageName;
   meta.kind() = JSWindowActorMessageKind::Message;
 
   SendRawMessage(meta, std::move(data), aRv);
 }
 
 already_AddRefed<Promise> JSWindowActor::SendQuery(
     JSContext* aCx, const nsAString& aMessageName, JS::Handle<JS::Value> aObj,
     JS::Handle<JS::Value> aTransfers, ErrorResult& aRv) {
   ipc::StructuredCloneData data;
-  if (!aObj.isUndefined() && !nsFrameMessageManager::GetParamsForMessage(
-                                 aCx, aObj, aTransfers, data)) {
+  if (!nsFrameMessageManager::GetParamsForMessage(aCx, aObj, aTransfers,
+                                                  data)) {
     aRv.Throw(NS_ERROR_DOM_DATA_CLONE_ERR);
     return nullptr;
   }
 
   nsIGlobalObject* global = xpc::CurrentNativeGlobal(aCx);
   if (NS_WARN_IF(!global)) {
     aRv.Throw(NS_ERROR_UNEXPECTED);
     return nullptr;
@@ -132,25 +132,29 @@ already_AddRefed<Promise> JSWindowActor:
   mPendingQueries.Put(meta.queryId(), promise);
 
   SendRawMessage(meta, std::move(data), aRv);
   return promise.forget();
 }
 
 void JSWindowActor::ReceiveRawMessage(const JSWindowActorMessageMeta& aMetadata,
                                       ipc::StructuredCloneData&& aData) {
-  AutoEntryScript aes(GetParentObject(),
-                      "JSWindowActor message handler");
+  AutoEntryScript aes(GetParentObject(), "JSWindowActor message handler");
   JSContext* cx = aes.cx();
 
   // Read the message into a JS object from IPC.
   ErrorResult error;
   JS::Rooted<JS::Value> data(cx);
   aData.Read(cx, &data, error);
   if (NS_WARN_IF(error.Failed())) {
+    if (XRE_IsParentProcess()) {
+      MOZ_ASSERT(false, "Should not receive non-decodable data");
+    } else {
+      MOZ_DIAGNOSTIC_ASSERT(false, "Should not receive non-decodable data");
+    }
     MOZ_ALWAYS_TRUE(error.MaybeSetPendingException(cx));
     return;
   }
 
   switch (aMetadata.kind()) {
     case JSWindowActorMessageKind::QueryResolve:
     case JSWindowActorMessageKind::QueryReject:
       ReceiveQueryReply(cx, aMetadata, data, error);
@@ -261,18 +265,19 @@ void JSWindowActor::QueryHandler::Reject
   }
 
   // Make sure that this rejection is reported, despite being "handled". This
   // is done by creating a new promise in the rejected state, and throwing it
   // away. This will be reported as an unhandled rejected promise.
   Unused << JS::CallOriginalPromiseReject(aCx, aValue);
 
   // The exception probably isn't cloneable, so just send down undefined.
-  SendReply(aCx, JSWindowActorMessageKind::QueryReject,
-            ipc::StructuredCloneData());
+  ipc::StructuredCloneData data;
+  data.Write(aCx, JS::UndefinedHandleValue, IgnoredErrorResult());
+  SendReply(aCx, JSWindowActorMessageKind::QueryReject, std::move(data));
 }
 
 void JSWindowActor::QueryHandler::ResolvedCallback(
     JSContext* aCx, JS::Handle<JS::Value> aValue) {
   if (!mActor) {
     return;
   }
 
@@ -288,18 +293,19 @@ void JSWindowActor::QueryHandler::Resolv
     msg.Append(mActor->Name());
     msg.Append(':');
     msg.Append(mMessageName);
     msg.Append(NS_LITERAL_STRING(": message reply cannot be cloned."));
     nsContentUtils::LogSimpleConsoleError(msg, "chrome", false, true);
 
     JS_ClearPendingException(aCx);
 
-    SendReply(aCx, JSWindowActorMessageKind::QueryReject,
-              ipc::StructuredCloneData());
+    ipc::StructuredCloneData data;
+    data.Write(aCx, JS::UndefinedHandleValue, IgnoredErrorResult());
+    SendReply(aCx, JSWindowActorMessageKind::QueryReject, std::move(data));
     return;
   }
 
   SendReply(aCx, JSWindowActorMessageKind::QueryResolve, std::move(data));
 }
 
 void JSWindowActor::QueryHandler::SendReply(JSContext* aCx,
                                             JSWindowActorMessageKind aKind,