chore: address feedback from review

shell/browser/api/electron_api_utility_process.cc

@@ -53,25 +53,6 @@ GetAllUtilityProcessWrappers() {
   return *s_all_utility_process_wrappers;
 }
 
-namespace {
-
-bool IsValidWrappable(const v8::Local<v8::Value>& obj) {
-  v8::Local<v8::Object> port = v8::Local<v8::Object>::Cast(obj);
-
-  if (!port->IsObject())
-    return false;
-
-  if (port->InternalFieldCount() != gin::kNumberOfInternalFields)
-    return false;
-
-  const auto* info = static_cast<gin::WrapperInfo*>(
-      port->GetAlignedPointerFromInternalField(gin::kWrapperInfoIndex));
-
-  return info && info->embedder == gin::kEmbedderNativeGin;
-}
-
-}  // namespace
-
 namespace api {
 
 gin::WrapperInfo UtilityProcessWrapper::kWrapperInfo = {
@@ -313,45 +294,45 @@ void UtilityProcessWrapper::PostMessage(gin::Arguments* args) {
 
   blink::TransferableMessage transferable_message;
 
-  auto* isolate = args->isolate();
-  gin_helper::ErrorThrower thrower(isolate);
+  gin_helper::ErrorThrower thrower(args->isolate());
 
   // |message| is any value that can be serialized to StructuredClone.
   v8::Local<v8::Value> message_value;
-  args->GetNext(&message_value);
-
-  if (!electron::SerializeV8Value(isolate, message_value,
-                                  &transferable_message)) {
-    // SerializeV8Value sets an exception.
-    return;
+  if (args->GetNext(&message_value)) {
+    if (!electron::SerializeV8Value(args->isolate(), message_value,
+                                    &transferable_message)) {
+      // SerializeV8Value sets an exception.
+      return;
+    }
   }
 
   v8::Local<v8::Value> transferables;
   std::vector<gin::Handle<MessagePort>> wrapped_ports;
   if (args->GetNext(&transferables)) {
     std::vector<v8::Local<v8::Value>> wrapped_port_values;
-    if (!gin::ConvertFromV8(isolate, transferables, &wrapped_port_values)) {
+    if (!gin::ConvertFromV8(args->isolate(), transferables,
+                            &wrapped_port_values)) {
       thrower.ThrowTypeError("transferables must be an array of MessagePorts");
       return;
     }
 
     for (unsigned i = 0; i < wrapped_port_values.size(); ++i) {
-      if (!IsValidWrappable(wrapped_port_values[i])) {
+      if (!gin_helper::IsValidWrappable(wrapped_port_values[i])) {
         thrower.ThrowTypeError("Port at index " + base::NumberToString(i) +
                                " is not a valid port");
         return;
       }
     }
 
-    if (!gin::ConvertFromV8(isolate, transferables, &wrapped_ports)) {
+    if (!gin::ConvertFromV8(args->isolate(), transferables, &wrapped_ports)) {
       thrower.ThrowTypeError("Passed an invalid MessagePort");
       return;
     }
   }
 
   bool threw_exception = false;
-  transferable_message.ports =
-      MessagePort::DisentanglePorts(isolate, wrapped_ports, &threw_exception);
+  transferable_message.ports = MessagePort::DisentanglePorts(
+      args->isolate(), wrapped_ports, &threw_exception);
   if (threw_exception)
     return;
 

shell/browser/api/message_port.cc

@@ -19,6 +19,7 @@
 #include "shell/common/gin_helper/dictionary.h"
 #include "shell/common/gin_helper/error_thrower.h"
 #include "shell/common/gin_helper/event_emitter_caller.h"
+#include "shell/common/gin_helper/wrappable.h"
 #include "shell/common/node_includes.h"
 #include "shell/common/v8_value_serializer.h"
 #include "third_party/blink/public/common/messaging/transferable_message.h"
@@ -27,25 +28,6 @@
 
 namespace electron {
 
-namespace {
-
-bool IsValidWrappable(const v8::Local<v8::Value>& obj) {
-  v8::Local<v8::Object> port = v8::Local<v8::Object>::Cast(obj);
-
-  if (!port->IsObject())
-    return false;
-
-  if (port->InternalFieldCount() != gin::kNumberOfInternalFields)
-    return false;
-
-  const auto* info = static_cast<gin::WrapperInfo*>(
-      port->GetAlignedPointerFromInternalField(gin::kWrapperInfoIndex));
-
-  return info && info->embedder == gin::kEmbedderNativeGin;
-}
-
-}  // namespace
-
 gin::WrapperInfo MessagePort::kWrapperInfo = {gin::kEmbedderNativeGin};
 
 MessagePort::MessagePort() = default;
@@ -69,44 +51,37 @@ void MessagePort::PostMessage(gin::Arguments* args) {
   DCHECK(!IsNeutered());
 
   blink::TransferableMessage transferable_message;
-
-  auto* isolate = args->isolate();
-  gin_helper::ErrorThrower thrower(isolate);
+  gin_helper::ErrorThrower thrower(args->isolate());
 
   // |message| is any value that can be serialized to StructuredClone.
   v8::Local<v8::Value> message_value;
-  args->GetNext(&message_value);
-
-  if (!electron::SerializeV8Value(isolate, message_value,
-                                  &transferable_message)) {
-    // SerializeV8Value sets an exception.
-    return;
-  }
-
-  if (!electron::SerializeV8Value(args->isolate(), message_value,
-                                  &transferable_message)) {
-    // SerializeV8Value sets an exception.
-    return;
+  if (args->GetNext(&message_value)) {
+    if (!electron::SerializeV8Value(args->isolate(), message_value,
+                                    &transferable_message)) {
+      // SerializeV8Value sets an exception.
+      return;
+    }
   }
 
   v8::Local<v8::Value> transferables;
   std::vector<gin::Handle<MessagePort>> wrapped_ports;
   if (args->GetNext(&transferables)) {
     std::vector<v8::Local<v8::Value>> wrapped_port_values;
-    if (!gin::ConvertFromV8(isolate, transferables, &wrapped_port_values)) {
+    if (!gin::ConvertFromV8(args->isolate(), transferables,
+                            &wrapped_port_values)) {
       thrower.ThrowTypeError("transferables must be an array of MessagePorts");
       return;
     }
 
     for (unsigned i = 0; i < wrapped_port_values.size(); ++i) {
-      if (!IsValidWrappable(wrapped_port_values[i])) {
+      if (!gin_helper::IsValidWrappable(wrapped_port_values[i])) {
         thrower.ThrowTypeError("Port at index " + base::NumberToString(i) +
                                " is not a valid port");
         return;
       }
     }
 
-    if (!gin::ConvertFromV8(isolate, transferables, &wrapped_ports)) {
+    if (!gin::ConvertFromV8(args->isolate(), transferables, &wrapped_ports)) {
       thrower.ThrowTypeError("Passed an invalid MessagePort");
       return;
     }
@@ -122,8 +97,8 @@ void MessagePort::PostMessage(gin::Arguments* args) {
   }
 
   bool threw_exception = false;
-  transferable_message.ports =
-      MessagePort::DisentanglePorts(isolate, wrapped_ports, &threw_exception);
+  transferable_message.ports = MessagePort::DisentanglePorts(
+      args->isolate(), wrapped_ports, &threw_exception);
   if (threw_exception)
     return;
 

shell/common/gin_helper/wrappable.cc

@@ -9,6 +9,21 @@
 
 namespace gin_helper {
 
+bool IsValidWrappable(const v8::Local<v8::Value>& obj) {
+  v8::Local<v8::Object> port = v8::Local<v8::Object>::Cast(obj);
+
+  if (!port->IsObject())
+    return false;
+
+  if (port->InternalFieldCount() != gin::kNumberOfInternalFields)
+    return false;
+
+  const auto* info = static_cast<gin::WrapperInfo*>(
+      port->GetAlignedPointerFromInternalField(gin::kWrapperInfoIndex));
+
+  return info && info->embedder == gin::kEmbedderNativeGin;
+}
+
 WrappableBase::WrappableBase() = default;
 
 WrappableBase::~WrappableBase() {

shell/common/gin_helper/wrappable.h

@@ -11,6 +11,11 @@
 
 namespace gin_helper {
 
+// WrapperInfo logic upstream doesn't take into account that an object
+// might have the correct number of internal fields but not be a WrappableBase.
+// We tried to upstream this, but were told to handle it on the embedder side.
+bool IsValidWrappable(const v8::Local<v8::Value>& obj);
+
 namespace internal {
 
 void* FromV8Impl(v8::Isolate* isolate, v8::Local<v8::Value> val);

spec/api-ipc-spec.ts

@@ -425,6 +425,20 @@ describe('ipc module', () => {
         expect(port2).not.to.be.null();
       });
 
+      it('should not throw when supported values are passed as message', () => {
+        const { port1 } = new MessageChannelMain();
+
+        // @ts-expect-error - this shouldn't crash.
+        expect(() => { port1.postMessage(); }).to.not.throw();
+
+        expect(() => { port1.postMessage(undefined); }).to.not.throw();
+        expect(() => { port1.postMessage(42); }).to.not.throw();
+        expect(() => { port1.postMessage(false); }).to.not.throw();
+        expect(() => { port1.postMessage([]); }).to.not.throw();
+        expect(() => { port1.postMessage('hello'); }).to.not.throw();
+        expect(() => { port1.postMessage({ hello: 'goodbye' }); }).to.not.throw();
+      });
+
       it('throws an error when an invalid parameter is sent to postMessage', () => {
         const { port1 } = new MessageChannelMain();
 

spec/api-utility-process-spec.ts

@@ -284,6 +284,20 @@ describe('utilityProcess module', () => {
       await exit;
     });
 
+    it('does not throw when supported values are passed', () => {
+      const child = utilityProcess.fork(path.join(fixturesPath, 'post-message.js'));
+
+      // @ts-expect-error - this shouldn't crash.
+      expect(() => { child.postMessage(); }).to.not.throw();
+
+      expect(() => { child.postMessage(undefined); }).to.not.throw();
+      expect(() => { child.postMessage(42); }).to.not.throw();
+      expect(() => { child.postMessage(false); }).to.not.throw();
+      expect(() => { child.postMessage([]); }).to.not.throw();
+      expect(() => { child.postMessage('hello'); }).to.not.throw();
+      expect(() => { child.postMessage({ hello: 'goodbye' }); }).to.not.throw();
+    });
+
     it('throws when an invalid transferrable is passed', () => {
       const child = utilityProcess.fork(path.join(fixturesPath, 'post-message.js'));
       expect(() => {