From 3bfe073fde8cd557d01600d55a1f46cca46b2b74 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Fri, 19 Oct 2018 17:22:49 +0900 Subject: [PATCH 1/2] 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 } From 2ea8d9e6ff69ecfe393d18a9cd4d9bad39eb9181 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Fri, 19 Oct 2018 17:51:39 +0900 Subject: [PATCH 2/2] refactor: make CreateAndTake take unique_ptr --- atom/browser/api/atom_api_web_contents.cc | 49 +++++++++++++---------- atom/browser/api/atom_api_web_contents.h | 13 +++--- 2 files changed, 34 insertions(+), 28 deletions(-) diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc index f66ca8bde7cb4..96f325f3b4e4b 100644 --- a/atom/browser/api/atom_api_web_contents.cc +++ b/atom/browser/api/atom_api_web_contents.cc @@ -320,13 +320,13 @@ WebContents::WebContents(v8::Isolate* isolate, } WebContents::WebContents(v8::Isolate* isolate, - content::WebContents* web_contents, + std::unique_ptr web_contents, Type type) - : content::WebContentsObserver(web_contents), type_(type) { + : content::WebContentsObserver(web_contents.get()), type_(type) { DCHECK(type != REMOTE) << "Can't take ownership of a remote WebContents"; auto session = Session::CreateFrom(isolate, GetBrowserContext()); session_.Reset(isolate, session.ToV8()); - InitWithSessionAndOptions(isolate, web_contents, session, + InitWithSessionAndOptions(isolate, std::move(web_contents), session, mate::Dictionary::CreateEmpty(isolate)); } @@ -413,7 +413,7 @@ WebContents::WebContents(v8::Isolate* isolate, web_contents = content::WebContents::Create(params); } - InitWithSessionAndOptions(isolate, web_contents.release(), session, options); + InitWithSessionAndOptions(isolate, std::move(web_contents), session, options); } void WebContents::InitZoomController(content::WebContents* web_contents, @@ -425,16 +425,21 @@ void WebContents::InitZoomController(content::WebContents* web_contents, zoom_controller_->SetDefaultZoomFactor(zoom_factor); } -void WebContents::InitWithSessionAndOptions(v8::Isolate* isolate, - content::WebContents* web_contents, - mate::Handle session, - const mate::Dictionary& options) { - Observe(web_contents); - InitWithWebContents(web_contents, session->browser_context(), IsGuest()); +void WebContents::InitWithSessionAndOptions( + v8::Isolate* isolate, + std::unique_ptr owned_web_contents, + mate::Handle session, + const mate::Dictionary& options) { + Observe(owned_web_contents.get()); + // TODO(zcbenz): Make InitWithWebContents take unique_ptr. + // At the time of writing we are going through a refactoring and I don't want + // to make other people's work harder. + InitWithWebContents(owned_web_contents.release(), session->browser_context(), + IsGuest()); managed_web_contents()->GetView()->SetDelegate(this); - auto* prefs = web_contents->GetMutableRendererPrefs(); + auto* prefs = web_contents()->GetMutableRendererPrefs(); prefs->accept_languages = g_browser_process->GetApplicationLocale(); #if defined(OS_LINUX) || defined(OS_WIN) @@ -451,17 +456,17 @@ void WebContents::InitWithSessionAndOptions(v8::Isolate* isolate, #endif // Save the preferences in C++. - new WebContentsPreferences(web_contents, options); + new WebContentsPreferences(web_contents(), options); // Initialize permission helper. - WebContentsPermissionHelper::CreateForWebContents(web_contents); + WebContentsPermissionHelper::CreateForWebContents(web_contents()); // Initialize security state client. - SecurityStateTabHelper::CreateForWebContents(web_contents); + SecurityStateTabHelper::CreateForWebContents(web_contents()); // Initialize zoom controller. - InitZoomController(web_contents, options); + InitZoomController(web_contents(), options); - web_contents->SetUserAgentOverride(GetBrowserContext()->GetUserAgent(), - false); + web_contents()->SetUserAgentOverride(GetBrowserContext()->GetUserAgent(), + false); if (IsGuest()) { NativeWindow* owner_window = nullptr; @@ -477,7 +482,7 @@ void WebContents::InitWithSessionAndOptions(v8::Isolate* isolate, } Init(isolate); - AttachAsUserData(web_contents); + AttachAsUserData(web_contents()); } WebContents::~WebContents() { @@ -558,7 +563,7 @@ void WebContents::AddNewContents( v8::Locker locker(isolate()); v8::HandleScope handle_scope(isolate()); auto api_web_contents = - CreateAndTake(isolate(), new_contents.release(), BROWSER_WINDOW); + CreateAndTake(isolate(), std::move(new_contents), 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(), tracker->url, tracker->frame_name)) { @@ -2194,10 +2199,10 @@ mate::Handle WebContents::Create(v8::Isolate* isolate, // static mate::Handle WebContents::CreateAndTake( v8::Isolate* isolate, - content::WebContents* web_contents, + std::unique_ptr web_contents, Type type) { - return mate::CreateHandle(isolate, - new WebContents(isolate, web_contents, type)); + return mate::CreateHandle( + isolate, new WebContents(isolate, std::move(web_contents), type)); } // static diff --git a/atom/browser/api/atom_api_web_contents.h b/atom/browser/api/atom_api_web_contents.h index 40014665cb487..31115e9abdc0c 100644 --- a/atom/browser/api/atom_api_web_contents.h +++ b/atom/browser/api/atom_api_web_contents.h @@ -91,7 +91,7 @@ class WebContents : public mate::TrackableObject, // The lifetime of |web_contents| will be managed by this class. static mate::Handle CreateAndTake( v8::Isolate* isolate, - content::WebContents* web_contents, + std::unique_ptr web_contents, Type type); // Get the V8 wrapper of |web_content|, return empty handle if not wrapped. @@ -296,16 +296,17 @@ class WebContents : public mate::TrackableObject, WebContents(v8::Isolate* isolate, content::WebContents* web_contents); // Takes over ownership of |web_contents|. WebContents(v8::Isolate* isolate, - content::WebContents* web_contents, + std::unique_ptr web_contents, Type type); // Creates a new content::WebContents. WebContents(v8::Isolate* isolate, const mate::Dictionary& options); ~WebContents() override; - void InitWithSessionAndOptions(v8::Isolate* isolate, - content::WebContents* web_contents, - mate::Handle session, - const mate::Dictionary& options); + void InitWithSessionAndOptions( + v8::Isolate* isolate, + std::unique_ptr web_contents, + mate::Handle session, + const mate::Dictionary& options); // content::WebContentsDelegate: bool DidAddMessageToConsole(content::WebContents* source,