refactor: try just using regular [Sync] for MessageSync (#20797)

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

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,

shell/renderer/api/atom_api_renderer_ipc.cc

@@ -41,19 +41,12 @@ class IPCRenderer : public gin::Wrappable<IPCRenderer> {
     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<IPCRenderer> {
     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<v8::Promise> Invoke(v8::Isolate* isolate,
@@ -93,7 +85,7 @@ class IPCRenderer : public gin::Wrappable<IPCRenderer> {
     gin_helper::Promise<blink::CloneableMessage> 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<blink::CloneableMessage> p,
@@ -113,8 +105,8 @@ class IPCRenderer : public gin::Wrappable<IPCRenderer> {
     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<IPCRenderer> {
     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<IPCRenderer> {
     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<base::SequencedTaskRunner> task_runner_;
-  scoped_refptr<electron::mojom::ThreadSafeElectronBrowserPtr>
-      electron_browser_ptr_;
+  electron::mojom::ElectronBrowserPtr electron_browser_ptr_;
 };
 
 gin::WrapperInfo IPCRenderer::kWrapperInfo = {gin::kEmbedderNativeGin};

spec-main/api-browser-window-spec.ts

@@ -595,13 +595,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)
       })
     })