From 06cc9a44fe15aabb891ddaeeb90233b9977f374d Mon Sep 17 00:00:00 2001 From: Thiago de Arruda Date: Mon, 15 Aug 2016 21:13:18 -0300 Subject: [PATCH] Add support for native chromium popups on sandboxed renderers. - Allow `api::Window` instances to be created from existing `api::WebContents`. - Override `WebContentsCreated` and `AddNewContents` to wrap renderer-created `content::WebContents` into `api::WebContents`. - For `content::WebContents` that should be displayed in new windows, pass the wrapped `api::WebContents` object to window manager. --- atom/browser/api/atom_api_web_contents.cc | 25 ++++++++++++++ atom/browser/api/atom_api_web_contents.h | 11 +++++++ atom/browser/api/atom_api_window.cc | 29 +++++++++------- atom/browser/native_window.cc | 4 +++ lib/browser/api/browser-window.js | 29 ++++++++++++++++ lib/browser/guest-window-manager.js | 40 +++++++++++++++++++++-- 6 files changed, 123 insertions(+), 15 deletions(-) diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc index 869defb6c02c..d6af1b672208 100644 --- a/atom/browser/api/atom_api_web_contents.cc +++ b/atom/browser/api/atom_api_web_contents.cc @@ -423,6 +423,31 @@ void WebContents::OnCreateWindow(const GURL& target_url, Emit("new-window", target_url, frame_name, disposition); } +void WebContents::WebContentsCreated(content::WebContents* source_contents, + int opener_render_frame_id, + const std::string& frame_name, + const GURL& target_url, + content::WebContents* new_contents) { + v8::Locker locker(isolate()); + v8::HandleScope handle_scope(isolate()); + auto api_web_contents = CreateFrom(isolate(), new_contents, BROWSER_WINDOW); + Emit("-web-contents-created", api_web_contents, target_url, frame_name); +} + +void WebContents::AddNewContents(content::WebContents* source, + content::WebContents* new_contents, + WindowOpenDisposition disposition, + const gfx::Rect& initial_rect, + bool user_gesture, + bool* was_blocked) { + v8::Locker locker(isolate()); + v8::HandleScope handle_scope(isolate()); + auto api_web_contents = CreateFrom(isolate(), new_contents); + Emit("-add-new-contents", api_web_contents, disposition, user_gesture, + initial_rect.x(), initial_rect.y(), initial_rect.width(), + initial_rect.height()); +} + content::WebContents* WebContents::OpenURLFromTab( content::WebContents* source, const content::OpenURLParams& params) { diff --git a/atom/browser/api/atom_api_web_contents.h b/atom/browser/api/atom_api_web_contents.h index e47bb06fb309..cfc3ccc4ab26 100644 --- a/atom/browser/api/atom_api_web_contents.h +++ b/atom/browser/api/atom_api_web_contents.h @@ -210,6 +210,17 @@ class WebContents : public mate::TrackableObject, const base::string16& message, int32_t line_no, const base::string16& source_id) override; + void WebContentsCreated(content::WebContents* source_contents, + int opener_render_frame_id, + const std::string& frame_name, + const GURL& target_url, + content::WebContents* new_contents) override; + void AddNewContents(content::WebContents* source, + content::WebContents* new_contents, + WindowOpenDisposition disposition, + const gfx::Rect& initial_rect, + bool user_gesture, + bool* was_blocked) override; content::WebContents* OpenURLFromTab( content::WebContents* source, const content::OpenURLParams& params) override; diff --git a/atom/browser/api/atom_api_window.cc b/atom/browser/api/atom_api_window.cc index d16200086402..7c83e558fffa 100644 --- a/atom/browser/api/atom_api_window.cc +++ b/atom/browser/api/atom_api_window.cc @@ -72,20 +72,25 @@ v8::Local ToBuffer(v8::Isolate* isolate, void* val, int size) { Window::Window(v8::Isolate* isolate, v8::Local wrapper, const mate::Dictionary& options) { - // Use options.webPreferences to create WebContents. - mate::Dictionary web_preferences = mate::Dictionary::CreateEmpty(isolate); - options.Get(options::kWebPreferences, &web_preferences); + mate::Handle web_contents; + // If no WebContents was passed to the constructor, create it from options. + if (!options.Get("webContents", &web_contents)) { + // Use options.webPreferences to create WebContents. + mate::Dictionary web_preferences = mate::Dictionary::CreateEmpty(isolate); + options.Get(options::kWebPreferences, &web_preferences); - // Copy the backgroundColor to webContents. - v8::Local value; - if (options.Get(options::kBackgroundColor, &value)) - web_preferences.Set(options::kBackgroundColor, value); + // Copy the backgroundColor to webContents. + v8::Local value; + if (options.Get(options::kBackgroundColor, &value)) + web_preferences.Set(options::kBackgroundColor, value); - v8::Local transparent; - if (options.Get("transparent", &transparent)) - web_preferences.Set("transparent", transparent); - // Creates the WebContents used by BrowserWindow. - auto web_contents = WebContents::Create(isolate, web_preferences); + v8::Local transparent; + if (options.Get("transparent", &transparent)) + web_preferences.Set("transparent", transparent); + + // Creates the WebContents used by BrowserWindow. + web_contents = WebContents::Create(isolate, web_preferences); + } Init(isolate, wrapper, options, web_contents); } diff --git a/atom/browser/native_window.cc b/atom/browser/native_window.cc index 728053323754..1cf5a23b6efe 100644 --- a/atom/browser/native_window.cc +++ b/atom/browser/native_window.cc @@ -391,6 +391,10 @@ void NativeWindow::RequestToClosePage() { if (window_unresposive_closure_.IsCancelled()) ScheduleUnresponsiveEvent(5000); + if (!web_contents()) + // Already closed by renderer + return; + if (web_contents()->NeedToFireBeforeUnload()) web_contents()->DispatchBeforeUnload(); else diff --git a/lib/browser/api/browser-window.js b/lib/browser/api/browser-window.js index ecfdda05f8db..920d2404432e 100644 --- a/lib/browser/api/browser-window.js +++ b/lib/browser/api/browser-window.js @@ -3,6 +3,7 @@ const {ipcMain} = require('electron') const {EventEmitter} = require('events') const {BrowserWindow} = process.atomBinding('window') +const v8Util = process.atomBinding('v8_util') Object.setPrototypeOf(BrowserWindow.prototype, EventEmitter.prototype) @@ -26,6 +27,34 @@ BrowserWindow.prototype._init = function () { ipcMain.emit('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_OPEN', event, url, frameName, disposition, options) }) + this.webContents.on('-web-contents-created', (event, webContents, url, + frameName) => { + v8Util.setHiddenValue(webContents, 'url-framename', {url, frameName}) + }) + // Create a new browser window for the native implementation of + // "window.open"(sandbox mode only) + this.webContents.on('-add-new-contents', (event, webContents, disposition, + userGesture, left, top, width, + height) => { + let urlFrameName = v8Util.getHiddenValue(webContents, 'url-framename') + if ((disposition !== 'foreground-tab' && disposition !== 'new-window') || + !urlFrameName) { + return + } + + let {url, frameName} = urlFrameName + v8Util.deleteHiddenValue(webContents, 'url-framename') + const options = { + show: true, + x: left, + y: top, + width: width || 800, + height: height || 600, + webContents: webContents + } + ipcMain.emit('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_OPEN', event, url, frameName, disposition, options) + }) + // window.resizeTo(...) // window.moveTo(...) this.webContents.on('move', (event, size) => { diff --git a/lib/browser/guest-window-manager.js b/lib/browser/guest-window-manager.js index 776ab91fec96..f20f1bbb0ab9 100644 --- a/lib/browser/guest-window-manager.js +++ b/lib/browser/guest-window-manager.js @@ -57,7 +57,30 @@ const createGuest = function (embedder, url, frameName, options) { } options.webPreferences.openerId = embedder.id guest = new BrowserWindow(options) - guest.loadURL(url) + 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('

hello

') + // + // 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. + guest.loadURL(url) + } // When |embedder| is destroyed we should also destroy attached guest, and if // guest is closed by user then we should prevent |embedder| from double @@ -72,8 +95,19 @@ const createGuest = function (embedder, url, frameName, options) { embedder.send('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_CLOSED_' + guestId) embedder.removeListener('render-view-deleted', closedByEmbedder) } - embedder.once('render-view-deleted', closedByEmbedder) - guest.once('closed', closedByUser) + if (!options.webPreferences.sandbox) { + // These events should only be handled when the guest window is opened by a + // non-sandboxed renderer for two reasons: + // + // - `render-view-deleted` is emitted when the popup is closed by the user, + // and that will eventually result in NativeWindow::NotifyWindowClosed + // using a dangling pointer since `destroy()` would have been called by + // `closeByEmbedded` + // - No need to emit `ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_CLOSED_` since + // there's no renderer code listening to it., + embedder.once('render-view-deleted', closedByEmbedder) + guest.once('closed', closedByUser) + } if (frameName) { frameToGuest[frameName] = guest