refactor: remove potential double free when managing WebContents (#15280)

* 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.

* refactor: make CreateAndTake take unique_ptr

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<content::WebContents> 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,
+void WebContents::InitWithSessionAndOptions(
-                                            content::WebContents* web_contents,
+    v8::Isolate* isolate,
+    std::unique_ptr<content::WebContents> owned_web_contents,
     mate::Handle<api::Session> session,
     const mate::Dictionary& options) {
-  Observe(web_contents);
+  Observe(owned_web_contents.get());
-  InitWithWebContents(web_contents, session->browser_context(), IsGuest());
+  // 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,16 +456,16 @@ 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(),
+  web_contents()->SetUserAgentOverride(GetBrowserContext()->GetUserAgent(),
                                        false);
 
   if (IsGuest()) {
@@ -477,7 +482,7 @@ void WebContents::InitWithSessionAndOptions(v8::Isolate* isolate,
   }
 
   Init(isolate);
-  AttachAsUserData(web_contents);
+  AttachAsUserData(web_contents());
 }
 
 WebContents::~WebContents() {
@@ -539,11 +544,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());
+  ChildWebContentsTracker::CreateForWebContents(new_contents);
-  v8::HandleScope handle_scope(isolate());
+  auto* tracker = ChildWebContentsTracker::FromWebContents(new_contents);
-  // Create V8 wrapper for the |new_contents|.
+  tracker->url = target_url;
-  auto wrapper = CreateAndTake(isolate(), new_contents, BROWSER_WINDOW);
+  tracker->frame_name = frame_name;
-  Emit("-web-contents-created", wrapper, target_url, frame_name);
 }
 
 void WebContents::AddNewContents(
@@ -553,17 +557,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
+  auto api_web_contents =
-  // the WebContentsCreated method, the release call here completes
+      CreateAndTake(isolate(), std::move(new_contents), BROWSER_WINDOW);
-  // the ownership transfer.
-  auto api_web_contents = From(isolate(), new_contents.release());
-  DCHECK(!api_web_contents.IsEmpty());
   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 */);
   }
 }
@@ -2196,10 +2199,10 @@ mate::Handle<WebContents> WebContents::Create(v8::Isolate* isolate,
 // static
 mate::Handle<WebContents> WebContents::CreateAndTake(
     v8::Isolate* isolate,
-    content::WebContents* web_contents,
+    std::unique_ptr<content::WebContents> web_contents,
     Type type) {
-  return mate::CreateHandle(isolate,
+  return mate::CreateHandle(
-                            new WebContents(isolate, web_contents, type));
+      isolate, new WebContents(isolate, std::move(web_contents), type));
 }
 
 // static

atom/browser/api/atom_api_web_contents.h

@@ -88,7 +88,7 @@ class WebContents : public mate::TrackableObject<WebContents>,
   // The lifetime of |web_contents| will be managed by this class.
   static mate::Handle<WebContents> CreateAndTake(
       v8::Isolate* isolate,
-      content::WebContents* web_contents,
+      std::unique_ptr<content::WebContents> web_contents,
       Type type);
 
   // Get the V8 wrapper of |web_content|, return empty handle if not wrapped.
@@ -293,14 +293,15 @@ class WebContents : public mate::TrackableObject<WebContents>,
   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<content::WebContents> web_contents,
               Type type);
   // Creates a new content::WebContents.
   WebContents(v8::Isolate* isolate, const mate::Dictionary& options);
   ~WebContents() override;
 
-  void InitWithSessionAndOptions(v8::Isolate* isolate,
+  void InitWithSessionAndOptions(
-                                 content::WebContents* web_contents,
+      v8::Isolate* isolate,
+      std::unique_ptr<content::WebContents> web_contents,
       mate::Handle<class Session> session,
       const mate::Dictionary& options);
 

atom/browser/atom_browser_client.cc

@@ -167,7 +167,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

atom/browser/child_web_contents_tracker.cc

@@ -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 <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

@@ -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 <string>
+
+#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 {
+struct ChildWebContentsTracker
- public:
+    : public content::WebContentsUserData<ChildWebContentsTracker> {
-  explicit ChildWebContentsTracker(content::WebContents* web_contents);
+  GURL url;
-  static bool IsChildWebContents(content::WebContents* web_contents);
+  std::string frame_name;
 
- protected:
+  explicit ChildWebContentsTracker(content::WebContents* web_contents) {}
-  void WebContentsDestroyed() override;
+
+ private:
+  friend class content::WebContentsUserData<ChildWebContentsTracker>;
+
+  DISALLOW_COPY_AND_ASSIGN(ChildWebContentsTracker);
 };
 
 }  // namespace atom

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",

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,
+    userGesture, left, top, width, height, url, frameName) => {
-    height) => {
-    const urlFrameName = v8Util.getHiddenValue(webContents, '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,

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
 }