Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: remove potential double free when managing WebContents #15280

Merged
merged 2 commits into from Oct 22, 2018
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -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,
content::WebContents* web_contents,
mate::Handle<api::Session> session,
const mate::Dictionary& options) {
Observe(web_contents);
InitWithWebContents(web_contents, session->browser_context(), IsGuest());
void WebContents::InitWithSessionAndOptions(
v8::Isolate* isolate,
std::unique_ptr<content::WebContents> owned_web_contents,
mate::Handle<api::Session> 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(),

This comment has been minimized.

Copy link
@deepak1556

deepak1556 Oct 22, 2018

Member

When fixing this, can you also move some of the CreateForWebContents there, would simply api layer.

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() {
@@ -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());
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 +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
// 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(), 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())) {
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,
new WebContents(isolate, web_contents, type));
return mate::CreateHandle(
isolate, new WebContents(isolate, std::move(web_contents), type));
}

// static
@@ -91,7 +91,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.
@@ -296,16 +296,17 @@ 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,
content::WebContents* web_contents,
mate::Handle<class Session> session,
const mate::Dictionary& options);
void InitWithSessionAndOptions(
v8::Isolate* isolate,
std::unique_ptr<content::WebContents> web_contents,
mate::Handle<class Session> session,
const mate::Dictionary& options);

// content::WebContentsDelegate:
bool DidAddMessageToConsole(content::WebContents* source,
@@ -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

This file was deleted.

@@ -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 {
public:
explicit ChildWebContentsTracker(content::WebContents* web_contents);
static bool IsChildWebContents(content::WebContents* web_contents);
struct ChildWebContentsTracker
: public content::WebContentsUserData<ChildWebContentsTracker> {
GURL url;
std::string frame_name;

explicit ChildWebContentsTracker(content::WebContents* web_contents) {}

private:
friend class content::WebContentsUserData<ChildWebContentsTracker>;

protected:
void WebContentsDestroyed() override;
DISALLOW_COPY_AND_ASSIGN(ChildWebContentsTracker);
};

} // namespace atom
@@ -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",
@@ -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,
@@ -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
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.