From fd4a0626c59b40a6dd782109a58fea90486c9496 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Wed, 16 May 2018 19:29:44 +0900 Subject: [PATCH 1/6] destroy WebContents when view is destroyed --- atom/browser/api/atom_api_view.h | 4 +-- .../browser/api/atom_api_web_contents_view.cc | 26 ++++++++++++------- atom/browser/api/atom_api_web_contents_view.h | 18 ++++++------- 3 files changed, 28 insertions(+), 20 deletions(-) diff --git a/atom/browser/api/atom_api_view.h b/atom/browser/api/atom_api_view.h index b1fb38e4c439..4e851ef28983 100644 --- a/atom/browser/api/atom_api_view.h +++ b/atom/browser/api/atom_api_view.h @@ -7,14 +7,14 @@ #include -#include "atom/browser/api/event_emitter.h" +#include "atom/browser/api/trackable_object.h" #include "ui/views/view.h" namespace atom { namespace api { -class View : public mate::EventEmitter { +class View : public mate::TrackableObject { public: static mate::WrappableBase* New(mate::Arguments* args); diff --git a/atom/browser/api/atom_api_web_contents_view.cc b/atom/browser/api/atom_api_web_contents_view.cc index 5bfd94dfee94..2b9443ba2a41 100644 --- a/atom/browser/api/atom_api_web_contents_view.cc +++ b/atom/browser/api/atom_api_web_contents_view.cc @@ -18,16 +18,16 @@ namespace atom { namespace api { -WebContentsView::WebContentsView( - v8::Isolate* isolate, - v8::Local web_contents_wrapper, - brightray::InspectableWebContents* web_contents) +WebContentsView::WebContentsView(v8::Isolate* isolate, + mate::Handle web_contents, + brightray::InspectableWebContents* iwc) #if defined(OS_MACOSX) - : View(new DelayedNativeViewHost(web_contents->GetView()->GetNativeView())), + : View(new DelayedNativeViewHost(iwc->GetView()->GetNativeView())), #else - : View(web_contents->GetView()->GetView()), + : View(iwc->GetView()->GetView()), #endif - web_contents_wrapper_(isolate, web_contents_wrapper) { + web_contents_(isolate, web_contents->GetWrapper()), + api_web_contents_(web_contents.get()) { #if defined(OS_MACOSX) // On macOS a View is created to wrap the NSView, and its lifetime is managed // by us. @@ -36,9 +36,17 @@ WebContentsView::WebContentsView( // On other platforms the View is managed by InspectableWebContents. set_delete_view(false); #endif + api_web_contents_->AddObserver(this); } -WebContentsView::~WebContentsView() {} +WebContentsView::~WebContentsView() { + api_web_contents_->RemoveObserver(this); + api_web_contents_->DestroyWebContents(false /* async */); +} + +void WebContentsView::OnCloseContents() { + // TODO(zcbenz): WebContents is closed, report the event. +} // static mate::WrappableBase* WebContentsView::New( @@ -50,7 +58,7 @@ mate::WrappableBase* WebContentsView::New( v8::Exception::Error(mate::StringToV8(args->isolate(), error))); return nullptr; } - auto* view = new WebContentsView(args->isolate(), web_contents->GetWrapper(), + auto* view = new WebContentsView(args->isolate(), web_contents, web_contents->managed_web_contents()); view->InitWith(args->isolate(), args->GetThis()); return view; diff --git a/atom/browser/api/atom_api_web_contents_view.h b/atom/browser/api/atom_api_web_contents_view.h index e3cf96f99085..c29657d870f9 100644 --- a/atom/browser/api/atom_api_web_contents_view.h +++ b/atom/browser/api/atom_api_web_contents_view.h @@ -6,11 +6,7 @@ #define ATOM_BROWSER_API_ATOM_API_WEB_CONTENTS_VIEW_H_ #include "atom/browser/api/atom_api_view.h" -#include "native_mate/handle.h" - -namespace brightray { -class InspectableWebContents; -} +#include "atom/browser/api/atom_api_web_contents.h" namespace atom { @@ -18,7 +14,7 @@ namespace api { class WebContents; -class WebContentsView : public View { +class WebContentsView : public View, public ExtendedWebContentsObserver { public: static mate::WrappableBase* New(mate::Arguments* args, mate::Handle web_contents); @@ -28,13 +24,17 @@ class WebContentsView : public View { protected: WebContentsView(v8::Isolate* isolate, - v8::Local web_contents_wrapper, - brightray::InspectableWebContents* web_contents); + mate::Handle web_contents, + brightray::InspectableWebContents* iwc); ~WebContentsView() override; + // ExtendedWebContentsObserver: + void OnCloseContents() override; + private: // Keep a reference to v8 wrapper. - v8::Global web_contents_wrapper_; + v8::Global web_contents_; + api::WebContents* api_web_contents_; DISALLOW_COPY_AND_ASSIGN(WebContentsView); }; From aeeb2a259f1d28ef6d9a1f89f177c9f230cab888 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Thu, 17 May 2018 09:09:45 +0900 Subject: [PATCH 2/6] destruct objects in stack order --- atom/browser/atom_browser_main_parts.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/atom/browser/atom_browser_main_parts.cc b/atom/browser/atom_browser_main_parts.cc index 8cccac09931f..6c071a318b5f 100644 --- a/atom/browser/atom_browser_main_parts.cc +++ b/atom/browser/atom_browser_main_parts.cc @@ -121,7 +121,10 @@ int AtomBrowserMainParts::GetExitCode() { void AtomBrowserMainParts::RegisterDestructionCallback( base::OnceClosure callback) { - destructors_.insert(destructors_.end(), std::move(callback)); + // The destructors should be called in reversed order, so dependencies between + // JavaScript objects can be correctly resolved. + // For example WebContentsView => WebContents => Session. + destructors_.insert(destructors_.begin(), std::move(callback)); } void AtomBrowserMainParts::PreEarlyInitialization() { From 51db1efb8a31888e501ea3d12f3c6b183e31a6f9 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Thu, 17 May 2018 17:01:56 +0900 Subject: [PATCH 3/6] prevent double-adding a WebContents to view --- .../browser/api/atom_api_web_contents_view.cc | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/atom/browser/api/atom_api_web_contents_view.cc b/atom/browser/api/atom_api_web_contents_view.cc index 2b9443ba2a41..a96f4a9c8d6c 100644 --- a/atom/browser/api/atom_api_web_contents_view.cc +++ b/atom/browser/api/atom_api_web_contents_view.cc @@ -6,6 +6,7 @@ #include "atom/browser/api/atom_api_web_contents.h" #include "brightray/browser/inspectable_web_contents_view.h" +#include "content/public/browser/web_contents_user_data.h" #include "native_mate/dictionary.h" #if defined(OS_MACOSX) @@ -14,6 +15,27 @@ #include "atom/common/node_includes.h" +namespace { + +// Used to indicate whether a WebContents already has a view. +class WebContentsViewRelay + : public content::WebContentsUserData { + public: + ~WebContentsViewRelay() override {} + + private: + explicit WebContentsViewRelay(content::WebContents* contents) {} + friend class content::WebContentsUserData; + + atom::api::WebContentsView* view_ = nullptr; + + DISALLOW_COPY_AND_ASSIGN(WebContentsViewRelay); +}; + +} // namespace + +DEFINE_WEB_CONTENTS_USER_DATA_KEY(WebContentsViewRelay); + namespace atom { namespace api { @@ -36,6 +58,7 @@ WebContentsView::WebContentsView(v8::Isolate* isolate, // On other platforms the View is managed by InspectableWebContents. set_delete_view(false); #endif + WebContentsViewRelay::CreateForWebContents(web_contents->web_contents()); api_web_contents_->AddObserver(this); } @@ -52,12 +75,23 @@ void WebContentsView::OnCloseContents() { mate::WrappableBase* WebContentsView::New( mate::Arguments* args, mate::Handle web_contents) { + // Currently we only support InspectableWebContents, e.g. the WebContents + // created by users directly. To support devToolsWebContents we need to create + // a wrapper view. if (!web_contents->managed_web_contents()) { const char* error = "The WebContents must be created by user"; args->isolate()->ThrowException( v8::Exception::Error(mate::StringToV8(args->isolate(), error))); return nullptr; } + // Check if the WebContents has already been added to a view. + if (WebContentsViewRelay::FromWebContents(web_contents->web_contents())) { + const char* error = "The WebContents has already been added to a View"; + args->isolate()->ThrowException( + v8::Exception::Error(mate::StringToV8(args->isolate(), error))); + return nullptr; + } + // Constructor call. auto* view = new WebContentsView(args->isolate(), web_contents, web_contents->managed_web_contents()); view->InitWith(args->isolate(), args->GetThis()); From 3b81312cf7357837302811683a41da72a0fdf003 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Fri, 18 May 2018 16:31:43 +0900 Subject: [PATCH 4/6] clear pointer when WebContents is closed --- atom/browser/api/atom_api_web_contents_view.cc | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/atom/browser/api/atom_api_web_contents_view.cc b/atom/browser/api/atom_api_web_contents_view.cc index a96f4a9c8d6c..e0fb14d16d81 100644 --- a/atom/browser/api/atom_api_web_contents_view.cc +++ b/atom/browser/api/atom_api_web_contents_view.cc @@ -63,12 +63,15 @@ WebContentsView::WebContentsView(v8::Isolate* isolate, } WebContentsView::~WebContentsView() { - api_web_contents_->RemoveObserver(this); - api_web_contents_->DestroyWebContents(false /* async */); + if (api_web_contents_) { + api_web_contents_->RemoveObserver(this); + api_web_contents_->DestroyWebContents(false /* async */); + } } void WebContentsView::OnCloseContents() { - // TODO(zcbenz): WebContents is closed, report the event. + api_web_contents_ = nullptr; + web_contents_.Reset(); } // static From 300c7a4b0466a00954b38d9e8ce404b6fb65f243 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Fri, 18 May 2018 16:36:43 +0900 Subject: [PATCH 5/6] add tests for View and WebContentsView --- spec/api-view-spec.js | 14 ++++++++++++++ spec/api-web-contents-view-spec.js | 25 +++++++++++++++++++++++++ 2 files changed, 39 insertions(+) create mode 100644 spec/api-view-spec.js create mode 100644 spec/api-web-contents-view-spec.js diff --git a/spec/api-view-spec.js b/spec/api-view-spec.js new file mode 100644 index 000000000000..a47283b124d5 --- /dev/null +++ b/spec/api-view-spec.js @@ -0,0 +1,14 @@ +'use strict' + +const {closeWindow} = require('./window-helpers') +const {TopLevelWindow, View} = require('electron').remote + +describe('View', () => { + let w = null + afterEach(() => closeWindow(w).then(() => { w = null })) + + it('can be used as content view', () => { + w = new TopLevelWindow({show: false}) + w.setContentView(new View()) + }) +}) diff --git a/spec/api-web-contents-view-spec.js b/spec/api-web-contents-view-spec.js new file mode 100644 index 000000000000..b61ddb616386 --- /dev/null +++ b/spec/api-web-contents-view-spec.js @@ -0,0 +1,25 @@ +'use strict' + +const assert = require('assert') +const {closeWindow} = require('./window-helpers') +const {webContents, TopLevelWindow, WebContentsView} = require('electron').remote + +describe('WebContentsView', () => { + let w = null + afterEach(() => closeWindow(w).then(() => { w = null })) + + it('can be used as content view', () => { + const web = webContents.create({}) + w = new TopLevelWindow({show: false}) + w.setContentView(new WebContentsView(web)) + }) + + it('prevents adding same WebContents', () => { + const web = webContents.create({}) + w = new TopLevelWindow({show: false}) + w.setContentView(new WebContentsView(web)) + assert.throws(() => { + w.setContentView(new WebContentsView(web)) + }, /The WebContents has already been added to a View/) + }) +}) From 595b0663b23438327a9067c7fa9e701733f05cb3 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Mon, 21 May 2018 10:50:26 +0900 Subject: [PATCH 6/6] WebContents may be managed by multiple owners --- atom/browser/api/atom_api_web_contents_view.cc | 8 +++----- atom/browser/api/atom_api_web_contents_view.h | 13 +++++++++---- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/atom/browser/api/atom_api_web_contents_view.cc b/atom/browser/api/atom_api_web_contents_view.cc index e0fb14d16d81..de27f8eeed64 100644 --- a/atom/browser/api/atom_api_web_contents_view.cc +++ b/atom/browser/api/atom_api_web_contents_view.cc @@ -59,17 +59,15 @@ WebContentsView::WebContentsView(v8::Isolate* isolate, set_delete_view(false); #endif WebContentsViewRelay::CreateForWebContents(web_contents->web_contents()); - api_web_contents_->AddObserver(this); + Observe(web_contents->web_contents()); } WebContentsView::~WebContentsView() { - if (api_web_contents_) { - api_web_contents_->RemoveObserver(this); + if (api_web_contents_) api_web_contents_->DestroyWebContents(false /* async */); - } } -void WebContentsView::OnCloseContents() { +void WebContentsView::WebContentsDestroyed() { api_web_contents_ = nullptr; web_contents_.Reset(); } diff --git a/atom/browser/api/atom_api_web_contents_view.h b/atom/browser/api/atom_api_web_contents_view.h index c29657d870f9..d502918a155a 100644 --- a/atom/browser/api/atom_api_web_contents_view.h +++ b/atom/browser/api/atom_api_web_contents_view.h @@ -6,7 +6,12 @@ #define ATOM_BROWSER_API_ATOM_API_WEB_CONTENTS_VIEW_H_ #include "atom/browser/api/atom_api_view.h" -#include "atom/browser/api/atom_api_web_contents.h" +#include "content/public/browser/web_contents_observer.h" +#include "native_mate/handle.h" + +namespace brightray { +class InspectableWebContents; +} namespace atom { @@ -14,7 +19,7 @@ namespace api { class WebContents; -class WebContentsView : public View, public ExtendedWebContentsObserver { +class WebContentsView : public View, public content::WebContentsObserver { public: static mate::WrappableBase* New(mate::Arguments* args, mate::Handle web_contents); @@ -28,8 +33,8 @@ class WebContentsView : public View, public ExtendedWebContentsObserver { brightray::InspectableWebContents* iwc); ~WebContentsView() override; - // ExtendedWebContentsObserver: - void OnCloseContents() override; + // content::WebContentsObserver: + void WebContentsDestroyed() override; private: // Keep a reference to v8 wrapper.