From 3bfe073fde8cd557d01600d55a1f46cca46b2b74 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Fri, 19 Oct 2018 17:22:49 +0900 Subject: [PATCH] refactor: remove -new-contents-created event Chromium expects us to take ownership of WebContents in AddNewContents, we should not create V8 wrapper in WebContentsCreated, otherwise we would have WebContents being managed by 2 unique_ptr at the same time. --- atom/browser/api/atom_api_web_contents.cc | 22 +++++++-------- atom/browser/atom_browser_client.cc | 2 +- atom/browser/child_web_contents_tracker.cc | 33 ---------------------- atom/browser/child_web_contents_tracker.h | 20 ++++++++----- filenames.gni | 1 - lib/browser/api/browser-window.js | 14 ++------- lib/browser/guest-view-manager.js | 8 ------ 7 files changed, 26 insertions(+), 74 deletions(-) delete mode 100644 atom/browser/child_web_contents_tracker.cc diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc index 80cf594058aa8..f66ca8bde7cb4 100644 --- a/atom/browser/api/atom_api_web_contents.cc +++ b/atom/browser/api/atom_api_web_contents.cc @@ -539,11 +539,10 @@ void WebContents::WebContentsCreated(content::WebContents* source_contents, const std::string& frame_name, const GURL& target_url, content::WebContents* new_contents) { - v8::Locker locker(isolate()); - v8::HandleScope handle_scope(isolate()); - // Create V8 wrapper for the |new_contents|. - auto wrapper = CreateAndTake(isolate(), new_contents, BROWSER_WINDOW); - Emit("-web-contents-created", wrapper, target_url, frame_name); + ChildWebContentsTracker::CreateForWebContents(new_contents); + auto* tracker = ChildWebContentsTracker::FromWebContents(new_contents); + tracker->url = target_url; + tracker->frame_name = frame_name; } void WebContents::AddNewContents( @@ -553,17 +552,16 @@ void WebContents::AddNewContents( const gfx::Rect& initial_rect, bool user_gesture, bool* was_blocked) { - new ChildWebContentsTracker(new_contents.get()); + auto* tracker = ChildWebContentsTracker::FromWebContents(new_contents.get()); + DCHECK(tracker); + v8::Locker locker(isolate()); v8::HandleScope handle_scope(isolate()); - // Note that the ownership of |new_contents| has already been claimed by - // the WebContentsCreated method, the release call here completes - // the ownership transfer. - auto api_web_contents = From(isolate(), new_contents.release()); - DCHECK(!api_web_contents.IsEmpty()); + auto api_web_contents = + CreateAndTake(isolate(), new_contents.release(), BROWSER_WINDOW); if (Emit("-add-new-contents", api_web_contents, disposition, user_gesture, initial_rect.x(), initial_rect.y(), initial_rect.width(), - initial_rect.height())) { + initial_rect.height(), tracker->url, tracker->frame_name)) { api_web_contents->DestroyWebContents(true /* async */); } } diff --git a/atom/browser/atom_browser_client.cc b/atom/browser/atom_browser_client.cc index 2d195818f6a63..d8cc62efcc59e 100644 --- a/atom/browser/atom_browser_client.cc +++ b/atom/browser/atom_browser_client.cc @@ -165,7 +165,7 @@ bool AtomBrowserClient::ShouldCreateNewSiteInstance( } auto* web_contents = content::WebContents::FromRenderFrameHost(render_frame_host); - if (!ChildWebContentsTracker::IsChildWebContents(web_contents)) { + if (!ChildWebContentsTracker::FromWebContents(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 diff --git a/atom/browser/child_web_contents_tracker.cc b/atom/browser/child_web_contents_tracker.cc deleted file mode 100644 index 3c9eb0d3f044e..0000000000000 --- a/atom/browser/child_web_contents_tracker.cc +++ /dev/null @@ -1,33 +0,0 @@ -// 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 - -namespace atom { - -namespace { - -std::unordered_set 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 diff --git a/atom/browser/child_web_contents_tracker.h b/atom/browser/child_web_contents_tracker.h index 785509e6ea753..64b80d5ce9c58 100644 --- a/atom/browser/child_web_contents_tracker.h +++ b/atom/browser/child_web_contents_tracker.h @@ -5,19 +5,25 @@ #ifndef ATOM_BROWSER_CHILD_WEB_CONTENTS_TRACKER_H_ #define ATOM_BROWSER_CHILD_WEB_CONTENTS_TRACKER_H_ -#include "content/public/browser/web_contents_observer.h" +#include + +#include "content/public/browser/web_contents_user_data.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); +struct ChildWebContentsTracker + : public content::WebContentsUserData { + GURL url; + std::string frame_name; + + explicit ChildWebContentsTracker(content::WebContents* web_contents) {} + + private: + friend class content::WebContentsUserData; - protected: - void WebContentsDestroyed() override; + DISALLOW_COPY_AND_ASSIGN(ChildWebContentsTracker); }; } // namespace atom diff --git a/filenames.gni b/filenames.gni index 28362d87d9dde..d17cb81d5ae41 100644 --- a/filenames.gni +++ b/filenames.gni @@ -239,7 +239,6 @@ filenames = { "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", diff --git a/lib/browser/api/browser-window.js b/lib/browser/api/browser-window.js index a563700a3792d..7fd90bb6cde70 100644 --- a/lib/browser/api/browser-window.js +++ b/lib/browser/api/browser-window.js @@ -3,7 +3,6 @@ const electron = require('electron') const { WebContentsView, TopLevelWindow } = electron const { BrowserWindow } = process.atomBinding('window') -const v8Util = process.atomBinding('v8_util') const ipcMain = require('@electron/internal/browser/ipc-main-internal') Object.setPrototypeOf(BrowserWindow.prototype, TopLevelWindow.prototype) @@ -32,25 +31,16 @@ BrowserWindow.prototype._init = function () { options, additionalFeatures, postData) }) - 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", used in sandbox and nativeWindowOpen mode this.webContents.on('-add-new-contents', (event, webContents, disposition, - userGesture, left, top, width, - height) => { - const urlFrameName = v8Util.getHiddenValue(webContents, 'url-framename') + userGesture, left, top, width, height, url, frameName) => { if ((disposition !== 'foreground-tab' && disposition !== 'new-window' && - disposition !== 'background-tab') || !urlFrameName) { + disposition !== 'background-tab')) { event.preventDefault() return } - const { url, frameName } = urlFrameName - v8Util.deleteHiddenValue(webContents, 'url-framename') const options = { show: true, x: left, diff --git a/lib/browser/guest-view-manager.js b/lib/browser/guest-view-manager.js index b47d53501e333..ec59d47f8e90d 100644 --- a/lib/browser/guest-view-manager.js +++ b/lib/browser/guest-view-manager.js @@ -157,14 +157,6 @@ const createGuest = function (embedder, params) { } } }) - guest.on('-web-contents-created', (...args) => { - if (guest.getLastWebPreferences().nativeWindowOpen === true) { - const embedder = getEmbedder(guestInstanceId) - if (embedder != null) { - embedder.emit('-web-contents-created', ...args) - } - } - }) return guestInstanceId }