diff --git a/shell/renderer/electron_api_service_impl.cc b/shell/renderer/electron_api_service_impl.cc index 14a38f445bbd..24b6f901b278 100644 --- a/shell/renderer/electron_api_service_impl.cc +++ b/shell/renderer/electron_api_service_impl.cc @@ -98,38 +98,61 @@ 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) { + renderer_client_(renderer_client), + weak_factory_(this) {} + +void ElectronApiServiceImpl::BindTo( + mojom::ElectronRendererAssociatedRequest request) { + // Note: BindTo might be called for multiple times. + if (binding_.is_bound()) + binding_.Unbind(); + binding_.Bind(std::move(request)); - binding_.set_connection_error_handler(base::BindOnce( - &ElectronApiServiceImpl::OnDestruct, base::Unretained(this))); + binding_.set_connection_error_handler( + base::BindOnce(&ElectronApiServiceImpl::OnConnectionError, GetWeakPtr())); } -// static -void ElectronApiServiceImpl::CreateMojoService( - content::RenderFrame* render_frame, - RendererClientBase* renderer_client, - mojom::ElectronRendererAssociatedRequest request) { - DCHECK(render_frame); - - // Owns itself. Will be deleted when the render frame is destroyed. - new ElectronApiServiceImpl(render_frame, renderer_client, std::move(request)); +void ElectronApiServiceImpl::DidCreateDocumentElement() { + document_created_ = true; } 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, base::Value arguments, int32_t sender_id) { - blink::WebLocalFrame* frame = render_frame_->GetWebFrame(); + // 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/shell/renderer/electron_api_service_impl.h b/shell/renderer/electron_api_service_impl.h index eb14479af379..cbc155850874 100644 --- a/shell/renderer/electron_api_service_impl.h +++ b/shell/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/shell/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,19 +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/shell/renderer/renderer_client_base.cc b/shell/renderer/renderer_client_base.cc index ebfe87b4fb0f..5830b26ecd4f 100644 --- a/shell/renderer/renderer_client_base.cc +++ b/shell/renderer/renderer_client_base.cc @@ -227,17 +227,12 @@ void RendererClientBase::RenderFrameCreated( 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::CreateMojoService, - render_frame, this)); + base::BindRepeating(&ElectronApiServiceImpl::BindTo, + service->GetWeakPtr())); #if BUILDFLAG(ENABLE_PDF_VIEWER) // Allow access to file scheme from pdf viewer. diff --git a/spec-main/api-web-contents-spec.ts b/spec-main/api-web-contents-spec.ts index 6db667cf493c..55d297b1f74e 100644 --- a/spec-main/api-web-contents-spec.ts +++ b/spec-main/api-web-contents-spec.ts @@ -3,7 +3,7 @@ import { AddressInfo } from 'net' import * as chaiAsPromised from 'chai-as-promised' import * as path from 'path' import * as http from 'http' -import { BrowserWindow, webContents } from 'electron' +import { BrowserWindow, ipcMain, webContents } from 'electron' import { emittedOnce } from './events-helpers'; import { closeAllWindows } from './window-helpers'; @@ -77,6 +77,26 @@ describe('webContents module', () => { w.webContents.send(null as any) }).to.throw('Missing required channel argument') }) + + 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() + }) + const w = new BrowserWindow({ + show: false, + webPreferences: { + nodeIntegration: true, + sandbox: false, + contextIsolation: false + } + }) + w.loadFile(path.join(fixturesPath, 'pages', 'send-after-node.html')) + setTimeout(() => { + w.webContents.send("test") + }, 50) + }) }) describe('webContents.executeJavaScript', () => { diff --git a/spec/fixtures/pages/send-after-node.html b/spec/fixtures/pages/send-after-node.html new file mode 100644 index 000000000000..68f5a40b6707 --- /dev/null +++ b/spec/fixtures/pages/send-after-node.html @@ -0,0 +1,9 @@ + + + + +