From 8be40e71410f87018fb3897b28cc2a3bec1a6629 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Mon, 12 Aug 2019 15:17:10 +0900 Subject: [PATCH 1/3] fix: don't handle browser messages before document element is created --- atom/renderer/electron_api_service_impl.cc | 23 ++++++++++++++++++++++ atom/renderer/electron_api_service_impl.h | 4 ++++ spec-main/api-web-contents-spec.ts | 18 ++++++++++++++++- spec/fixtures/pages/send-after-node.html | 9 +++++++++ 4 files changed, 53 insertions(+), 1 deletion(-) create mode 100644 spec/fixtures/pages/send-after-node.html diff --git a/atom/renderer/electron_api_service_impl.cc b/atom/renderer/electron_api_service_impl.cc index af73cfc76a7ca..d769f74a43a39 100644 --- a/atom/renderer/electron_api_service_impl.cc +++ b/atom/renderer/electron_api_service_impl.cc @@ -120,6 +120,10 @@ void ElectronApiServiceImpl::CreateMojoService( new ElectronApiServiceImpl(render_frame, renderer_client, std::move(request)); } +void ElectronApiServiceImpl::DidCreateDocumentElement() { + document_created_ = true; +} + void ElectronApiServiceImpl::OnDestruct() { delete this; } @@ -129,6 +133,25 @@ void ElectronApiServiceImpl::Message(bool internal, const std::string& channel, base::Value arguments, int32_t sender_id) { + // Don't handle browser messages before document element is created. + // + // Note: It is probably better to save the message and then replay it after + // document is ready, but current behavior has been there since the first + // day of Electron, and no one has complained so far. + // + // Reason 1: + // When we receive a message from the browser, we try to transfer it + // to a web page, and when we do that Blink creates an empty + // document element if it hasn't been created yet, and it makes our init + // script to run while `window.location` is still "about:blank". + // (See https://github.com/electron/electron/pull/1044.) + // + // Reason 2: + // The libuv message loop integration would be broken for unkown reasons. + // (See https://github.com/electron/electron/issues/19368.) + if (!document_created_) + return; + blink::WebLocalFrame* frame = render_frame_->GetWebFrame(); if (!frame) return; diff --git a/atom/renderer/electron_api_service_impl.h b/atom/renderer/electron_api_service_impl.h index 4c198a9ccccef..8bbe38a53fcb8 100644 --- a/atom/renderer/electron_api_service_impl.h +++ b/atom/renderer/electron_api_service_impl.h @@ -40,8 +40,12 @@ class ElectronApiServiceImpl : public mojom::ElectronRenderer, mojom::ElectronRendererAssociatedRequest request); // RenderFrameObserver implementation. + void DidCreateDocumentElement() override; void OnDestruct() override; + // Whether the DOM document element has been created. + bool document_created_ = false; + mojo::AssociatedBinding binding_; content::RenderFrame* render_frame_; diff --git a/spec-main/api-web-contents-spec.ts b/spec-main/api-web-contents-spec.ts index 364f193765d3a..2615f99a8d94f 100644 --- a/spec-main/api-web-contents-spec.ts +++ b/spec-main/api-web-contents-spec.ts @@ -1,7 +1,7 @@ import * as chai from 'chai' import * as chaiAsPromised from 'chai-as-promised' import * as path from 'path' -import { BrowserWindow, webContents } from 'electron' +import { BrowserWindow, ipcMain, webContents } from 'electron' import { emittedOnce } from './events-helpers'; import { closeWindow } from './window-helpers'; @@ -22,6 +22,8 @@ describe('webContents module', () => { webPreferences: { backgroundThrottling: false, nodeIntegration: true, + sandbox: false, + contextIsolation: false, webviewTag: true } }) @@ -52,4 +54,18 @@ describe('webContents module', () => { expect(all[all.length - 1].getType()).to.equal('remote') }) }) + + describe('webContents.send(channel, args...)', () => { + it('does not block node async APIs when sent before document is ready', (done) => { + // Please reference https://github.com/electron/electron/issues/19368 if + // this test fails. + ipcMain.once('async-node-api-done', () => { + done() + }) + w.loadFile(path.join(fixturesPath, 'pages', 'send-after-node.html')) + setTimeout(() => { + w.webContents.send("test") + }, 50) + }) + }) }) diff --git a/spec/fixtures/pages/send-after-node.html b/spec/fixtures/pages/send-after-node.html new file mode 100644 index 0000000000000..68f5a40b6707b --- /dev/null +++ b/spec/fixtures/pages/send-after-node.html @@ -0,0 +1,9 @@ + + + + + From 0bb495614cbd661902a4ce2f35fe91e196a173d9 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Mon, 12 Aug 2019 21:30:27 +0900 Subject: [PATCH 2/3] fix: bind ElectronApiServiceImpl later DidCreateDocumentElement is called before the ElectronApiServiceImpl gets bound. --- atom/renderer/electron_api_service_impl.cc | 31 +++++++++++----------- atom/renderer/electron_api_service_impl.h | 20 ++++++++------ atom/renderer/renderer_client_base.cc | 5 ++-- 3 files changed, 30 insertions(+), 26 deletions(-) diff --git a/atom/renderer/electron_api_service_impl.cc b/atom/renderer/electron_api_service_impl.cc index d769f74a43a39..cfe9de64ee523 100644 --- a/atom/renderer/electron_api_service_impl.cc +++ b/atom/renderer/electron_api_service_impl.cc @@ -98,26 +98,20 @@ ElectronApiServiceImpl::~ElectronApiServiceImpl() = default; ElectronApiServiceImpl::ElectronApiServiceImpl( content::RenderFrame* render_frame, - RendererClientBase* renderer_client, - mojom::ElectronRendererAssociatedRequest request) + RendererClientBase* renderer_client) : content::RenderFrameObserver(render_frame), binding_(this), - render_frame_(render_frame), - renderer_client_(renderer_client) { - binding_.Bind(std::move(request)); - binding_.set_connection_error_handler(base::BindOnce( - &ElectronApiServiceImpl::OnDestruct, base::Unretained(this))); -} + renderer_client_(renderer_client), + weak_factory_(this) {} -// static -void ElectronApiServiceImpl::CreateMojoService( - content::RenderFrame* render_frame, - RendererClientBase* renderer_client, +void ElectronApiServiceImpl::BindTo( mojom::ElectronRendererAssociatedRequest request) { - DCHECK(render_frame); + if (binding_.is_bound()) + binding_.Unbind(); - // Owns itself. Will be deleted when the render frame is destroyed. - new ElectronApiServiceImpl(render_frame, renderer_client, std::move(request)); + binding_.Bind(std::move(request)); + binding_.set_connection_error_handler( + base::BindOnce(&ElectronApiServiceImpl::OnConnectionError, GetWeakPtr())); } void ElectronApiServiceImpl::DidCreateDocumentElement() { @@ -128,6 +122,11 @@ void ElectronApiServiceImpl::OnDestruct() { delete this; } +void ElectronApiServiceImpl::OnConnectionError() { + if (binding_.is_bound()) + binding_.Unbind(); +} + void ElectronApiServiceImpl::Message(bool internal, bool send_to_all, const std::string& channel, @@ -152,7 +151,7 @@ void ElectronApiServiceImpl::Message(bool internal, if (!document_created_) return; - blink::WebLocalFrame* frame = render_frame_->GetWebFrame(); + blink::WebLocalFrame* frame = render_frame()->GetWebFrame(); if (!frame) return; diff --git a/atom/renderer/electron_api_service_impl.h b/atom/renderer/electron_api_service_impl.h index 8bbe38a53fcb8..6540e4ec80778 100644 --- a/atom/renderer/electron_api_service_impl.h +++ b/atom/renderer/electron_api_service_impl.h @@ -7,6 +7,7 @@ #include +#include "base/memory/weak_ptr.h" #include "content/public/renderer/render_frame.h" #include "content/public/renderer/render_frame_observer.h" #include "electron/atom/common/api/api.mojom.h" @@ -19,10 +20,10 @@ class RendererClientBase; class ElectronApiServiceImpl : public mojom::ElectronRenderer, public content::RenderFrameObserver { public: - static void CreateMojoService( - content::RenderFrame* render_frame, - RendererClientBase* renderer_client, - mojom::ElectronRendererAssociatedRequest request); + ElectronApiServiceImpl(content::RenderFrame* render_frame, + RendererClientBase* renderer_client); + + void BindTo(mojom::ElectronRendererAssociatedRequest request); void Message(bool internal, bool send_to_all, @@ -33,23 +34,26 @@ class ElectronApiServiceImpl : public mojom::ElectronRenderer, void TakeHeapSnapshot(mojo::ScopedHandle file, TakeHeapSnapshotCallback callback) override; + base::WeakPtr GetWeakPtr() { + return weak_factory_.GetWeakPtr(); + } + private: ~ElectronApiServiceImpl() override; - ElectronApiServiceImpl(content::RenderFrame* render_frame, - RendererClientBase* renderer_client, - mojom::ElectronRendererAssociatedRequest request); // RenderFrameObserver implementation. void DidCreateDocumentElement() override; void OnDestruct() override; + void OnConnectionError(); + // Whether the DOM document element has been created. bool document_created_ = false; mojo::AssociatedBinding binding_; - content::RenderFrame* render_frame_; RendererClientBase* renderer_client_; + base::WeakPtrFactory weak_factory_; DISALLOW_COPY_AND_ASSIGN(ElectronApiServiceImpl); }; diff --git a/atom/renderer/renderer_client_base.cc b/atom/renderer/renderer_client_base.cc index b5d62ad04ee8f..40dffe1211684 100644 --- a/atom/renderer/renderer_client_base.cc +++ b/atom/renderer/renderer_client_base.cc @@ -234,9 +234,10 @@ void RendererClientBase::RenderFrameCreated( // it is possible to trigger the preload script before the document is ready // through this interface, we should delay adding it to the registry until // the document is ready. + auto* service = new ElectronApiServiceImpl(render_frame, this); render_frame->GetAssociatedInterfaceRegistry()->AddInterface( - base::BindRepeating(&ElectronApiServiceImpl::CreateMojoService, - render_frame, this)); + base::BindRepeating(&ElectronApiServiceImpl::BindTo, + service->GetWeakPtr())); #if BUILDFLAG(ENABLE_PDF_VIEWER) // Allow access to file scheme from pdf viewer. From 2adef1df3a999fcd724fa13aaa2ab8904e946132 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Mon, 12 Aug 2019 22:05:32 +0900 Subject: [PATCH 3/3] chore: add comment --- atom/renderer/electron_api_service_impl.cc | 1 + atom/renderer/renderer_client_base.cc | 10 ++-------- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/atom/renderer/electron_api_service_impl.cc b/atom/renderer/electron_api_service_impl.cc index cfe9de64ee523..563b4c3dc86f2 100644 --- a/atom/renderer/electron_api_service_impl.cc +++ b/atom/renderer/electron_api_service_impl.cc @@ -106,6 +106,7 @@ ElectronApiServiceImpl::ElectronApiServiceImpl( void ElectronApiServiceImpl::BindTo( mojom::ElectronRendererAssociatedRequest request) { + // Note: BindTo might be called for multiple times. if (binding_.is_bound()) binding_.Unbind(); diff --git a/atom/renderer/renderer_client_base.cc b/atom/renderer/renderer_client_base.cc index 40dffe1211684..330a49bfa7277 100644 --- a/atom/renderer/renderer_client_base.cc +++ b/atom/renderer/renderer_client_base.cc @@ -226,14 +226,8 @@ void RendererClientBase::RenderFrameCreated( render_frame, std::make_unique()); #endif - // TODO(nornagon): it might be possible for an IPC message sent to this - // service to trigger v8 context creation before the page has begun loading. - // However, it's unclear whether such a timing is possible to trigger, and we - // don't have any test to confirm it. Add a test that confirms that a - // main->renderer IPC can't cause the preload script to be executed twice. If - // it is possible to trigger the preload script before the document is ready - // through this interface, we should delay adding it to the registry until - // the document is ready. + // Note: ElectronApiServiceImpl has to be created now to capture the + // DidCreateDocumentElement event. auto* service = new ElectronApiServiceImpl(render_frame, this); render_frame->GetAssociatedInterfaceRegistry()->AddInterface( base::BindRepeating(&ElectronApiServiceImpl::BindTo,