diff --git a/lib/browser/api/browser-window.js b/lib/browser/api/browser-window.js index c7f6331e72bc8..2cd258e8182e5 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 } } diff --git a/shell/common/api/api.mojom b/shell/common/api/api.mojom index a549fad71d197..9c2dd2167c9f7 100644 --- a/shell/common/api/api.mojom +++ b/shell/common/api/api.mojom @@ -42,10 +42,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 bb8a3a09b4e26..b355feeee8e41 100644 --- a/shell/renderer/api/atom_api_renderer_ipc.cc +++ b/shell/renderer/api/atom_api_renderer_ipc.cc @@ -35,20 +35,13 @@ RenderFrame* GetCurrentRenderFrame() { class IPCRenderer : public mate::Wrappable { public: - explicit IPCRenderer(v8::Isolate* isolate) - : task_runner_(base::CreateSingleThreadTaskRunner({base::ThreadPool()})) { + explicit IPCRenderer(v8::Isolate* isolate) { Init(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_)); } static void BuildPrototype(v8::Isolate* isolate, @@ -69,7 +62,7 @@ class IPCRenderer : public mate::Wrappable { void Send(bool internal, const std::string& channel, const base::ListValue& arguments) { - electron_browser_ptr_->get()->Message(internal, channel, arguments.Clone()); + electron_browser_ptr_->Message(internal, channel, arguments.Clone()); } v8::Local Invoke(mate::Arguments* args, @@ -78,7 +71,7 @@ class IPCRenderer : public mate::Wrappable { electron::util::Promise p(args->isolate()); auto handle = p.GetHandle(); - electron_browser_ptr_->get()->Invoke( + electron_browser_ptr_->Invoke( channel, arguments.Clone(), base::BindOnce([](electron::util::Promise p, base::Value result) { p.Resolve(result); }, @@ -92,103 +85,42 @@ class IPCRenderer : public mate::Wrappable { int32_t web_contents_id, const std::string& channel, const base::ListValue& arguments) { - electron_browser_ptr_->get()->MessageTo( - internal, send_to_all, web_contents_id, channel, arguments.Clone()); + electron_browser_ptr_->MessageTo(internal, send_to_all, web_contents_id, + channel, arguments.Clone()); } void SendToHost(const std::string& channel, const base::ListValue& arguments) { - electron_browser_ptr_->get()->MessageHost(channel, arguments.Clone()); + electron_browser_ptr_->MessageHost(channel, arguments.Clone()); } base::Value SendSync(bool internal, const std::string& channel, const base::ListValue& arguments) { - // 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: ⭐️ - base::Value 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, - arguments.Clone())); - response_received_event.Wait(); + electron_browser_ptr_->MessageSync(internal, channel, arguments.Clone(), + &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, arguments.Clone())); + // response_received_event.Wait(); return result; } private: - void SendMessageSyncOnWorkerThread(base::WaitableEvent* event, - base::Value* result, - bool internal, - const std::string& channel, - base::Value 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, - base::Value* result, - base::Value response) { - *result = std::move(response); - event->Signal(); - } - - scoped_refptr task_runner_; - scoped_refptr - electron_browser_ptr_; + electron::mojom::ElectronBrowserPtr electron_browser_ptr_; }; void Initialize(v8::Local exports, diff --git a/spec-main/api-browser-window-spec.ts b/spec-main/api-browser-window-spec.ts index 331dd657b9bcc..0d8ccd575c84c 100644 --- a/spec-main/api-browser-window-spec.ts +++ b/spec-main/api-browser-window-spec.ts @@ -599,13 +599,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) }) })