fix: don't handle browser messages before document element is created (6-0-x) (electron#19719)

zcbenz · MarshallOfSound · commit 98d78f2fb5e9 · 2019-08-12T10:45:36.000-07:00
* fix: don't handle browser messages before document element is created

* fix: bind ElectronApiServiceImpl later

DidCreateDocumentElement is called before the ElectronApiServiceImpl
gets bound.

* chore: add comment
diff --git a/atom/renderer/electron_api_service_impl.cc b/atom/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) {
-  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);
+  // 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::OnConnectionError, GetWeakPtr()));
+}
 
-  // 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/atom/renderer/electron_api_service_impl.h b/atom/renderer/electron_api_service_impl.h
@@ -7,6 +7,7 @@
 
 #include <string>
 
+#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,19 +34,26 @@ class ElectronApiServiceImpl : public mojom::ElectronRenderer,
   void TakeHeapSnapshot(mojo::ScopedHandle file,
                         TakeHeapSnapshotCallback callback) override;
 
+  base::WeakPtr<ElectronApiServiceImpl> 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<mojom::ElectronRenderer> binding_;
 
-  content::RenderFrame* render_frame_;
   RendererClientBase* renderer_client_;
+  base::WeakPtrFactory<ElectronApiServiceImpl> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ElectronApiServiceImpl);
 };
diff --git a/atom/renderer/renderer_client_base.cc b/atom/renderer/renderer_client_base.cc
@@ -226,17 +226,12 @@ void RendererClientBase::RenderFrameCreated(
       render_frame, std::make_unique<atom::PrintRenderFrameHelperDelegate>());
 #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
@@ -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
@@ -0,0 +1,9 @@
+<html>
+<body>
+<script type="text/javascript" charset="utf-8">
+  require('fs').readdir(process.cwd(), () => {
+    require('electron').ipcRenderer.send('async-node-api-done');
+  })
+</script>
+</body>
+</html>
