New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to use native window.open() in non-sandboxed webcontents #8963

Merged
merged 45 commits into from May 11, 2017

Conversation

Projects
None yet
8 participants
@seanchas116
Copy link
Contributor

seanchas116 commented Mar 19, 2017

This pull request adds nativeWindowOpen option to use native window.open() in non-sandboxed webcontents.

It would be useful when you opens dialogs or preferences windows from your app, as it allows synchronous access to DOM elements in the new windows.
Also synchronous window.open() would be faster because it doesn't need another new process and you could share the same instance of JavaScript libraries/frameworks among windows.
This is a simple use case example: https://github.com/seanchas116/electron-native-window-open-example

I don't know the risk of running multiple Node contexts in the same process, so I would be glad to know it.

Close #8959

@tarruda

This comment has been minimized.

Copy link
Contributor

tarruda commented Mar 20, 2017

According to @zcbenz, it is not safe to have multiple v8 contexts running in parallel due to node.js not supporting multiple instances. See also #1865 (comment)

@seanchas116

This comment has been minimized.

Copy link
Contributor

seanchas116 commented Mar 20, 2017

Probably the problem would be similar to web worker support (#8852) .

@seanchas116

This comment has been minimized.

Copy link
Contributor

seanchas116 commented Mar 20, 2017

Looks like reloading doesn't work well in this mode currently.
(I tried running spec with nativeWindowOpen: true in the host window. It runs without failures besides existing window.open/opener tests, but after reloading the window several tests failed)
I'm trying to fix it...

@seanchas116 seanchas116 force-pushed the seanchas116:native-window-open branch from a73f5ab to c3c67f7 Mar 20, 2017

@seanchas116

This comment has been minimized.

Copy link
Contributor

seanchas116 commented Mar 21, 2017

I ensured renderer process restart after reloading, and reloading now works well.
(Some test cases still fail after reload but the same cases fail on master too)

@zcbenz zcbenz self-assigned this Mar 22, 2017

@seanchas116 seanchas116 force-pushed the seanchas116:native-window-open branch from c04f8c0 to 6f9dbd4 Mar 23, 2017

@@ -90,14 +98,22 @@ bool AtomBrowserClient::ShouldCreateNewSiteInstance(
content::BrowserContext* browser_context,
content::SiteInstance* current_instance,
const GURL& url) {

if (g_suppress_renderer_process_restart) {

This comment has been minimized.

@tarruda

tarruda Mar 27, 2017

Contributor

I wonder if it is possible to have a race condition on this variable. Imagine the following chain of events:

  • renderer with nativeWindowOpen invokes window.open
  • soon after, another renderer without nativeWindowOpen invokes window.open

Can you guarantee that ShouldCreateNewSiteInstance will be invoked for the first navigation before the second navigation?

Also, imagine two renderers with nativeWindowOpen invoking window.open in parallel, both would cause g_suppress_renderer_process_restart to be set, but it seems possible that only one would read it as true.

This comment has been minimized.

@deepak1556

deepak1556 Mar 29, 2017

Member

Did you mean g_reuse_renderer_process_for_new_window ? g_suppress_renderer_process_restart is only used during history navigations, I am not sure why it would affect this mode alone.

This comment has been minimized.

@tarruda

tarruda Mar 31, 2017

Contributor

Did you mean g_reuse_renderer_process_for_new_window ?

Yes that's what I mean, thanks.

This comment has been minimized.

@seanchas116

seanchas116 Apr 2, 2017

Contributor

Can I only allow about:blank as synchronously-opened window for now?

if (!options.webContents || url !== 'about:blank') {
// We should not call `loadURL` if the window was constructed from an
// existing webContents(window.open in a sandboxed renderer) and if the url
// is not 'about:blank'.
//
// Navigating to the url when creating the window from an existing
// webContents would not be necessary(it will navigate there anyway), but
// apparently there's a bug that allows the child window to be scripted by
// the opener, even when the child window is from another origin.
//
// That's why the second condition(url !== "about:blank") is required: to
// force `OverrideSiteInstanceForNavigation` to be called and consequently
// spawn a new renderer if the new window is targeting a different origin.
//
// If the URL is "about:blank", then it is very likely that the opener just
// wants to synchronously script the popup, for example:
//
// let popup = window.open()
// popup.document.body.write('<h1>hello</h1>')
//
// The above code would not work if a navigation to "about:blank" is done
// here, since the window would be cleared of all changes in the next tick.
const loadOptions = {}
if (postData != null) {
loadOptions.postData = postData
loadOptions.extraHeaders = 'content-type: application/x-www-form-urlencoded'
if (postData.length > 0) {
const postDataFront = postData[0].bytes.toString()
const boundary = /^--.*[^-\r\n]/.exec(postDataFront)
if (boundary != null) {
loadOptions.extraHeaders = `content-type: multipart/form-data; boundary=${boundary[0].substr(2)}`
}
}
}
guest.loadURL(url, loadOptions)
}

In the current implementation, if the URL opened with window.open is not about:blank, createGuest causes OverrideSiteInstanceForNavigation, and that's why I did workaround with g_reuse_renderer_process_for_new_window variable and prevented renderer process spawn in ShouldCreateNewSiteInstance.

If about:blank is only allowed as synchronously-opened window, I can remove g_reuse_renderer_process_for_new_window completely.

This comment has been minimized.

@deepak1556

deepak1556 Apr 3, 2017

Member

that's why I did workaround with g_reuse_renderer_process_for_new_window variable and prevented renderer process spawn in ShouldCreateNewSiteInstance.

The render process is not reused for same origin child window, process.pid returns different values. g_reuse_render_process_for_new_window is being modified on two different threads, which causes a race.

### Use Native `window.open()`

If you want to use native `window.open()` implementation, pass `useNativeWindowOpen: true` in `webPreferences` option.
Native `window.open()` allows synchronous access to opened windows so it is convenient choise if you need to open a dialog or a preferences window.

This comment has been minimized.

@kevinsawicki

kevinsawicki Mar 27, 2017

Contributor

choise -> choice

@zcbenz

This comment has been minimized.

Copy link
Contributor

zcbenz commented Mar 28, 2017

According to @zcbenz, it is not safe to have multiple v8 contexts running in parallel due to node.js not supporting multiple instances. See also #1865 (comment)

Things have changed after Node v7 and it is being fixed (#8811).

height: 100
})
modal = new BrowserWindow(options)
event.newGuest = modal

This comment has been minimized.

@zcbenz

zcbenz Mar 28, 2017

Contributor

modal is not defined anywhere, maybe just event.newGuest = new BrowserWindow(options).

@@ -242,10 +242,14 @@ ipcMain.on('ELECTRON_GUEST_WINDOW_MANAGER_INTERNAL_WINDOW_OPEN', function (event
disposition, options,
additionalFeatures, postData) {
options = mergeBrowserWindowOptions(event.sender, options)
const {webContents} = options

This comment has been minimized.

@zcbenz

zcbenz Mar 28, 2017

Contributor

It is better to use options.webContents directly, a single webContents can have lots of meanings and it is hard to find its declaration when reading the code.

@@ -106,6 +106,8 @@ void WebContentsPreferences::AppendExtraCommandLineSwitches(
// integration.
if (IsSandboxed(web_contents))
command_line->AppendSwitch(switches::kEnableSandbox);
if (UsesNativeWindowOpen(web_contents))

This comment has been minimized.

@zcbenz

zcbenz Mar 28, 2017

Contributor

We should just use if (web_preferences.GetBoolean("nativeWindowOpen", &b) && b), calling UsesNativeWindowOpen() does unnecessary checks here.

event.sender.emit('new-window', event, url, frameName, disposition, options, additionalFeatures)
const newGuest = event.newGuest
if ((event.sender.isGuest() && !event.sender.allowPopups) || event.defaultPrevented) {
if (newGuest !== undefined && newGuest !== null) {
if (webContents === newGuest.webContents) {
event.defaultPrevented = false

This comment has been minimized.

@zcbenz

zcbenz Mar 28, 2017

Contributor

Should add a comment on why doing this.

@seanchas116

This comment has been minimized.

Copy link
Contributor

seanchas116 commented Apr 6, 2017

I changed code not to use g_reuse_renderer_process_for_new_window, and instead it now tracks non-child WebContents to check if the process should be restarted.

@poiru

This comment has been minimized.

Copy link
Member

poiru commented Apr 18, 2017

FYI libchromiumcontent was updated in #9219.

@@ -4,5 +4,6 @@ const {ipcRenderer} = require('electron')

const {guestInstanceId, openerId} = process
const hiddenPage = process.argv.includes('--hidden-page')
const usesNativeWindowOpen = process.argv.includes('--native-window-open')

This comment has been minimized.

@kevinsawicki

kevinsawicki Apr 26, 2017

Contributor

It would be great to support this as well when contextIsolation is enabled, which mean this variable would need to be threaded through in lib/isolated_renderer/init.js.

This comment has been minimized.

@seanchas116

seanchas116 Apr 30, 2017

Contributor

I've updated it in 8b6b512.

<script type="text/javascript" charset="utf-8">
const {ipcRenderer} = require("electron");
const tests = {
'blank': () => {

This comment has been minimized.

@kevinsawicki

kevinsawicki Apr 26, 2017

Contributor

I think it might be nice to split up this file into 3 separate files, one for each case being tested. Then we don't risk one test case possibly influencing another since they will each be self-contained.

@zcbenz

zcbenz approved these changes May 10, 2017

Copy link
Contributor

zcbenz left a comment

Looks good to me 👍

@zcbenz zcbenz merged commit 0c3f80c into electron:master May 11, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@seanchas116

This comment has been minimized.

Copy link
Contributor

seanchas116 commented May 11, 2017

Thanks!

@quanglam2807

This comment has been minimized.

Copy link

quanglam2807 commented May 17, 2017

Thanks so much for the work. I really need this.

It seems like it has a bug with webview. Try this:

<!DOCTYPE html>
<html>
  <head>
    <title>Modal with native window.open()</title>
  </head>
  <body>
    <webview id="foo" src="https://asana.com" style="display:inline-flex; width:640px; height:480px" webpreferences="nativeWindowOpen"></webview>
  </body>
</html>

Go to Log in > Login with Google, Electron will crash.

Update: It's related to #8378, not this PR.

@trusktr

This comment has been minimized.

Copy link

trusktr commented Jun 5, 2018

If I'm using native window.open, how do I get the BrowserWindow associated with the native window returned from window.open?

@trusktr

This comment has been minimized.

Copy link

trusktr commented Jun 5, 2018

I found a hack. Using native window.open:

window.open( ... ) // returns a native window object.
const win = BrowserWindow.getFocusedWindow() // returns the BrowserWindow of the window we just opened.

Is this reliable? Is there a better way?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment