From 66c208ff622116a4560469304c67a75c568bfec0 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Wed, 9 Feb 2022 11:57:58 +0900 Subject: [PATCH] Revert "fix: window.open not overriding parent's webPreferences (#32057)" This reverts commit 35ac7fb8e61be744206918684a6881d460591620. --- docs/api/window-open.md | 3 -- lib/browser/api/web-contents.ts | 26 +++++------ lib/browser/guest-window-manager.ts | 4 -- shell/renderer/api/electron_api_web_frame.cc | 8 ---- .../electron_render_frame_observer.cc | 46 ------------------- .../renderer/electron_render_frame_observer.h | 1 - shell/renderer/electron_renderer_client.cc | 1 - shell/renderer/web_worker_observer.cc | 2 - spec-main/chromium-spec.ts | 11 ----- .../setimmediate-window-open-crash/index.html | 16 +++---- spec/chromium-spec.js | 10 ++++ spec/static/main.js | 3 +- 12 files changed, 33 insertions(+), 98 deletions(-) diff --git a/docs/api/window-open.md b/docs/api/window-open.md index a2cb76a2a569e..cdaac04291dde 100644 --- a/docs/api/window-open.md +++ b/docs/api/window-open.md @@ -60,9 +60,6 @@ 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 diff --git a/lib/browser/api/web-contents.ts b/lib/browser/api/web-contents.ts index 5705cec695a07..c739836f46329 100644 --- a/lib/browser/api/web-contents.ts +++ b/lib/browser/api/web-contents.ts @@ -680,6 +680,16 @@ 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 @@ -689,19 +699,9 @@ WebContents.prototype._init = function () { transparent: windowOpenOverriddenOptions.transparent, ...windowOpenOverriddenOptions.webPreferences } : undefined; - // TODO(zcbenz): The features string is parsed twice: here where it is - // 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 |new-window| event - // 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); + this._setNextChildWebPreferences( + makeWebPreferences({ embedder: event.sender, secureOverrideWebPreferences }) + ); } }); diff --git a/lib/browser/guest-window-manager.ts b/lib/browser/guest-window-manager.ts index 9815865f72499..b4cc53b6121b9 100644 --- a/lib/browser/guest-window-manager.ts +++ b/lib/browser/guest-window-manager.ts @@ -196,10 +196,6 @@ function makeBrowserWindowOptions ({ embedder, features, overrideOptions }: { height: 600, ...parsedOptions, ...overrideOptions, - // Note that for normal code path an existing WebContents created by - // Chromium will be used, with web preferences parsed in the - // |-will-add-new-contents| event. - // The |webPreferences| here is only used by the |new-window| event. webPreferences: makeWebPreferences({ embedder, insecureParsedWebPreferences: parsedWebPreferences, diff --git a/shell/renderer/api/electron_api_web_frame.cc b/shell/renderer/api/electron_api_web_frame.cc index 25d2b5433b680..b2ff67ebbf73a 100644 --- a/shell/renderer/api/electron_api_web_frame.cc +++ b/shell/renderer/api/electron_api_web_frame.cc @@ -497,14 +497,6 @@ class WebFrameRenderer : public gin::Wrappable, if (pref_name == options::kPreloadScripts) { return gin::ConvertToV8(isolate, prefs.preloads); } 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. diff --git a/shell/renderer/electron_render_frame_observer.cc b/shell/renderer/electron_render_frame_observer.cc index ba1a122868c64..4eb985f5f7981 100644 --- a/shell/renderer/electron_render_frame_observer.cc +++ b/shell/renderer/electron_render_frame_observer.cc @@ -31,7 +31,6 @@ #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 { @@ -59,57 +58,12 @@ 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(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 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 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(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); diff --git a/shell/renderer/electron_render_frame_observer.h b/shell/renderer/electron_render_frame_observer.h index b7a3718172ac7..bbfd4970489c6 100644 --- a/shell/renderer/electron_render_frame_observer.h +++ b/shell/renderer/electron_render_frame_observer.h @@ -44,7 +44,6 @@ 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_; }; diff --git a/shell/renderer/electron_renderer_client.cc b/shell/renderer/electron_renderer_client.cc index c507bdcffccbb..a7a98839fb54e 100644 --- a/shell/renderer/electron_renderer_client.cc +++ b/shell/renderer/electron_renderer_client.cc @@ -157,7 +157,6 @@ 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()); diff --git a/shell/renderer/web_worker_observer.cc b/shell/renderer/web_worker_observer.cc index 287be9ead13f0..2faec5ffc4347 100644 --- a/shell/renderer/web_worker_observer.cc +++ b/shell/renderer/web_worker_observer.cc @@ -37,8 +37,6 @@ 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()); } diff --git a/spec-main/chromium-spec.ts b/spec-main/chromium-spec.ts index f2df2aa34c94f..01d3d26c60112 100644 --- a/spec-main/chromium-spec.ts +++ b/spec-main/chromium-spec.ts @@ -809,17 +809,6 @@ describe('chromium features', () => { expect(typeofProcessGlobal).to.equal('undefined'); }); - it('can disable node integration when it is enabled on the parent window', async () => { - const w = new BrowserWindow({ show: false, webPreferences: { nodeIntegration: 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')); diff --git a/spec-main/fixtures/crash-cases/setimmediate-window-open-crash/index.html b/spec-main/fixtures/crash-cases/setimmediate-window-open-crash/index.html index f0c803fc08c8e..9bb131aef4548 100644 --- a/spec-main/fixtures/crash-cases/setimmediate-window-open-crash/index.html +++ b/spec-main/fixtures/crash-cases/setimmediate-window-open-crash/index.html @@ -1,12 +1,10 @@ diff --git a/spec/chromium-spec.js b/spec/chromium-spec.js index 86737f78860a1..26e43b12ae749 100644 --- a/spec/chromium-spec.js +++ b/spec/chromium-spec.js @@ -154,6 +154,16 @@ 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'); diff --git a/spec/static/main.js b/spec/static/main.js index 28ec06f524cc7..77a7dfc8fc125 100644 --- a/spec/static/main.js +++ b/spec/static/main.js @@ -109,7 +109,8 @@ app.whenReady().then(async function () { backgroundThrottling: false, nodeIntegration: true, webviewTag: true, - contextIsolation: false + contextIsolation: false, + nativeWindowOpen: false } }); window.loadFile('static/index.html', {