fix: window.open not overriding parent's webPreferences (#32057)

* fix: window.open not overriding parent's webPreferences

* test: remove "nativeWindowOpen: false" from renderer tests

docs/api/window-open.md

@@ -64,6 +64,9 @@ window.open('https://github.com', '_blank', 'top=500,left=200,frame=false,nodeIn
   `features` will be passed to any registered `webContents`'s
   `did-create-window` event handler in the `options` argument.
 * `frameName` follows the specification of `windowName` located in the [native documentation](https://developer.mozilla.org/en-US/docs/Web/API/Window/open#parameters).
+* When opening `about:blank`, the child window's `WebPreferences` will be copied
+  from the parent window, and there is no way to override it because Chromium
+  skips browser side navigation in this case.
 
 To customize or cancel the creation of the window, you can optionally set an
 override handler with `webContents.setWindowOpenHandler()` from the main

lib/browser/api/web-contents.ts

@@ -680,16 +680,6 @@ WebContents.prototype._init = function () {
         postBody
       };
       windowOpenOverriddenOptions = this._callWindowOpenHandler(event, details);
-      // if attempting to use this API with the deprecated new-window event,
-      // windowOpenOverriddenOptions will always return null. This ensures
-      // short-term backwards compatibility until new-window is removed.
-      const parsedFeatures = parseFeatures(rawFeatures);
-      const overriddenFeatures: BrowserWindowConstructorOptions = {
-        ...parsedFeatures.options,
-        webPreferences: parsedFeatures.webPreferences
-      };
-      windowOpenOverriddenOptions = windowOpenOverriddenOptions || overriddenFeatures;
-
       if (!event.defaultPrevented) {
         const secureOverrideWebPreferences = windowOpenOverriddenOptions ? {
           // Allow setting of backgroundColor as a webPreference even though
@@ -699,9 +689,19 @@ WebContents.prototype._init = function () {
           transparent: windowOpenOverriddenOptions.transparent,
           ...windowOpenOverriddenOptions.webPreferences
         } : undefined;
-        this._setNextChildWebPreferences(
+        // TODO(zcbenz): The features string is parsed twice: here where it is
-          makeWebPreferences({ embedder: event.sender, secureOverrideWebPreferences })
+        // passed to C++, and in |makeBrowserWindowOptions| later where it is
-        );
+        // not actually used since the WebContents is created here.
+        // We should be able to remove the latter once the |nativeWindowOpen|
+        // option is removed.
+        const { webPreferences: parsedWebPreferences } = parseFeatures(rawFeatures);
+        // Parameters should keep same with |makeBrowserWindowOptions|.
+        const webPreferences = makeWebPreferences({
+          embedder: event.sender,
+          insecureParsedWebPreferences: parsedWebPreferences,
+          secureOverrideWebPreferences
+        });
+        this._setNextChildWebPreferences(webPreferences);
       }
     });
 

lib/browser/guest-window-manager.ts

@@ -217,6 +217,10 @@ function makeBrowserWindowOptions ({ embedder, features, overrideOptions }: {
       height: 600,
       ...parsedOptions,
       ...overrideOptions,
+      // Note that for |nativeWindowOpen: true| the WebContents is created in
+      // |api::WebContents::WebContentsCreatedWithFullParams|, with prefs
+      // parsed in the |-will-add-new-contents| event.
+      // The |webPreferences| here is only used by |nativeWindowOpen: false|.
       webPreferences: makeWebPreferences({
         embedder,
         insecureParsedWebPreferences: parsedWebPreferences,

shell/renderer/api/electron_api_web_frame.cc

@@ -500,6 +500,14 @@ class WebFrameRenderer : public gin::Wrappable<WebFrameRenderer>,
       // NOTE: openerId is internal-only.
       return gin::ConvertToV8(isolate, prefs.opener_id);
     } else if (pref_name == "isWebView") {
+      // FIXME(zcbenz): For child windows opened with window.open('') from
+      // webview, the WebPreferences is inherited from webview and the value
+      // of |is_webview| is wrong.
+      // Please check ElectronRenderFrameObserver::DidInstallConditionalFeatures
+      // for the background.
+      auto* web_frame = render_frame->GetWebFrame();
+      if (web_frame->Opener())
+        return gin::ConvertToV8(isolate, false);
       return gin::ConvertToV8(isolate, prefs.is_webview);
     } else if (pref_name == options::kHiddenPage) {
       // NOTE: hiddenPage is internal-only.

shell/renderer/electron_render_frame_observer.cc

@@ -31,6 +31,7 @@
 #include "third_party/blink/public/web/web_element.h"
 #include "third_party/blink/public/web/web_local_frame.h"
 #include "third_party/blink/public/web/web_script_source.h"
+#include "third_party/blink/renderer/core/frame/web_local_frame_impl.h"  // nogncheck
 #include "ui/base/resource/resource_bundle.h"
 
 namespace electron {
@@ -58,12 +59,57 @@ ElectronRenderFrameObserver::ElectronRenderFrameObserver(
 }
 
 void ElectronRenderFrameObserver::DidClearWindowObject() {
+  // Do a delayed Node.js initialization for child window.
+  // Check DidInstallConditionalFeatures below for the background.
+  auto* web_frame =
+      static_cast<blink::WebLocalFrameImpl*>(render_frame_->GetWebFrame());
+  if (has_delayed_node_initialization_ && web_frame->Opener() &&
+      !web_frame->IsOnInitialEmptyDocument()) {
+    v8::Isolate* isolate = blink::MainThreadIsolate();
+    v8::HandleScope handle_scope(isolate);
+    v8::MicrotasksScope microtasks_scope(
+        isolate, v8::MicrotasksScope::kDoNotRunMicrotasks);
+    v8::Handle<v8::Context> context = web_frame->MainWorldScriptContext();
+    v8::Context::Scope context_scope(context);
+    // DidClearWindowObject only emits for the main world.
+    DidInstallConditionalFeatures(context, MAIN_WORLD_ID);
+  }
+
   renderer_client_->DidClearWindowObject(render_frame_);
 }
 
 void ElectronRenderFrameObserver::DidInstallConditionalFeatures(
     v8::Handle<v8::Context> context,
     int world_id) {
+  // When a child window is created with window.open, its WebPreferences will
+  // be copied from its parent, and Chromium will intialize JS context in it
+  // immediately.
+  // Normally the WebPreferences is overriden in browser before navigation,
+  // but this behavior bypasses the browser side navigation and the child
+  // window will get wrong WebPreferences in the initialization.
+  // This will end up initializing Node.js in the child window with wrong
+  // WebPreferences, leads to problem that child window having node integration
+  // while "nodeIntegration=no" is passed.
+  // We work around this issue by delaying the child window's initialization of
+  // Node.js if this is the initial empty document, and only do it when the
+  // acutal page has started to load.
+  auto* web_frame =
+      static_cast<blink::WebLocalFrameImpl*>(render_frame_->GetWebFrame());
+  if (web_frame->Opener() && web_frame->IsOnInitialEmptyDocument()) {
+    // FIXME(zcbenz): Chromium does not do any browser side navigation for
+    // window.open('about:blank'), so there is no way to override WebPreferences
+    // of it. We should not delay Node.js initialization as there will be no
+    // further loadings.
+    // Please check http://crbug.com/1215096 for updates which may help remove
+    // this hack.
+    GURL url = web_frame->GetDocument().Url();
+    if (!url.IsAboutBlank()) {
+      has_delayed_node_initialization_ = true;
+      return;
+    }
+  }
+  has_delayed_node_initialization_ = false;
+
   auto* isolate = context->GetIsolate();
   v8::MicrotasksScope microtasks_scope(
       isolate, v8::MicrotasksScope::kDoNotRunMicrotasks);

shell/renderer/electron_render_frame_observer.h

@@ -44,6 +44,7 @@ class ElectronRenderFrameObserver : public content::RenderFrameObserver {
   void OnTakeHeapSnapshot(IPC::PlatformFileForTransit file_handle,
                           const std::string& channel);
 
+  bool has_delayed_node_initialization_ = false;
   content::RenderFrame* render_frame_;
   RendererClientBase* renderer_client_;
 };

shell/renderer/electron_renderer_client.cc

@@ -157,6 +157,7 @@ void ElectronRendererClient::WillReleaseScriptContext(
   // We also do this if we have disable electron site instance overrides to
   // avoid memory leaks
   auto prefs = render_frame->GetBlinkPreferences();
+  gin_helper::MicrotasksScope microtasks_scope(env->isolate());
   node::FreeEnvironment(env);
   if (env == node_bindings_->uv_env())
     node::FreeIsolateData(node_bindings_->isolate_data());

shell/renderer/web_worker_observer.cc

@@ -37,6 +37,8 @@ WebWorkerObserver::WebWorkerObserver()
 
 WebWorkerObserver::~WebWorkerObserver() {
   lazy_tls.Pointer()->Set(nullptr);
+  gin_helper::MicrotasksScope microtasks_scope(
+      node_bindings_->uv_env()->isolate());
   node::FreeEnvironment(node_bindings_->uv_env());
   node::FreeIsolateData(node_bindings_->isolate_data());
 }

spec-main/chromium-spec.ts

@@ -814,6 +814,17 @@ describe('chromium features', () => {
       expect(typeofProcessGlobal).to.equal('undefined');
     });
 
+    it('can disable node integration when it is enabled on the parent window with nativeWindowOpen: true', async () => {
+      const w = new BrowserWindow({ show: false, webPreferences: { nodeIntegration: true, nativeWindowOpen: true } });
+      w.loadURL('about:blank');
+      w.webContents.executeJavaScript(`
+        { b = window.open('about:blank', '', 'nodeIntegration=no,show=no'); null }
+      `);
+      const [, contents] = await emittedOnce(app, 'web-contents-created');
+      const typeofProcessGlobal = await contents.executeJavaScript('typeof process');
+      expect(typeofProcessGlobal).to.equal('undefined');
+    });
+
     it('disables JavaScript when it is disabled on the parent window', async () => {
       const w = new BrowserWindow({ show: true, webPreferences: { nodeIntegration: true } });
       w.webContents.loadFile(path.resolve(__dirname, 'fixtures', 'blank.html'));

spec-main/fixtures/crash-cases/setimmediate-window-open-crash/index.html

@@ -1,10 +1,12 @@
 <html>
 <body>
 <script>
-// `setImmediate` schedules a function to be run on the next UV tick, which
+if (location.hash) {
-//  ensures that `window.open` is run from `UvRunOnce()`.
+  window.opener.postMessage('foo', '*');
-setImmediate(async () => {
+} else {
-  if (!location.hash) {
+  // `setImmediate` schedules a function to be run on the next UV tick, which
+  //  ensures that `window.open` is run from `UvRunOnce()`.
+  setImmediate(async () => {
     const p = new Promise(resolve => {
       window.addEventListener('message', resolve, { once: true });
     });
@@ -12,10 +14,8 @@ setImmediate(async () => {
     const e = await p;
 
     window.close();
-  } else {
+  });
-    window.opener.postMessage('foo', '*');
+}
-  }
-});
 </script>
 </body>
 </html>

spec/chromium-spec.js

@@ -154,16 +154,6 @@ describe('chromium feature', () => {
     });
   });
 
-  describe('window.postMessage', () => {
-    it('throws an exception when the targetOrigin cannot be converted to a string', () => {
-      const b = window.open('');
-      expect(() => {
-        b.postMessage('test', { toString: null });
-      }).to.throw('Cannot convert object to primitive value');
-      b.close();
-    });
-  });
-
   describe('window.opener.postMessage', () => {
     it('sets source and origin correctly', async () => {
       const message = waitForEvent(window, 'message');

spec/static/main.js

@@ -109,8 +109,7 @@ app.whenReady().then(async function () {
       backgroundThrottling: false,
       nodeIntegration: true,
       webviewTag: true,
-      contextIsolation: false,
+      contextIsolation: false
-      nativeWindowOpen: false
     }
   });
   window.loadFile('static/index.html', {