feat: remove deprecated base::Value-based serialization (#21560)

* feat: remove deprecated base::Value-based serialization

* add note to breaking-changes

docs/api/breaking-changes.md

@@ -28,6 +28,22 @@ Electron 8.x, and has been removed in Electron 9.x. The layout zoom level limits
 are now fixed at a minimum of 0.25 and a maximum of 5.0, as defined
 [here](https://chromium.googlesource.com/chromium/src/+/938b37a6d2886bf8335fc7db792f1eb46c65b2ae/third_party/blink/common/page/page_zoom.cc#11).
 
+### Sending non-JS objects over IPC now throws an exception
+
+In Electron 8.0, IPC was changed to use the Structured Clone Algorithm,
+bringing significant performance improvements. To help ease the transition, the
+old IPC serialization algorithm was kept and used for some objects that aren't
+serializable with Structured Clone. In particular, DOM objects (e.g. `Element`,
+`Location` and `DOMMatrix`), Node.js objects backed by C++ classes (e.g.
+`process.env`, some members of `Stream`), and Electron objects backed by C++
+classes (e.g. `WebContents`, `BrowserWindow` and `WebFrame`) are not
+serializable with Structured Clone. Whenever the old algorithm was invoked, a
+deprecation warning was printed.
+
+In Electron 9.0, the old serialization algorithm has been removed, and sending
+such non-serializable objects will now throw an "object could not be cloned"
+error.
+
 ## Planned Breaking API Changes (8.0)
 
 ### Values sent over IPC are now serialized with Structured Clone Algorithm

shell/common/gin_converters/blink_converter.cc

@@ -12,8 +12,6 @@
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "gin/converter.h"
-#include "mojo/public/cpp/base/values_mojom_traits.h"
-#include "mojo/public/mojom/base/values.mojom.h"
 #include "shell/common/deprecate_util.h"
 #include "shell/common/gin_converters/value_converter.h"
 #include "shell/common/gin_helper/dictionary.h"
@@ -510,42 +508,23 @@ bool Converter<network::mojom::ReferrerPolicy>::FromV8(
 }
 
 namespace {
-constexpr uint8_t kNewSerializationTag = 0;
-constexpr uint8_t kOldSerializationTag = 1;
 
 class V8Serializer : public v8::ValueSerializer::Delegate {
  public:
-  explicit V8Serializer(v8::Isolate* isolate,
-                        bool use_old_serialization = false)
-      : isolate_(isolate),
-        serializer_(isolate, this),
-        use_old_serialization_(use_old_serialization) {}
+  explicit V8Serializer(v8::Isolate* isolate)
+      : isolate_(isolate), serializer_(isolate, this) {}
   ~V8Serializer() override = default;
 
   bool Serialize(v8::Local<v8::Value> value, blink::CloneableMessage* out) {
     serializer_.WriteHeader();
-    if (use_old_serialization_) {
-      WriteTag(kOldSerializationTag);
-      if (!WriteBaseValue(value)) {
-        isolate_->ThrowException(
-            StringToV8(isolate_, "An object could not be cloned."));
-        return false;
-      }
-    } else {
-      WriteTag(kNewSerializationTag);
-      bool wrote_value;
-      v8::TryCatch try_catch(isolate_);
-      if (!serializer_.WriteValue(isolate_->GetCurrentContext(), value)
-               .To(&wrote_value)) {
-        try_catch.Reset();
-        if (!V8Serializer(isolate_, true).Serialize(value, out)) {
-          try_catch.ReThrow();
-          return false;
-        }
-        return true;
-      }
-      DCHECK(wrote_value);
+    bool wrote_value;
+    if (!serializer_.WriteValue(isolate_->GetCurrentContext(), value)
+             .To(&wrote_value)) {
+      isolate_->ThrowException(v8::Exception::Error(
+          StringToV8(isolate_, "An object could not be cloned.")));
+      return false;
     }
+    DCHECK(wrote_value);
 
     std::pair<uint8_t*, size_t> buffer = serializer_.Release();
     DCHECK_EQ(buffer.first, data_.data());
@@ -555,29 +534,6 @@ class V8Serializer : public v8::ValueSerializer::Delegate {
     return true;
   }
 
-  bool WriteBaseValue(v8::Local<v8::Value> object) {
-    node::Environment* env = node::Environment::GetCurrent(isolate_);
-    if (env) {
-      electron::EmitDeprecationWarning(
-          env,
-          "Passing functions, DOM objects and other non-cloneable JavaScript "
-          "objects to IPC methods is deprecated and will throw an exception "
-          "beginning with Electron 9.",
-          "DeprecationWarning");
-    }
-    base::Value value;
-    if (!ConvertFromV8(isolate_, object, &value)) {
-      return false;
-    }
-    mojo::Message message = mojo_base::mojom::Value::SerializeAsMessage(&value);
-
-    serializer_.WriteUint32(message.data_num_bytes());
-    serializer_.WriteRawBytes(message.data(), message.data_num_bytes());
-    return true;
-  }
-
-  void WriteTag(uint8_t tag) { serializer_.WriteRawBytes(&tag, 1); }
-
   // v8::ValueSerializer::Delegate
   void* ReallocateBufferMemory(void* old_buffer,
                                size_t size,
@@ -601,7 +557,6 @@ class V8Serializer : public v8::ValueSerializer::Delegate {
   v8::Isolate* isolate_;
   std::vector<uint8_t> data_;
   v8::ValueSerializer serializer_;
-  bool use_old_serialization_;
 };
 
 class V8Deserializer : public v8::ValueDeserializer::Delegate {
@@ -620,54 +575,11 @@ class V8Deserializer : public v8::ValueDeserializer::Delegate {
     if (!deserializer_.ReadHeader(context).To(&read_header))
       return v8::Null(isolate_);
     DCHECK(read_header);
-    uint8_t tag;
-    if (!ReadTag(&tag))
+    v8::Local<v8::Value> value;
+    if (!deserializer_.ReadValue(context).ToLocal(&value)) {
       return v8::Null(isolate_);
-    switch (tag) {
-      case kNewSerializationTag: {
-        v8::Local<v8::Value> value;
-        if (!deserializer_.ReadValue(context).ToLocal(&value)) {
-          return v8::Null(isolate_);
-        }
-        return scope.Escape(value);
-      }
-      case kOldSerializationTag: {
-        v8::Local<v8::Value> value;
-        if (!ReadBaseValue(&value)) {
-          return v8::Null(isolate_);
-        }
-        return scope.Escape(value);
-      }
-      default:
-        NOTREACHED() << "Invalid tag: " << tag;
-        return v8::Null(isolate_);
     }
-  }
-
-  bool ReadTag(uint8_t* tag) {
-    const void* tag_bytes;
-    if (!deserializer_.ReadRawBytes(1, &tag_bytes))
-      return false;
-    *tag = *reinterpret_cast<const uint8_t*>(tag_bytes);
-    return true;
-  }
-
-  bool ReadBaseValue(v8::Local<v8::Value>* value) {
-    uint32_t length;
-    const void* data;
-    if (!deserializer_.ReadUint32(&length) ||
-        !deserializer_.ReadRawBytes(length, &data)) {
-      return false;
-    }
-    mojo::Message message(
-        base::make_span(reinterpret_cast<const uint8_t*>(data), length), {});
-    base::Value out;
-    if (!mojo_base::mojom::Value::DeserializeFromMessage(std::move(message),
-                                                         &out)) {
-      return false;
-    }
-    *value = ConvertToV8(isolate_, out);
-    return true;
+    return scope.Escape(value);
   }
 
  private:

spec-main/api-ipc-renderer-spec.ts

@@ -52,22 +52,15 @@ describe('ipcRenderer module', () => {
       expect(Buffer.from(data).equals(received)).to.be.true()
     })
 
-    // TODO(nornagon): Change this test to expect an exception to be thrown in
-    // Electron 9.
-    it('can send objects with DOM class prototypes', async () => {
-      w.webContents.executeJavaScript(`{
+    it('throws when sending objects with DOM class prototypes', async () => {
+      await expect(w.webContents.executeJavaScript(`{
         const { ipcRenderer } = require('electron')
         ipcRenderer.send('message', document.location)
-      }`)
-      const [, value] = await emittedOnce(ipcMain, 'message')
-      expect(value.protocol).to.equal('about:')
-      expect(value.hostname).to.equal('')
+      }`)).to.eventually.be.rejected()
     })
 
-    // TODO(nornagon): Change this test to expect an exception to be thrown in
-    // Electron 9.
     it('does not crash when sending external objects', async () => {
-      w.webContents.executeJavaScript(`{
+      await expect(w.webContents.executeJavaScript(`{
         const { ipcRenderer } = require('electron')
         const http = require('http')
 
@@ -75,10 +68,7 @@ describe('ipcRenderer module', () => {
         const stream = request.agent.sockets['127.0.0.1:5000:'][0]._handle._externalStream
 
         ipcRenderer.send('message', stream)
-      }`)
-      const [, externalStreamValue] = await emittedOnce(ipcMain, 'message')
-
-      expect(externalStreamValue).to.be.null()
+      }`)).to.eventually.be.rejected()
     })
 
     it('can send objects that both reference the same object', async () => {

spec-main/api-remote-spec.ts

@@ -48,11 +48,12 @@ function makeWindow () {
   before(async () => {
     w = new BrowserWindow({ show: false, webPreferences: { nodeIntegration: true, enableRemoteModule: true } })
     await w.loadURL('about:blank')
-    await w.webContents.executeJavaScript(`
+    await w.webContents.executeJavaScript(`{
       const chai_1 = window.chai_1 = require('chai')
       chai_1.use(require('chai-as-promised'))
       chai_1.use(require('dirty-chai'))
-    `)
+      null
+    }`)
   })
   after(closeAllWindows)
   return () => w
@@ -63,11 +64,12 @@ function makeEachWindow () {
   beforeEach(async () => {
     w = new BrowserWindow({ show: false, webPreferences: { nodeIntegration: true, enableRemoteModule: true } })
     await w.loadURL('about:blank')
-    await w.webContents.executeJavaScript(`
+    await w.webContents.executeJavaScript(`{
       const chai_1 = window.chai_1 = require('chai')
       chai_1.use(require('chai-as-promised'))
       chai_1.use(require('dirty-chai'))
-    `)
+      null
+    }`)
   })
   afterEach(closeAllWindows)
   return () => w

spec-main/modules-spec.ts

@@ -20,7 +20,7 @@ describe('modules support', () => {
       it('can be required in renderer', async () => {
         const w = new BrowserWindow({ show: false, webPreferences: { nodeIntegration: true } })
         w.loadURL('about:blank')
-        await expect(w.webContents.executeJavaScript(`{ require('echo') }`)).to.be.fulfilled()
+        await expect(w.webContents.executeJavaScript(`{ require('echo'); null }`)).to.be.fulfilled()
       })
 
       ifit(features.isRunAsNodeEnabled())('can be required in node binary', function (done) {