diff --git a/docs/api/breaking-changes.md b/docs/api/breaking-changes.md index ab8cff86ad46..c679e6646c55 100644 --- a/docs/api/breaking-changes.md +++ b/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 diff --git a/shell/common/gin_converters/blink_converter.cc b/shell/common/gin_converters/blink_converter.cc index 0be8953d489f..ae9f98641d0a 100644 --- a/shell/common/gin_converters/blink_converter.cc +++ b/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::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 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 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 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 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 value; + if (!deserializer_.ReadValue(context).ToLocal(&value)) { return v8::Null(isolate_); - switch (tag) { - case kNewSerializationTag: { - v8::Local value; - if (!deserializer_.ReadValue(context).ToLocal(&value)) { - return v8::Null(isolate_); - } - return scope.Escape(value); - } - case kOldSerializationTag: { - v8::Local 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(tag_bytes); - return true; - } - - bool ReadBaseValue(v8::Local* 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(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: diff --git a/spec-main/api-ipc-renderer-spec.ts b/spec-main/api-ipc-renderer-spec.ts index c9505965ca59..dd4bab169afa 100644 --- a/spec-main/api-ipc-renderer-spec.ts +++ b/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 () => { diff --git a/spec-main/api-remote-spec.ts b/spec-main/api-remote-spec.ts index f6b8bad17a23..a80b3114b313 100644 --- a/spec-main/api-remote-spec.ts +++ b/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 diff --git a/spec-main/modules-spec.ts b/spec-main/modules-spec.ts index f224a5021ee5..590ffbeafe46 100644 --- a/spec-main/modules-spec.ts +++ b/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) {