Merge pull request #8963 from seanchas116/native-window-open

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

atom/browser/api/atom_api_web_contents.cc

@@ -13,6 +13,7 @@
 #include "atom/browser/atom_browser_client.h"
 #include "atom/browser/atom_browser_context.h"
 #include "atom/browser/atom_browser_main_parts.h"
+#include "atom/browser/child_web_contents_tracker.h"
 #include "atom/browser/lib/bluetooth_chooser.h"
 #include "atom/browser/native_window.h"
 #include "atom/browser/net/atom_network_delegate.h"
@@ -485,6 +486,7 @@ void WebContents::AddNewContents(content::WebContents* source,
                                  const gfx::Rect& initial_rect,
                                  bool user_gesture,
                                  bool* was_blocked) {
+  new ChildWebContentsTracker(new_contents);
   v8::Locker locker(isolate());
   v8::HandleScope handle_scope(isolate());
   auto api_web_contents = CreateFrom(isolate(), new_contents);

atom/browser/atom_browser_client.cc

@@ -15,6 +15,7 @@
 #include "atom/browser/atom_quota_permission_context.h"
 #include "atom/browser/atom_resource_dispatcher_host_delegate.h"
 #include "atom/browser/atom_speech_recognition_manager_delegate.h"
+#include "atom/browser/child_web_contents_tracker.h"
 #include "atom/browser/native_window.h"
 #include "atom/browser/web_contents_permission_helper.h"
 #include "atom/browser/web_contents_preferences.h"
@@ -87,17 +88,31 @@ content::WebContents* AtomBrowserClient::GetWebContentsFromProcessID(
 }
 
 bool AtomBrowserClient::ShouldCreateNewSiteInstance(
+    content::RenderFrameHost* render_frame_host,
     content::BrowserContext* browser_context,
     content::SiteInstance* current_instance,
     const GURL& url) {
-
   if (url.SchemeIs(url::kJavaScriptScheme))
     // "javacript:" scheme should always use same SiteInstance
     return false;
 
-  if (!IsRendererSandboxed(current_instance->GetProcess()->GetID()))
-    // non-sandboxed renderers should always create a new SiteInstance
-    return true;
+  int process_id = current_instance->GetProcess()->GetID();
+  if (!IsRendererSandboxed(process_id)) {
+    if (!RendererUsesNativeWindowOpen(process_id)) {
+      // non-sandboxed renderers without native window.open should always create
+      // a new SiteInstance
+      return true;
+    }
+    auto web_contents =
+        content::WebContents::FromRenderFrameHost(render_frame_host);
+    if (!ChildWebContentsTracker::IsChildWebContents(web_contents)) {
+      // Root WebContents should always create new process to make sure
+      // native addons are loaded correctly after reload / navigation.
+      // (Non-root WebContents opened by window.open() should try to
+      //  reuse process to allow synchronous cross-window scripting.)
+      return true;
+    }
+  }
 
   // Create new a SiteInstance if navigating to a different site.
   auto src_url = current_instance->GetSiteURL();
@@ -109,19 +124,27 @@ bool AtomBrowserClient::ShouldCreateNewSiteInstance(
       content::SiteInstance::GetSiteForURL(browser_context, url) != src_url;
 }
 
-void AtomBrowserClient::AddSandboxedRendererId(int process_id) {
-  base::AutoLock auto_lock(sandboxed_renderers_lock_);
-  sandboxed_renderers_.insert(process_id);
+void AtomBrowserClient::AddProcessPreferences(
+    int process_id, AtomBrowserClient::ProcessPreferences prefs) {
+  base::AutoLock auto_lock(process_preferences_lock_);
+  process_preferences_[process_id] = prefs;
 }
 
-void AtomBrowserClient::RemoveSandboxedRendererId(int process_id) {
-  base::AutoLock auto_lock(sandboxed_renderers_lock_);
-  sandboxed_renderers_.erase(process_id);
+void AtomBrowserClient::RemoveProcessPreferences(int process_id) {
+  base::AutoLock auto_lock(process_preferences_lock_);
+  process_preferences_.erase(process_id);
 }
 
 bool AtomBrowserClient::IsRendererSandboxed(int process_id) {
-  base::AutoLock auto_lock(sandboxed_renderers_lock_);
-  return sandboxed_renderers_.count(process_id);
+  base::AutoLock auto_lock(process_preferences_lock_);
+  auto it = process_preferences_.find(process_id);
+  return it != process_preferences_.end() && it->second.sandbox;
+}
+
+bool AtomBrowserClient::RendererUsesNativeWindowOpen(int process_id) {
+  base::AutoLock auto_lock(process_preferences_lock_);
+  auto it = process_preferences_.find(process_id);
+  return it != process_preferences_.end() && it->second.native_window_open;
 }
 
 void AtomBrowserClient::RenderProcessWillLaunch(
@@ -133,11 +156,13 @@ void AtomBrowserClient::RenderProcessWillLaunch(
       new WidevineCdmMessageFilter(process_id, host->GetBrowserContext()));
 
   content::WebContents* web_contents = GetWebContentsFromProcessID(process_id);
-  if (WebContentsPreferences::IsSandboxed(web_contents)) {
-    AddSandboxedRendererId(host->GetID());
-    // ensure the sandboxed renderer id is removed later
-    host->AddObserver(this);
-  }
+  ProcessPreferences process_prefs;
+  process_prefs.sandbox = WebContentsPreferences::IsSandboxed(web_contents);
+  process_prefs.native_window_open
+      = WebContentsPreferences::UsesNativeWindowOpen(web_contents);
+  AddProcessPreferences(host->GetID(), process_prefs);
+  // ensure the ProcessPreferences is removed later
+  host->AddObserver(this);
 }
 
 content::SpeechRecognitionManagerDelegate*
@@ -182,7 +207,8 @@ void AtomBrowserClient::OverrideSiteInstanceForNavigation(
     return;
   }
 
-  if (!ShouldCreateNewSiteInstance(browser_context, current_instance, url))
+  if (!ShouldCreateNewSiteInstance(render_frame_host, browser_context,
+                                   current_instance, url))
     return;
 
   scoped_refptr<content::SiteInstance> site_instance =
@@ -315,7 +341,8 @@ bool AtomBrowserClient::CanCreateWindow(
     bool* no_javascript_access) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
-  if (IsRendererSandboxed(opener_render_process_id)) {
+  if (IsRendererSandboxed(opener_render_process_id)
+      || RendererUsesNativeWindowOpen(opener_render_process_id)) {
     *no_javascript_access = false;
     return true;
   }
@@ -380,7 +407,7 @@ void AtomBrowserClient::RenderProcessHostDestroyed(
       break;
     }
   }
-  RemoveSandboxedRendererId(process_id);
+  RemoveProcessPreferences(process_id);
 }
 
 }  // namespace atom

atom/browser/atom_browser_client.h

@@ -111,19 +111,24 @@ class AtomBrowserClient : public brightray::BrowserClient,
   void RenderProcessHostDestroyed(content::RenderProcessHost* host) override;
 
  private:
-  bool ShouldCreateNewSiteInstance(content::BrowserContext* browser_context,
+  bool ShouldCreateNewSiteInstance(content::RenderFrameHost* render_frame_host,
+                                   content::BrowserContext* browser_context,
                                    content::SiteInstance* current_instance,
                                    const GURL& dest_url);
-  // Add/remove a process id to `sandboxed_renderers_`.
-  void AddSandboxedRendererId(int process_id);
-  void RemoveSandboxedRendererId(int process_id);
+  struct ProcessPreferences {
+    bool sandbox;
+    bool native_window_open;
+  };
+  void AddProcessPreferences(int process_id, ProcessPreferences prefs);
+  void RemoveProcessPreferences(int process_id);
   bool IsRendererSandboxed(int process_id);
+  bool RendererUsesNativeWindowOpen(int process_id);
 
   // pending_render_process => current_render_process.
   std::map<int, int> pending_processes_;
-  // Set that contains the process ids of all sandboxed renderers
-  std::set<int> sandboxed_renderers_;
-  base::Lock sandboxed_renderers_lock_;
+
+  std::map<int, ProcessPreferences> process_preferences_;
+  base::Lock process_preferences_lock_;
 
   std::unique_ptr<AtomResourceDispatcherHostDelegate>
       resource_dispatcher_host_delegate_;

atom/browser/child_web_contents_tracker.cc

@@ -0,0 +1,33 @@
+// Copyright (c) 2017 GitHub, Inc.
+// Use of this source code is governed by the MIT license that can be
+// found in the LICENSE file.
+
+#include "atom/browser/child_web_contents_tracker.h"
+
+#include <unordered_set>
+
+namespace atom {
+
+namespace {
+
+std::unordered_set<content::WebContents*> g_child_web_contents;
+
+}  // namespace
+
+ChildWebContentsTracker::ChildWebContentsTracker(
+    content::WebContents* web_contents)
+    : content::WebContentsObserver(web_contents) {
+  g_child_web_contents.insert(web_contents);
+}
+
+bool ChildWebContentsTracker::IsChildWebContents(
+    content::WebContents* web_contents) {
+  return g_child_web_contents.find(web_contents) != g_child_web_contents.end();
+}
+
+void ChildWebContentsTracker::WebContentsDestroyed() {
+  g_child_web_contents.erase(web_contents());
+  delete this;
+}
+
+}  // namespace atom

atom/browser/child_web_contents_tracker.h

@@ -0,0 +1,25 @@
+// Copyright (c) 2017 GitHub, Inc.
+// Use of this source code is governed by the MIT license that can be
+// found in the LICENSE file.
+
+#ifndef ATOM_BROWSER_CHILD_WEB_CONTENTS_TRACKER_H_
+#define ATOM_BROWSER_CHILD_WEB_CONTENTS_TRACKER_H_
+
+#include "content/public/browser/web_contents_observer.h"
+
+namespace atom {
+
+// ChildWebContentsTracker tracks child WebContents
+// created by native `window.open()`
+class ChildWebContentsTracker : public content::WebContentsObserver {
+ public:
+  explicit ChildWebContentsTracker(content::WebContents* web_contents);
+  static bool IsChildWebContents(content::WebContents* web_contents);
+
+ protected:
+  void WebContentsDestroyed() override;
+};
+
+}  // namespace atom
+
+#endif  // ATOM_BROWSER_CHILD_WEB_CONTENTS_TRACKER_H_

atom/browser/web_contents_preferences.cc

@@ -106,6 +106,8 @@ void WebContentsPreferences::AppendExtraCommandLineSwitches(
   // integration.
   if (IsSandboxed(web_contents))
     command_line->AppendSwitch(switches::kEnableSandbox);
+  if (web_preferences.GetBoolean("nativeWindowOpen", &b) && b)
+    command_line->AppendSwitch(switches::kNativeWindowOpen);
 
   // The preload script.
   base::FilePath::StringType preload;
@@ -212,6 +214,22 @@ bool WebContentsPreferences::IsSandboxed(content::WebContents* web_contents) {
   return sandboxed;
 }
 
+bool WebContentsPreferences::UsesNativeWindowOpen(
+    content::WebContents* web_contents) {
+  WebContentsPreferences* self;
+  if (!web_contents)
+    return false;
+
+  self = FromWebContents(web_contents);
+  if (!self)
+    return false;
+
+  base::DictionaryValue& web_preferences = self->web_preferences_;
+  bool use = false;
+  web_preferences.GetBoolean("nativeWindowOpen", &use);
+  return use;
+}
+
 // static
 void WebContentsPreferences::OverrideWebkitPrefs(
     content::WebContents* web_contents, content::WebPreferences* prefs) {

atom/browser/web_contents_preferences.h

@@ -38,6 +38,7 @@ class WebContentsPreferences
       content::WebContents* web_contents, base::CommandLine* command_line);
 
   static bool IsSandboxed(content::WebContents* web_contents);
+  static bool UsesNativeWindowOpen(content::WebContents* web_contents);
 
   // Modify the WebPreferences according to |web_contents|'s preferences.
   static void OverrideWebkitPrefs(

atom/common/options_switches.cc

@@ -172,6 +172,7 @@ const char kGuestInstanceID[]  = "guest-instance-id";
 const char kOpenerID[]         = "opener-id";
 const char kScrollBounce[]     = "scroll-bounce";
 const char kHiddenPage[]       = "hidden-page";
+const char kNativeWindowOpen[] = "native-window-open";
 
 // Command switch passed to renderer process to control nodeIntegration.
 const char kNodeIntegrationInWorker[]  = "node-integration-in-worker";

atom/common/options_switches.h

@@ -92,6 +92,7 @@ extern const char kGuestInstanceID[];
 extern const char kOpenerID[];
 extern const char kScrollBounce[];
 extern const char kHiddenPage[];
+extern const char kNativeWindowOpen[];
 extern const char kNodeIntegrationInWorker[];
 
 extern const char kWidevineCdmPath[];

atom/renderer/atom_renderer_client.cc

@@ -204,6 +204,8 @@ void AtomRendererClient::SetupMainWorldOverrides(
     dict.Set(options::kOpenerID,
              command_line->GetSwitchValueASCII(switches::kOpenerID));
   dict.Set("hiddenPage", command_line->HasSwitch(switches::kHiddenPage));
+  dict.Set("nativeWindowOpen",
+           command_line->HasSwitch(switches::kNativeWindowOpen));
 
   v8::Local<v8::Value> args[] = { binding };
   ignore_result(func->Call(context, v8::Null(isolate), 1, args));

docs/api/browser-window.md

@@ -307,6 +307,7 @@ It creates a new `BrowserWindow` with native properties as set by the `options`.
       'Electron Isolated Context' entry in the combo box at the top of the
       Console tab. **Note:** This option is currently experimental and may
       change or be removed in future Electron releases.
+    * `nativeWindowOpen` Boolean (optional) - Whether to use native `window.open()`. Defaults to `false`.
 
 When setting minimum or maximum window size with `minWidth`/`maxWidth`/
 `minHeight`/`maxHeight`, it only constrains the users. It won't prevent you from

docs/api/window-open.md

@@ -45,3 +45,40 @@ has to be a field of `BrowserWindow`'s options.
 
 Sends a message to the parent window with the specified origin or `*` for no
 origin preference.
+
+### 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 choice if you need to open a dialog or a preferences window.
+
+The creation of the `BrowserWindow` is customizable in `WebContents`'s `new-window` event.
+
+```javascript
+// main process
+const mainWindow = new BrowserWindow({
+  width: 800,
+  height: 600,
+  webPreferences: {
+    nativeWindowOpen: true
+  }
+})
+mainWindow.webContents.on('new-window', (event, url, frameName, disposition, options, additionalFeatures) => {
+  if (frameName === 'modal') {
+    // open window as modal
+    event.preventDefault()
+    Object.assign(options, {
+      modal: true,
+      parent: mainWindow,
+      width: 100,
+      height: 100
+    })
+    event.newGuest = new BrowserWindow(options)
+  }
+})
+```
+
+```javascript
+// renderer process (mainWindow)
+let modal = window.open('', 'modal')
+modal.document.write('<h1>Hello</h1>')
+```

filenames.gypi

@@ -202,6 +202,8 @@
       'atom/browser/browser_mac.mm',
       'atom/browser/browser_win.cc',
       'atom/browser/browser_observer.h',
+      'atom/browser/child_web_contents_tracker.cc',
+      'atom/browser/child_web_contents_tracker.h',
       'atom/browser/common_web_contents_delegate_mac.mm',
       'atom/browser/common_web_contents_delegate_views.cc',
       'atom/browser/common_web_contents_delegate.cc',

lib/browser/guest-window-manager.js

@@ -255,6 +255,11 @@ ipcMain.on('ELECTRON_GUEST_WINDOW_MANAGER_INTERNAL_WINDOW_OPEN', function (event
   const newGuest = event.newGuest
   if ((event.sender.isGuest() && !event.sender.allowPopups) || event.defaultPrevented) {
     if (newGuest !== undefined && newGuest !== null) {
+      if (options.webContents === newGuest.webContents) {
+        // the webContents is not changed, so set defaultPrevented to false to
+        // stop the callers of this event from destroying the webContents.
+        event.defaultPrevented = false
+      }
       event.returnValue = setupGuest(event.sender, frameName, newGuest, options)
     } else {
       event.returnValue = null

lib/isolated_renderer/init.js

@@ -19,8 +19,8 @@ const ipcRenderer = {
   once () {}
 }
 
-let {guestInstanceId, hiddenPage, openerId} = binding
+let {guestInstanceId, hiddenPage, openerId, nativeWindowOpen} = binding
 if (guestInstanceId != null) guestInstanceId = parseInt(guestInstanceId)
 if (openerId != null) openerId = parseInt(openerId)
 
-require('../renderer/window-setup')(ipcRenderer, guestInstanceId, openerId, hiddenPage)
+require('../renderer/window-setup')(ipcRenderer, guestInstanceId, openerId, hiddenPage, nativeWindowOpen)

lib/renderer/override.js

@@ -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')
 
-require('./window-setup')(ipcRenderer, guestInstanceId, openerId, hiddenPage)
+require('./window-setup')(ipcRenderer, guestInstanceId, openerId, hiddenPage, usesNativeWindowOpen)

lib/renderer/window-setup.js

@@ -106,7 +106,7 @@ const getHistoryOperation = function (ipcRenderer, ...args) {
   return ipcRenderer.sendSync('ELECTRON_SYNC_NAVIGATION_CONTROLLER', ...args)
 }
 
-module.exports = (ipcRenderer, guestInstanceId, openerId, hiddenPage) => {
+module.exports = (ipcRenderer, guestInstanceId, openerId, hiddenPage, usesNativeWindowOpen) => {
   if (guestInstanceId == null) {
     // Override default window.close.
     window.close = function () {
@@ -114,16 +114,18 @@ module.exports = (ipcRenderer, guestInstanceId, openerId, hiddenPage) => {
     }
   }
 
-  // Make the browser window or guest view emit "new-window" event.
-  window.open = function (url, frameName, features) {
-    if (url != null && url !== '') {
-      url = resolveURL(url)
-    }
-    const guestId = ipcRenderer.sendSync('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_OPEN', url, toString(frameName), toString(features))
-    if (guestId != null) {
-      return getOrCreateProxy(ipcRenderer, guestId)
-    } else {
-      return null
+  if (!usesNativeWindowOpen) {
+    // Make the browser window or guest view emit "new-window" event.
+    window.open = function (url, frameName, features) {
+      if (url != null && url !== '') {
+        url = resolveURL(url)
+      }
+      const guestId = ipcRenderer.sendSync('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_OPEN', url, toString(frameName), toString(features))
+      if (guestId != null) {
+        return getOrCreateProxy(ipcRenderer, guestId)
+      } else {
+        return null
+      }
     }
   }
 

spec/api-browser-window-spec.js

@@ -1174,6 +1174,70 @@ describe('BrowserWindow module', function () {
         })
       })
     })
+
+    describe('nativeWindowOpen option', () => {
+      beforeEach(() => {
+        w.destroy()
+        w = new BrowserWindow({
+          show: false,
+          webPreferences: {
+            nativeWindowOpen: true
+          }
+        })
+      })
+
+      it('opens window of about:blank with cross-scripting enabled', (done) => {
+        ipcMain.once('answer', (event, content) => {
+          assert.equal(content, 'Hello')
+          done()
+        })
+        w.loadURL('file://' + path.join(fixtures, 'api', 'native-window-open-blank.html'))
+      })
+
+      it('opens window of same domain with cross-scripting enabled', (done) => {
+        ipcMain.once('answer', (event, content) => {
+          assert.equal(content, 'Hello')
+          done()
+        })
+        w.loadURL('file://' + path.join(fixtures, 'api', 'native-window-open-file.html'))
+      })
+
+      if (process.platform !== 'win32' || process.execPath.toLowerCase().indexOf('\\out\\d\\') === -1) {
+        it('loads native addons correctly after reload', (done) => {
+          ipcMain.once('answer', (event, content) => {
+            assert.equal(content, 'function')
+            ipcMain.once('answer', (event, content) => {
+              assert.equal(content, 'function')
+              done()
+            })
+            w.reload()
+          })
+          w.loadURL('file://' + path.join(fixtures, 'api', 'native-window-open-native-addon.html'))
+        })
+      }
+    })
+  })
+
+  describe('nativeWindowOpen + contextIsolation options', () => {
+    beforeEach(() => {
+      w.destroy()
+      w = new BrowserWindow({
+        show: false,
+        webPreferences: {
+          nativeWindowOpen: true,
+          contextIsolation: true,
+          preload: path.join(fixtures, 'api', 'native-window-open-isolated-preload.js')
+        }
+      })
+    })
+
+    it('opens window with cross-scripting enabled from isolated context', (done) => {
+      ipcMain.once('answer', (event, content) => {
+        assert.equal(content, 'Hello')
+        done()
+      })
+      w.loadURL('file://' + path.join(fixtures, 'api', 'native-window-open-isolated.html'))
+    })
   })
 
   describe('beforeunload handler', function () {

spec/fixtures/api/native-window-open-blank.html

@@ -0,0 +1,11 @@
+<html>
+<body>
+<script type="text/javascript" charset="utf-8">
+  const {ipcRenderer} = require('electron')
+  const popup = window.open()
+  popup.document.write('<h1>Hello</h1>')
+  const content = popup.document.querySelector('h1').innerText
+  ipcRenderer.send('answer', content)
+</script>
+</body>
+</html>

spec/fixtures/api/native-window-open-child.html

@@ -0,0 +1 @@
+<h1>Hello</h1>

spec/fixtures/api/native-window-open-file.html

@@ -0,0 +1,12 @@
+<html>
+<body>
+<script type="text/javascript" charset="utf-8">
+  const {ipcRenderer} = require('electron')
+  const popup = window.open('native-window-open-child.html')
+  popup.onload = () => {
+    const content = popup.document.querySelector('h1').innerText
+    ipcRenderer.send('answer', content)
+  }
+</script>
+</body>
+</html>

spec/fixtures/api/native-window-open-isolated-preload.js

@@ -0,0 +1,5 @@
+const {ipcRenderer} = require('electron')
+
+window.addEventListener('message', (event) => {
+  ipcRenderer.send('answer', event.data)
+})

spec/fixtures/api/native-window-open-isolated.html

@@ -0,0 +1,10 @@
+<html>
+<body>
+<script type="text/javascript" charset="utf-8">
+  const popup = window.open()
+  popup.document.write('<h1>Hello</h1>')
+  const content = popup.document.querySelector('h1').innerText
+  window.postMessage(content, '*')
+</script>
+</body>
+</html>

spec/fixtures/api/native-window-open-native-addon.html

@@ -0,0 +1,9 @@
+<html>
+<body>
+<script type="text/javascript" charset="utf-8">
+  const {ipcRenderer} = require('electron')
+  const runas = require('runas')
+  ipcRenderer.send('answer', typeof runas)
+</script>
+</body>
+</html>