Browse Source

fix: prevent crash if BrowserView webContents was destroyed (#25112)

Shelley Vohr 4 years ago
parent
commit
c8a0b2b71d

+ 0 - 10
docs/api/browser-view.md

@@ -40,16 +40,6 @@ A [`WebContents`](web-contents.md) object owned by this view.
 
 Objects created with `new BrowserView` have the following instance methods:
 
-#### `view.destroy()`
-
-Force closing the view, the `unload` and `beforeunload` events won't be emitted
-for the web page. After you're done with a view, call this function in order to
-free memory and other resources as soon as possible.
-
-#### `view.isDestroyed()`
-
-Returns `Boolean` - Whether the view is destroyed.
-
 #### `view.setAutoResize(options)` _Experimental_
 
 * `options` Object

+ 6 - 4
shell/browser/api/electron_api_base_window.cc

@@ -745,7 +745,8 @@ void BaseWindow::AddBrowserView(v8::Local<v8::Value> value) {
     auto get_that_view = browser_views_.find(browser_view->ID());
     if (get_that_view == browser_views_.end()) {
       window_->AddBrowserView(browser_view->view());
-      browser_view->web_contents()->SetOwnerWindow(window_.get());
+      if (browser_view->web_contents())
+        browser_view->web_contents()->SetOwnerWindow(window_.get());
       browser_views_[browser_view->ID()].Reset(isolate(), value);
     }
   }
@@ -758,8 +759,8 @@ void BaseWindow::RemoveBrowserView(v8::Local<v8::Value> value) {
     auto get_that_view = browser_views_.find(browser_view->ID());
     if (get_that_view != browser_views_.end()) {
       window_->RemoveBrowserView(browser_view->view());
-      browser_view->web_contents()->SetOwnerWindow(nullptr);
-
+      if (browser_view->web_contents())
+        browser_view->web_contents()->SetOwnerWindow(nullptr);
       (*get_that_view).second.Reset(isolate(), value);
       browser_views_.erase(get_that_view);
     }
@@ -1055,7 +1056,8 @@ void BaseWindow::ResetBrowserViews() {
                            &browser_view) &&
         !browser_view.IsEmpty()) {
       window_->RemoveBrowserView(browser_view->view());
-      browser_view->web_contents()->SetOwnerWindow(nullptr);
+      if (browser_view->web_contents())
+        browser_view->web_contents()->SetOwnerWindow(nullptr);
     }
 
     item.second.Reset();

+ 2 - 1
shell/browser/api/electron_api_web_contents.cc

@@ -658,7 +658,8 @@ WebContents::~WebContents() {
     } else {
       // Destroy WebContents asynchronously unless app is shutting down,
       // because destroy() might be called inside WebContents's event handler.
-      DestroyWebContents(!IsGuest() /* async */);
+      bool is_browser_view = type_ == Type::BROWSER_VIEW;
+      DestroyWebContents(!(IsGuest() || is_browser_view) /* async */);
       // The WebContentsDestroyed will not be called automatically because we
       // destroy the webContents in the next tick. So we have to manually
       // call it here to make sure "destroyed" event is emitted.

+ 12 - 8
shell/browser/native_window_mac.mm

@@ -1275,12 +1275,15 @@ void NativeWindowMac::AddBrowserView(NativeBrowserView* view) {
   }
 
   add_browser_view(view);
-  auto* native_view =
-      view->GetInspectableWebContentsView()->GetNativeView().GetNativeNSView();
-  [[window_ contentView] addSubview:native_view
-                         positioned:NSWindowAbove
-                         relativeTo:nil];
-  native_view.hidden = NO;
+  if (view->GetInspectableWebContentsView()) {
+    auto* native_view = view->GetInspectableWebContentsView()
+                            ->GetNativeView()
+                            .GetNativeNSView();
+    [[window_ contentView] addSubview:native_view
+                           positioned:NSWindowAbove
+                           relativeTo:nil];
+    native_view.hidden = NO;
+  }
 
   [CATransaction commit];
 }
@@ -1294,8 +1297,9 @@ void NativeWindowMac::RemoveBrowserView(NativeBrowserView* view) {
     return;
   }
 
-  [view->GetInspectableWebContentsView()->GetNativeView().GetNativeNSView()
-      removeFromSuperview];
+  if (view->GetInspectableWebContentsView())
+    [view->GetInspectableWebContentsView()->GetNativeView().GetNativeNSView()
+        removeFromSuperview];
   remove_browser_view(view);
 
   [CATransaction commit];

+ 6 - 5
shell/browser/native_window_views.cc

@@ -1072,9 +1072,9 @@ void NativeWindowViews::AddBrowserView(NativeBrowserView* view) {
   }
 
   add_browser_view(view);
-
-  content_view()->AddChildView(
-      view->GetInspectableWebContentsView()->GetView());
+  if (view->GetInspectableWebContentsView())
+    content_view()->AddChildView(
+        view->GetInspectableWebContentsView()->GetView());
 }
 
 void NativeWindowViews::RemoveBrowserView(NativeBrowserView* view) {
@@ -1085,8 +1085,9 @@ void NativeWindowViews::RemoveBrowserView(NativeBrowserView* view) {
     return;
   }
 
-  content_view()->RemoveChildView(
-      view->GetInspectableWebContentsView()->GetView());
+  if (view->GetInspectableWebContentsView())
+    content_view()->RemoveChildView(
+        view->GetInspectableWebContentsView()->GetView());
   remove_browser_view(view);
 }
 

+ 15 - 4
spec-main/api-browser-view-spec.ts

@@ -134,20 +134,31 @@ describe('BrowserView module', () => {
       w.addBrowserView(view2);
       defer(() => w.removeBrowserView(view2));
     });
+
     it('does not throw if called multiple times with same view', () => {
       view = new BrowserView();
       w.addBrowserView(view);
       w.addBrowserView(view);
       w.addBrowserView(view);
     });
+
+    it('does not crash if the BrowserView webContents are destroyed prior to window removal', () => {
+      expect(() => {
+        const view1 = new BrowserView();
+        (view1.webContents as any).destroy();
+        w.addBrowserView(view1);
+      }).to.not.throw();
+    });
   });
 
   describe('BrowserWindow.removeBrowserView()', () => {
     it('does not throw if called multiple times with same view', () => {
-      view = new BrowserView();
-      w.addBrowserView(view);
-      w.removeBrowserView(view);
-      w.removeBrowserView(view);
+      expect(() => {
+        view = new BrowserView();
+        w.addBrowserView(view);
+        w.removeBrowserView(view);
+        w.removeBrowserView(view);
+      }).to.not.throw();
     });
   });