Transferable Streams: Remove exception special handling

Serialising and deserialising exceptions is now supported directly, so
transferable streams do not have to work around the lack of support.
Remove the workarounds.

Change the semantics so that failures to serialise are not hidden by
implicit conversion to undefined. Instead the cancel() or abort() method
will return a rejection, and the stream will be errored.

Update the http/streams/transferable/reason.html web test to reflect the
new semantics.

BUG=894838

Change-Id: I44c5bd968b4544df2706269900ca8461eabfa00c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2257655Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#782976}

third_party/blink/renderer/core/streams/transferable_streams.cc

@@ -52,22 +52,6 @@ namespace {
 // These are the types of messages that are sent between peers.
 enum class MessageType { kPull, kCancel, kChunk, kClose, kAbort, kError };
 
-// These are the different ways an error reason can be encoded.
-enum class ErrorType { kTypeError, kJson, kDomException, kUndefined };
-
-bool IsATypeError(ScriptState* script_state, v8::Local<v8::Object> object) {
-  // There isn't a 100% reliable way to identify a TypeError.
-  return object->IsNativeError() &&
-         object->GetConstructorName()
-             ->Equals(script_state->GetContext(),
-                      V8AtomicString(script_state->GetIsolate(), "TypeError"))
-             .ToChecked();
-}
-
-bool IsADOMException(v8::Isolate* isolate, v8::Local<v8::Object> object) {
-  return V8DOMException::HasInstance(object, isolate);
-}
-
 // Creates a JavaScript object with a null prototype structured like {key1:
 // value2, key2: value2}. This is used to create objects to be serialized by
 // postMessage.
@@ -107,9 +91,8 @@ bool UnpackKeyValueObject(ScriptState* script_state,
   return true;
 }
 
-// Send a message with type |type| and contents |value| over |port|. The type
-// will be packed as a number with key "t", and the value will be packed with
-// key "v".
+// Sends a message with type |type| and contents |value| over |port|. The type
+// is packed as a number with key "t", and the value is packed with key "v".
 void PackAndPostMessage(ScriptState* script_state,
                         MessagePort* port,
                         MessageType type,
@@ -125,198 +108,41 @@ void PackAndPostMessage(ScriptState* script_state,
                     PostMessageOptions::Create(), exception_state);
 }
 
-// Packs an error into an {e: number, s: string} object for transmission by
-// postMessage. Serializing the resulting object should never fail.
-v8::Local<v8::Object> PackErrorType(v8::Isolate* isolate,
-                                    ErrorType type,
-                                    v8::Local<v8::String> string) {
-  auto error_as_number = v8::Number::New(isolate, static_cast<int>(type));
-  return CreateKeyValueObject(isolate, "e", error_as_number, "s", string);
-}
-
-// Overload for the common case where |string| is a compile-time constant.
-v8::Local<v8::Object> PackErrorType(v8::Isolate* isolate,
-                                    ErrorType type,
-                                    const char* string) {
-  return PackErrorType(isolate, type, V8String(isolate, string));
-}
-
-// We'd like to able to transfer TypeError exceptions, but we can't, so we hack
-// around it. PackReason() is guaranteed to succeed and the object produced is
-// guaranteed to be serializable by postMessage(), however data may be lost. It
-// is not very efficient, and has fairly arbitrary semantics.
-// TODO(ricea): Replace once Errors are serializable.
-v8::Local<v8::Value> PackReason(ScriptState* script_state,
-                                v8::Local<v8::Value> reason) {
-  auto* isolate = script_state->GetIsolate();
-  auto context = script_state->GetContext();
-  if (reason->IsString() || reason->IsNumber() || reason->IsBoolean()) {
-    v8::TryCatch try_catch(isolate);
-    v8::Local<v8::String> stringified;
-    if (!v8::JSON::Stringify(context, reason).ToLocal(&stringified)) {
-      return PackErrorType(isolate, ErrorType::kTypeError,
-                           "Cannot transfer message");
-    }
-
-    return PackErrorType(isolate, ErrorType::kJson, stringified);
-  }
-
-  if (reason->IsNull()) {
-    return PackErrorType(isolate, ErrorType::kJson, "null");
-  }
-
-  if (reason->IsFunction() || reason->IsSymbol() || !reason->IsObject()) {
-    // Squash to undefined
-    return PackErrorType(isolate, ErrorType::kUndefined, "");
-  }
-
-  if (IsATypeError(script_state, reason.As<v8::Object>())) {
-    v8::TryCatch try_catch(isolate);
-    // "message" on TypeError is a normal property, meaning that if it
-    // is set, it is set on the object itself. We can take advantage of
-    // this to avoid executing user JavaScript in the case when the
-    // TypeError was generated internally.
-    v8::Local<v8::Value> descriptor;
-    if (!reason.As<v8::Object>()
-             ->GetOwnPropertyDescriptor(context,
-                                        V8AtomicString(isolate, "message"))
-             .ToLocal(&descriptor)) {
-      return PackErrorType(isolate, ErrorType::kTypeError,
-                           "Cannot transfer message");
-    }
-    if (descriptor->IsUndefined()) {
-      return PackErrorType(isolate, ErrorType::kTypeError, "");
-    }
-    v8::Local<v8::Value> message;
-    CHECK(descriptor->IsObject());
-    if (!descriptor.As<v8::Object>()
-             ->Get(context, V8AtomicString(isolate, "value"))
-             .ToLocal(&message)) {
-      message = V8String(isolate, "Cannot transfer message");
-    } else if (!message->IsString()) {
-      message = V8String(isolate, "");
-    }
-    return PackErrorType(isolate, ErrorType::kTypeError,
-                         message.As<v8::String>());
-  }
-
-  if (IsADOMException(isolate, reason.As<v8::Object>())) {
-    DOMException* dom_exception =
-        V8DOMException::ToImpl(reason.As<v8::Object>());
-    String message = dom_exception->message();
-    String name = dom_exception->name();
-    v8::Local<v8::Value> packed = CreateKeyValueObject(
-        isolate, "m", V8String(isolate, message), "n", V8String(isolate, name));
-    // It should be impossible for this to fail, except for out-of-memory.
-    v8::Local<v8::String> packed_string =
-        v8::JSON::Stringify(context, packed).ToLocalChecked();
-    return PackErrorType(isolate, ErrorType::kDomException, packed_string);
-  }
-
-  v8::TryCatch try_catch(isolate);
-  v8::Local<v8::Value> json;
-  if (!v8::JSON::Stringify(context, reason).ToLocal(&json)) {
-    return PackErrorType(isolate, ErrorType::kTypeError,
-                         "Cannot transfer message");
+// Sends a kError message to the remote side, disregarding failure.
+void SendError(ScriptState* script_state,
+               MessagePort* port,
+               v8::Local<v8::Value> error) {
+  ExceptionState exception_state(script_state->GetIsolate(),
+                                 ExceptionState::kUnknownContext, "", "");
+  PackAndPostMessage(script_state, port, MessageType::kError, error,
+                     exception_state);
+  if (exception_state.HadException()) {
+    DLOG(WARNING) << "Disregarding exception while sending error";
+    exception_state.ClearException();
   }
-
-  return PackErrorType(isolate, ErrorType::kJson, json.As<v8::String>());
 }
 
-// Converts an object created by PackReason() back into a clone of the original
-// object, minus any data that was discarded by PackReason().
-bool UnpackReason(ScriptState* script_state,
-                  v8::Local<v8::Value> packed_reason,
-                  v8::Local<v8::Value>* reason) {
-  // We need to be robust against malformed input because it could come from a
-  // compromised renderer.
-  if (!packed_reason->IsObject()) {
-    DLOG(WARNING) << "packed_reason is not an object";
-    return false;
-  }
-
-  v8::Local<v8::Value> encoder_value;
-  v8::Local<v8::Value> string_value;
-  if (!UnpackKeyValueObject(script_state, packed_reason.As<v8::Object>(), "e",
-                            &encoder_value, "s", &string_value)) {
-    return false;
-  }
-
-  if (!encoder_value->IsNumber()) {
-    DLOG(WARNING) << "encoder_value is not a number";
-    return false;
-  }
+// Same as PackAndPostMessage(), except that it attempts to handle exceptions by
+// sending a kError message to the remote side. On failure |error| is set to the
+// original exception and the function returns false. Any error from sending the
+// kError message is ignored.
+bool PackAndPostMessageHandlingExceptions(ScriptState* script_state,
+                                          MessagePort* port,
+                                          MessageType type,
+                                          v8::Local<v8::Value> value,
+                                          v8::Local<v8::Value>* error) {
+  ExceptionState exception_state(script_state->GetIsolate(),
+                                 ExceptionState::kUnknownContext, "", "");
+  PackAndPostMessage(script_state, port, type, value, exception_state);
 
-  int encoder = encoder_value.As<v8::Number>()->Value();
-  if (!string_value->IsString()) {
-    DLOG(WARNING) << "string_value is not a string";
+  if (exception_state.HadException()) {
+    *error = exception_state.GetException();
+    SendError(script_state, port, *error);
+    exception_state.ClearException();
     return false;
   }
 
-  v8::Local<v8::String> string = string_value.As<v8::String>();
-  auto* isolate = script_state->GetIsolate();
-  auto context = script_state->GetContext();
-  switch (static_cast<ErrorType>(encoder)) {
-    case ErrorType::kJson: {
-      v8::TryCatch try_catch(isolate);
-      if (!v8::JSON::Parse(context, string).ToLocal(reason)) {
-        DLOG(WARNING) << "JSON Parse failed. Content: " << ToCoreString(string);
-        return false;
-      }
-      return true;
-    }
-
-    case ErrorType::kTypeError:
-      *reason = v8::Exception::TypeError(string);
-      return true;
-
-    case ErrorType::kDomException: {
-      v8::TryCatch try_catch(isolate);
-      v8::Local<v8::Value> packed_exception;
-      if (!v8::JSON::Parse(context, string).ToLocal(&packed_exception)) {
-        DLOG(WARNING) << "Packed DOMException JSON parse failed";
-        return false;
-      }
-
-      if (!packed_exception->IsObject()) {
-        DLOG(WARNING) << "Packed DOMException is not an object";
-        return false;
-      }
-
-      v8::Local<v8::Value> message;
-      v8::Local<v8::Value> name;
-      if (!UnpackKeyValueObject(script_state, packed_exception.As<v8::Object>(),
-                                "m", &message, "n", &name)) {
-        DLOG(WARNING) << "Failed unpacking packed DOMException";
-        return false;
-      }
-
-      if (!message->IsString()) {
-        DLOG(WARNING) << "DOMException message is not a string";
-        return false;
-      }
-
-      if (!name->IsString()) {
-        DLOG(WARNING) << "DOMException name is not a string";
-        return false;
-      }
-
-      auto ToBlink = [](v8::Local<v8::Value> value) {
-        return ToBlinkString<String>(value.As<v8::String>(), kDoNotExternalize);
-      };
-      *reason = ToV8(DOMException::Create(ToBlink(message), ToBlink(name)),
-                     script_state);
-      return true;
-    }
-
-    case ErrorType::kUndefined:
-      *reason = v8::Undefined(isolate);
-      return true;
-
-    default:
-      DLOG(WARNING) << "Invalid ErrorType: " << encoder;
-      return false;
-  }
+  return true;
 }
 
 // Base class for CrossRealmTransformWritable and CrossRealmTransformReadable.
@@ -398,15 +224,8 @@ class CrossRealmTransformErrorListener final : public NativeEventListener {
         DOMException::Create("chunk could not be cloned", "DataCloneError");
     auto* message_port = target_->GetMessagePort();
     v8::Local<v8::Value> error_value = ToV8(error, script_state);
-    ExceptionState exception_state(script_state->GetIsolate(),
-                                   ExceptionState::kUnknownContext, "", "");
-
-    PackAndPostMessage(script_state, message_port, MessageType::kError,
-                       PackReason(script_state, error_value), exception_state);
-    if (exception_state.HadException()) {
-      DLOG(WARNING) << "Ignoring postMessage failure in error listener";
-      exception_state.ClearException();
-    }
+
+    SendError(script_state, message_port, error_value);
 
     message_port->close();
     target_->HandleError(error_value);
@@ -520,24 +339,14 @@ class CrossRealmTransformWritable::WriteAlgorithm final
                                  v8::Local<v8::Value> chunk) {
     writable_->backpressure_promise_ =
         MakeGarbageCollected<StreamPromiseResolver>(script_state);
-    ExceptionState exception_state(script_state->GetIsolate(),
-                                   ExceptionState::kUnknownContext, "", "");
-    PackAndPostMessage(script_state, writable_->message_port_,
-                       MessageType::kChunk, chunk, exception_state);
-    if (exception_state.HadException()) {
-      auto exception = exception_state.GetException();
-      exception_state.ClearException();
-
-      PackAndPostMessage(
-          script_state, writable_->message_port_, MessageType::kError,
-          PackReason(writable_->script_state_, exception), exception_state);
-      if (exception_state.HadException()) {
-        DLOG(WARNING) << "Disregarding exception while sending error";
-        exception_state.ClearException();
-      }
 
+    v8::Local<v8::Value> error;
+    bool success = PackAndPostMessageHandlingExceptions(
+        script_state, writable_->message_port_, MessageType::kChunk, chunk,
+        &error);
+    if (!success) {
       writable_->message_port_->close();
-      return PromiseReject(script_state, exception);
+      return PromiseReject(script_state, error);
     }
 
     return PromiseResolveWithUndefined(script_state);
@@ -557,17 +366,18 @@ class CrossRealmTransformWritable::CloseAlgorithm final
                              int argc,
                              v8::Local<v8::Value> argv[]) override {
     DCHECK_EQ(argc, 0);
-    ExceptionState exception_state(script_state->GetIsolate(),
-                                   ExceptionState::kUnknownContext, "", "");
-    PackAndPostMessage(
+
+    v8::Local<v8::Value> error;
+    bool success = PackAndPostMessageHandlingExceptions(
         script_state, writable_->message_port_, MessageType::kClose,
-        v8::Undefined(script_state->GetIsolate()), exception_state);
-    if (exception_state.HadException()) {
-      DLOG(WARNING) << "Ignoring exception from PackAndPostMessage kClose";
-      exception_state.ClearException();
-    }
+        v8::Undefined(script_state->GetIsolate()), &error);
 
     writable_->message_port_->close();
+
+    if (!success) {
+      return PromiseReject(script_state, error);
+    }
+
     return PromiseResolveWithUndefined(script_state);
   }
 
@@ -592,16 +402,18 @@ class CrossRealmTransformWritable::AbortAlgorithm final
                              v8::Local<v8::Value> argv[]) override {
     DCHECK_EQ(argc, 1);
     auto reason = argv[0];
-    ExceptionState exception_state(script_state->GetIsolate(),
-                                   ExceptionState::kUnknownContext, "", "");
-    PackAndPostMessage(
-        script_state, writable_->message_port_, MessageType::kAbort,
-        PackReason(writable_->script_state_, reason), exception_state);
-    if (exception_state.HadException()) {
-      DLOG(WARNING) << "Ignoring exception from PackAndPostMessage kAbort";
-      exception_state.ClearException();
-    }
+
+    v8::Local<v8::Value> error;
+    bool success = PackAndPostMessageHandlingExceptions(
+        script_state, writable_->message_port_, MessageType::kAbort, reason,
+        &error);
+
     writable_->message_port_->close();
+
+    if (!success) {
+      return PromiseReject(script_state, error);
+    }
+
     return PromiseResolveWithUndefined(script_state);
   }
 
@@ -649,14 +461,8 @@ void CrossRealmTransformWritable::HandleMessage(MessageType type,
 
     case MessageType::kCancel:
     case MessageType::kError: {
-      v8::Local<v8::Value> reason;
-      if (!UnpackReason(script_state_, value, &reason)) {
-        DLOG(WARNING)
-            << "Invalid message from peer ignored (unable to unpack value)";
-        return;
-      }
       WritableStreamDefaultController::ErrorIfNeeded(script_state_, controller_,
-                                                     reason);
+                                                     value);
       if (backpressure_promise_) {
         backpressure_promise_->ResolveWithUndefined(script_state_);
         backpressure_promise_ = nullptr;
@@ -725,15 +531,15 @@ class CrossRealmTransformReadable::PullAlgorithm final
                              v8::Local<v8::Value> argv[]) override {
     DCHECK_EQ(argc, 0);
     auto* isolate = script_state->GetIsolate();
-    ExceptionState exception_state(isolate, ExceptionState::kUnknownContext, "",
-                                   "");
 
-    PackAndPostMessage(
+    v8::Local<v8::Value> error;
+    bool success = PackAndPostMessageHandlingExceptions(
         script_state, readable_->message_port_, MessageType::kPull,
-        v8::Undefined(script_state->GetIsolate()), exception_state);
-    if (exception_state.HadException()) {
-      DLOG(WARNING) << "Ignoring exception from PackAndPostMessage kClose";
-      exception_state.ClearException();
+        v8::Undefined(isolate), &error);
+
+    if (!success) {
+      readable_->message_port_->close();
+      return PromiseReject(script_state, error);
     }
 
     return readable_->backpressure_promise_->V8Promise(isolate);
@@ -761,18 +567,18 @@ class CrossRealmTransformReadable::CancelAlgorithm final
     DCHECK_EQ(argc, 1);
     auto reason = argv[0];
     readable_->finished_ = true;
-    ExceptionState exception_state(script_state->GetIsolate(),
-                                   ExceptionState::kUnknownContext, "", "");
-
-    PackAndPostMessage(script_state, readable_->message_port_,
-                       MessageType::kCancel, PackReason(script_state, reason),
-                       exception_state);
-    if (exception_state.HadException()) {
-      DLOG(WARNING) << "Ignoring exception from PackAndPostMessage kClose";
-      exception_state.ClearException();
-    }
+
+    v8::Local<v8::Value> error;
+    bool success = PackAndPostMessageHandlingExceptions(
+        script_state, readable_->message_port_, MessageType::kCancel, reason,
+        &error);
 
     readable_->message_port_->close();
+
+    if (!success) {
+      return PromiseReject(script_state, error);
+    }
+
     return PromiseResolveWithUndefined(script_state);
   }
 
@@ -837,15 +643,7 @@ void CrossRealmTransformReadable::HandleMessage(MessageType type,
     case MessageType::kAbort:
     case MessageType::kError: {
       finished_ = true;
-      v8::Local<v8::Value> reason;
-      if (!UnpackReason(script_state_, value, &reason)) {
-        DLOG(WARNING)
-            << "Invalid message from peer ignored (unable to unpack value)";
-        return;
-      }
-
-      ReadableStreamDefaultController::Error(script_state_, controller_,
-                                             reason);
+      ReadableStreamDefaultController::Error(script_state_, controller_, value);
       message_port_->close();
       return;
     }

third_party/blink/web_tests/http/tests/streams/transferable/reason.html

@@ -6,23 +6,26 @@
 <script>
 'use strict';
 
-// These tests verify the algorithm for passing through error reasons from one
-// realm to another. We only test these for cancel on a ReadableStream, and
-// assume that all the other places that should be using the same algorithm are
-// doing so.
+// Chrome used to special-case the reason for cancel() and abort() in order to
+// handle exceptions correctly. This is no longer necessary. These tests are
+// retained to avoid regressions.
 
-function getTransferredReason(originalReason) {
-  return new Promise((resolve, reject) => {
-    createTransferredReadableStream({
-      cancel(reason) {
-        resolve(reason);
-      }
-    }).then(rs => rs.cancel(originalReason))
-      .catch(reject);
+async function getTransferredReason(originalReason) {
+  let resolvePromise;
+  const rv = new Promise(resolve => {
+    resolvePromise = resolve;
   });
+  const rs = await createTransferredReadableStream({
+    cancel(reason) {
+      resolvePromise(reason);
+    }
+  });
+  await rs.cancel(originalReason);
+  return rv;
 }
 
-for (const value of ['hi', '\t\r\n', 7, 3.0, undefined, null, true, false]) {
+for (const value of ['hi', '\t\r\n', 7, 3.0, undefined, null, true, false,
+                    NaN, Infinity]) {
   promise_test(async () => {
     const reason = await getTransferredReason(value);
     assert_equals(reason, value, 'reason should match');
@@ -30,17 +33,12 @@ for (const value of ['hi', '\t\r\n', 7, 3.0, undefined, null, true, false]) {
 }
 
 for (const badType of [Symbol('hi'), _ => 'hi']) {
-  promise_test(async () => {
-    const reason = await getTransferredReason(badType);
-    assert_equals(reason, undefined, 'reason should be undefined');
-  }, `reason with a type of '${typeof badType}' should be squished to undefined`);
-}
-
-for (const badNumber of [NaN, Infinity]) {
-  promise_test(async () => {
-    const reason = await getTransferredReason(badNumber);
-    assert_equals(reason, null, 'reason should be null');
-  }, `number with a value of '${badNumber}' should be squished to null`);
+  promise_test(async t => {
+    return promise_rejects_dom(t, 'DataCloneError',
+                               getTransferredReason(badType),
+                               'cancel() should reject');
+  }, `reason with a type of '${typeof badType}' should be rejected and ` +
+     `error the stream`);
 }
 
 promise_test(async () => {
@@ -55,8 +53,10 @@ promise_test(async () => {
   const circularObject = {};
   circularObject.self = circularObject;
   const reason = await getTransferredReason(circularObject);
-  assert_true(reason instanceof TypeError, 'a TypeError should be output');
-}, 'objects that cannot be expressed in JSON should result in a TypeError');
+  assert_true(reason instanceof Object, 'an Object should be output');
+  assert_equals(reason.self, reason,
+                'the object should have a circular reference');
+}, 'objects that cannot be expressed in JSON should also be preserved');
 
 promise_test(async () => {
   const originalReason = new TypeError('hi');
@@ -77,10 +77,10 @@ promise_test(async () => {
 
 promise_test(async () => {
   const originalReason = new TypeError();
-  originalReason.message = {};
+  originalReason.message = [1, 2, 3];
   const reason = await getTransferredReason(originalReason);
-  assert_equals(reason.message, '', 'message should not be preserved');
-}, 'a TypeError message should not be preserved if it is not a string');
+  assert_equals(reason.message, '1,2,3', 'message should be stringified');
+}, 'a TypeError message should be converted to a string');
 
 promise_test(async () => {
   const originalReason = new TypeError();
@@ -111,17 +111,22 @@ promise_test(async () => {
                 'the names should match');
 }, 'DOMException errors should be preserved');
 
-promise_test(async () => {
-  const originalReason = new RangeError('nope');
-  const reason = await getTransferredReason(originalReason);
-  assert_equals(typeof reason, 'object', 'reason should have type object');
-  assert_false(reason instanceof RangeError,
-               'reason should not be a RangeError');
-  assert_false(reason instanceof Error,
-               'reason should not be an Error');
-  assert_equals(reason.constructor, Object, 'reason should be an Object');
-  assert_array_equals(Object.getOwnPropertyNames(reason), [],
-                      'reason should have no properties');
-}, 'RangeErrors should not be preserved');
+for (const errorConstructor of [EvalError, RangeError,
+                                ReferenceError, SyntaxError, TypeError,
+                                URIError]) {
+  promise_test(async () => {
+    const originalReason = new errorConstructor('nope');
+    const reason = await getTransferredReason(originalReason);
+    assert_equals(typeof reason, 'object', 'reason should have type object');
+    assert_true(reason instanceof errorConstructor,
+                `reason should inherit ${errorConstructor.name}`);
+    assert_true(reason instanceof Error, 'reason should inherit Error');
+    assert_equals(reason.constructor, errorConstructor,
+                  'reason should have the right constructor');
+    assert_equals(reason.name, errorConstructor.name,
+                  `name should match constructor name`);
+    assert_equals(reason.message, 'nope', 'message should match');
+  }, `${errorConstructor.name} should be preserved`);
+}
 
 </script>