Merge pull request #12995 from electron/web-contents-view-tests

Fix crashes and add tests for View API

atom/browser/api/atom_api_view.h

@@ -7,14 +7,14 @@
 
 #include <memory>
 
-#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<View> {
+class View : public mate::TrackableObject<View> {
  public:
   static mate::WrappableBase* New(mate::Arguments* args);
 

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,20 +15,41 @@
 
 #include "atom/common/node_includes.h"
 
+namespace {
+
+// Used to indicate whether a WebContents already has a view.
+class WebContentsViewRelay
+    : public content::WebContentsUserData<WebContentsViewRelay> {
+ public:
+  ~WebContentsViewRelay() override {}
+
+ private:
+  explicit WebContentsViewRelay(content::WebContents* contents) {}
+  friend class content::WebContentsUserData<WebContentsViewRelay>;
+
+  atom::api::WebContentsView* view_ = nullptr;
+
+  DISALLOW_COPY_AND_ASSIGN(WebContentsViewRelay);
+};
+
+}  // namespace
+
+DEFINE_WEB_CONTENTS_USER_DATA_KEY(WebContentsViewRelay);
+
 namespace atom {
 
 namespace api {
 
-WebContentsView::WebContentsView(
+WebContentsView::WebContentsView(v8::Isolate* isolate,
-    v8::Isolate* isolate,
+                                 mate::Handle<WebContents> web_contents,
-    v8::Local<v8::Value> web_contents_wrapper,
+                                 brightray::InspectableWebContents* iwc)
-    brightray::InspectableWebContents* web_contents)
 #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,21 +58,42 @@ WebContentsView::WebContentsView(
   // On other platforms the View is managed by InspectableWebContents.
   set_delete_view(false);
 #endif
+  WebContentsViewRelay::CreateForWebContents(web_contents->web_contents());
+  Observe(web_contents->web_contents());
 }
 
-WebContentsView::~WebContentsView() {}
+WebContentsView::~WebContentsView() {
+  if (api_web_contents_)
+    api_web_contents_->DestroyWebContents(false /* async */);
+}
+
+void WebContentsView::WebContentsDestroyed() {
+  api_web_contents_ = nullptr;
+  web_contents_.Reset();
+}
 
 // static
 mate::WrappableBase* WebContentsView::New(
     mate::Arguments* args,
     mate::Handle<WebContents> 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;
   }
-  auto* view = new WebContentsView(args->isolate(), web_contents->GetWrapper(),
+  // 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());
   return view;

atom/browser/api/atom_api_web_contents_view.h

@@ -6,6 +6,7 @@
 #define ATOM_BROWSER_API_ATOM_API_WEB_CONTENTS_VIEW_H_
 
 #include "atom/browser/api/atom_api_view.h"
+#include "content/public/browser/web_contents_observer.h"
 #include "native_mate/handle.h"
 
 namespace brightray {
@@ -18,7 +19,7 @@ namespace api {
 
 class WebContents;
 
-class WebContentsView : public View {
+class WebContentsView : public View, public content::WebContentsObserver {
  public:
   static mate::WrappableBase* New(mate::Arguments* args,
                                   mate::Handle<WebContents> web_contents);
@@ -28,13 +29,17 @@ class WebContentsView : public View {
 
  protected:
   WebContentsView(v8::Isolate* isolate,
-                  v8::Local<v8::Value> web_contents_wrapper,
+                  mate::Handle<WebContents> web_contents,
-                  brightray::InspectableWebContents* web_contents);
+                  brightray::InspectableWebContents* iwc);
   ~WebContentsView() override;
 
+  // content::WebContentsObserver:
+  void WebContentsDestroyed() override;
+
  private:
   // Keep a reference to v8 wrapper.
-  v8::Global<v8::Value> web_contents_wrapper_;
+  v8::Global<v8::Value> web_contents_;
+  api::WebContents* api_web_contents_;
 
   DISALLOW_COPY_AND_ASSIGN(WebContentsView);
 };

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() {

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())
+  })
+})

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/)
+  })
+})