From fb738593cc7c8f88fe47f8811b39c8ed6c4f2e1c Mon Sep 17 00:00:00 2001 From: Jeremy Apthorp Date: Mon, 28 Oct 2019 17:05:42 -0700 Subject: [PATCH 1/2] refactor: try just using regular [Sync] for MessageSync --- shell/common/api/api.mojom | 5 +- shell/renderer/api/atom_api_renderer_ipc.cc | 107 ++------------------ 2 files changed, 12 insertions(+), 100 deletions(-) diff --git a/shell/common/api/api.mojom b/shell/common/api/api.mojom index 9548c0d30a385..d02f4132dd501 100644 --- a/shell/common/api/api.mojom +++ b/shell/common/api/api.mojom @@ -55,10 +55,7 @@ interface ElectronBrowser { // Emits an event on |channel| from the ipcMain JavaScript object in the main // process, and waits synchronously for a response. - // - // NB. this is not marked [Sync] because mojo synchronous methods can be - // reordered with respect to asynchronous methods on the same channel. - // Instead, callers can manually block on the response to this method. + [Sync] MessageSync( bool internal, string channel, diff --git a/shell/renderer/api/atom_api_renderer_ipc.cc b/shell/renderer/api/atom_api_renderer_ipc.cc index 1241d6f65d761..1720ea8ad16e3 100644 --- a/shell/renderer/api/atom_api_renderer_ipc.cc +++ b/shell/renderer/api/atom_api_renderer_ipc.cc @@ -41,19 +41,12 @@ class IPCRenderer : public gin::Wrappable { return gin::CreateHandle(isolate, new IPCRenderer(isolate)); } - explicit IPCRenderer(v8::Isolate* isolate) - : task_runner_(base::CreateSingleThreadTaskRunner({base::ThreadPool()})) { + explicit IPCRenderer(v8::Isolate* isolate) { RenderFrame* render_frame = GetCurrentRenderFrame(); DCHECK(render_frame); - // Bind the interface on the background runner. All accesses will be via - // the thread-safe pointer. This is to support our "fake-sync" - // MessageSync() hack; see the comment in IPCRenderer::SendSync. - electron::mojom::ElectronBrowserPtrInfo info; - render_frame->GetRemoteInterfaces()->GetInterface(mojo::MakeRequest(&info)); - electron_browser_ptr_ = - electron::mojom::ThreadSafeElectronBrowserPtr::Create(std::move(info), - task_runner_); + render_frame->GetRemoteInterfaces()->GetInterface( + mojo::MakeRequest(&electron_browser_ptr_)); } // gin::Wrappable: @@ -78,8 +71,7 @@ class IPCRenderer : public gin::Wrappable { if (!gin::ConvertFromV8(isolate, arguments, &message)) { return; } - electron_browser_ptr_->get()->Message(internal, channel, - std::move(message)); + electron_browser_ptr_->Message(internal, channel, std::move(message)); } v8::Local Invoke(v8::Isolate* isolate, @@ -93,7 +85,7 @@ class IPCRenderer : public gin::Wrappable { gin_helper::Promise p(isolate); auto handle = p.GetHandle(); - electron_browser_ptr_->get()->Invoke( + electron_browser_ptr_->Invoke( internal, channel, std::move(message), base::BindOnce( [](gin_helper::Promise p, @@ -113,8 +105,8 @@ class IPCRenderer : public gin::Wrappable { if (!gin::ConvertFromV8(isolate, arguments, &message)) { return; } - electron_browser_ptr_->get()->MessageTo( - internal, send_to_all, web_contents_id, channel, std::move(message)); + electron_browser_ptr_->MessageTo(internal, send_to_all, web_contents_id, + channel, std::move(message)); } void SendToHost(v8::Isolate* isolate, @@ -124,7 +116,7 @@ class IPCRenderer : public gin::Wrappable { if (!gin::ConvertFromV8(isolate, arguments, &message)) { return; } - electron_browser_ptr_->get()->MessageHost(channel, std::move(message)); + electron_browser_ptr_->MessageHost(channel, std::move(message)); } blink::CloneableMessage SendSync(v8::Isolate* isolate, @@ -135,91 +127,14 @@ class IPCRenderer : public gin::Wrappable { if (!gin::ConvertFromV8(isolate, arguments, &message)) { return blink::CloneableMessage(); } - // We aren't using a true synchronous mojo call here. We're calling an - // asynchronous method and blocking on the result. The reason we're doing - // this is a little complicated, so buckle up. - // - // Mojo has a concept of synchronous calls. However, synchronous calls are - // dangerous. In particular, it's quite possible for two processes to call - // synchronous methods on each other and cause a deadlock. Mojo has a - // mechanism to avoid this kind of deadlock: if a process is waiting on the - // result of a synchronous call, and it receives an incoming call for a - // synchronous method, it will process that request immediately, even - // though it's currently blocking. However, if it receives an incoming - // request for an _asynchronous_ method, that can't cause a deadlock, so it - // stashes the request on a queue to be processed once the synchronous - // thing it's waiting on returns. - // - // This behavior is useful for preventing deadlocks, but it is inconvenient - // here because it can result in messages being reordered. If the main - // process is awaiting the result of a synchronous call (which it does only - // very rarely, since it's bad to block the main process), and we send - // first an asynchronous message to the main process, followed by a - // synchronous message, then the main process will process the synchronous - // one first. - // - // It turns out, Electron has some dependency on message ordering, - // especially during window shutdown, and getting messages out of order can - // result in, for example, remote objects disappearing unexpectedly. To - // avoid these issues and guarantee consistent message ordering, we send - // all messages to the main process as asynchronous messages. This causes - // them to always be queued and processed in the same order they were - // received, even if they were received while the main process was waiting - // on a synchronous call. - // - // However, in the calling process, we still need to block on the result, - // because the caller is expecting a result synchronously. So we do a bit - // of a trick: we pass the Mojo handle over to a worker thread, send the - // asynchronous message from that thread, and then block on the result. - // It's important that we pass the handle over to the worker thread, - // because that allows Mojo to process incoming messages (most importantly, - // the response to our request) on that thread. If we didn't pass it to a - // worker thread, and instead sent the call from the main thread, we would - // never receive a response because Mojo wouldn't be able to run its - // message handling code, because the main thread would be tied up blocking - // on the WaitableEvent. - // - // Phew. If you got this far, here's a gold star: ⭐️ blink::CloneableMessage result; - - // A task is posted to a worker thread to execute the request so that - // this thread may block on a waitable event. It is safe to pass raw - // pointers to |result| and |response_received_event| as this stack frame - // will survive until the request is complete. - - base::WaitableEvent response_received_event; - task_runner_->PostTask( - FROM_HERE, base::BindOnce(&IPCRenderer::SendMessageSyncOnWorkerThread, - base::Unretained(this), - base::Unretained(&response_received_event), - base::Unretained(&result), internal, channel, - std::move(message))); - response_received_event.Wait(); + electron_browser_ptr_->MessageSync(internal, channel, std::move(message), + &result); return result; } - void SendMessageSyncOnWorkerThread(base::WaitableEvent* event, - blink::CloneableMessage* result, - bool internal, - const std::string& channel, - blink::CloneableMessage arguments) { - electron_browser_ptr_->get()->MessageSync( - internal, channel, std::move(arguments), - base::BindOnce(&IPCRenderer::ReturnSyncResponseToMainThread, - base::Unretained(event), base::Unretained(result))); - } - - static void ReturnSyncResponseToMainThread(base::WaitableEvent* event, - blink::CloneableMessage* result, - blink::CloneableMessage response) { - *result = std::move(response); - event->Signal(); - } - - scoped_refptr task_runner_; - scoped_refptr - electron_browser_ptr_; + electron::mojom::ElectronBrowserPtr electron_browser_ptr_; }; gin::WrapperInfo IPCRenderer::kWrapperInfo = {gin::kEmbedderNativeGin}; From 84d5d3b5f1b339ef476216688b95054ffe9bd92e Mon Sep 17 00:00:00 2001 From: Jeremy Apthorp Date: Thu, 5 Dec 2019 15:59:02 -0800 Subject: [PATCH 2/2] fix error in BrowserWindow.fromWebContents --- lib/browser/api/browser-window.js | 2 +- spec-main/api-browser-window-spec.ts | 8 +++----- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/lib/browser/api/browser-window.js b/lib/browser/api/browser-window.js index ab8bd433abac3..9f1ee96e0e0e2 100644 --- a/lib/browser/api/browser-window.js +++ b/lib/browser/api/browser-window.js @@ -116,7 +116,7 @@ BrowserWindow.getFocusedWindow = () => { BrowserWindow.fromWebContents = (webContents) => { for (const window of BrowserWindow.getAllWindows()) { - if (window.webContents.equal(webContents)) return window + if (window.webContents && window.webContents.equal(webContents)) return window } return null diff --git a/spec-main/api-browser-window-spec.ts b/spec-main/api-browser-window-spec.ts index 92fb36455ef8f..2fd69a88c8129 100644 --- a/spec-main/api-browser-window-spec.ts +++ b/spec-main/api-browser-window-spec.ts @@ -593,13 +593,11 @@ describe('BrowserWindow module', () => { }) describe('BrowserWindow.getFocusedWindow()', () => { - it('returns the opener window when dev tools window is focused', (done) => { + it('returns the opener window when dev tools window is focused', async () => { w.show() - w.webContents.once('devtools-focused', () => { - expect(BrowserWindow.getFocusedWindow()).to.equal(w) - done() - }) w.webContents.openDevTools({ mode: 'undocked' }) + await emittedOnce(w.webContents, 'devtools-focused') + expect(BrowserWindow.getFocusedWindow()).to.equal(w) }) })