fix: prevent destroyed view references from causing crashes (#25509)

* fix: prevent destroyed view references from causing crashes * Remove extraneous comments * Relocate crash test to proper location * Add WebContentsObserver * Add nullptr check and inspectable observer * Remote initial test file * Add test cases to test file * Rename and move testing file * Fix linting errors * Make functions exit early on check * Fix setBackgroundColor function call in test file * Fix styling of test file * Fix mac name mismatch and make variable name more clear Co-authored-by: Michaela Laurencin <mlaurencin@microsoft.com>
electron · Sep 21, 2020 · 0f4f5ac · 0f4f5ac
1 parent 4e080c4
commit 0f4f5ac
Show file tree

Hide file tree

Showing 5 changed files with 90 additions and 21 deletions.
diff --git a/shell/browser/native_browser_view.cc b/shell/browser/native_browser_view.cc
@@ -13,16 +13,26 @@ namespace electron {
 
 NativeBrowserView::NativeBrowserView(
     InspectableWebContents* inspectable_web_contents)
-    : inspectable_web_contents_(inspectable_web_contents) {}
+    : inspectable_web_contents_(inspectable_web_contents) {
+  Observe(inspectable_web_contents_->GetWebContents());
+}
 
 NativeBrowserView::~NativeBrowserView() = default;
 
 InspectableWebContentsView* NativeBrowserView::GetInspectableWebContentsView() {
+  if (!inspectable_web_contents_)
+    return nullptr;
   return inspectable_web_contents_->GetView();
 }
 
 content::WebContents* NativeBrowserView::GetWebContents() {
+  if (!inspectable_web_contents_)
+    return nullptr;
   return inspectable_web_contents_->GetWebContents();
 }
 
+void NativeBrowserView::WebContentsDestroyed() {
+  inspectable_web_contents_ = nullptr;
+}
+
 }  // namespace electron
diff --git a/shell/browser/native_browser_view.h b/shell/browser/native_browser_view.h
@@ -9,6 +9,7 @@
 
 #include "base/macros.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_observer.h"
 #include "third_party/skia/include/core/SkColor.h"
 
 namespace gfx {
@@ -27,9 +28,9 @@ enum AutoResizeFlags {
 class InspectableWebContents;
 class InspectableWebContentsView;
 
-class NativeBrowserView {
+class NativeBrowserView : public content::WebContentsObserver {
  public:
-  virtual ~NativeBrowserView();
+  ~NativeBrowserView() override;
 
   static NativeBrowserView* Create(
       InspectableWebContents* inspectable_web_contents);
@@ -52,6 +53,8 @@ class NativeBrowserView {
 
  protected:
   explicit NativeBrowserView(InspectableWebContents* inspectable_web_contents);
+  // content::WebContentsObserver:
+  void WebContentsDestroyed() override;
 
   InspectableWebContents* inspectable_web_contents_;
 

diff --git a/shell/browser/native_browser_view_mac.mm b/shell/browser/native_browser_view_mac.mm
@@ -162,8 +162,10 @@ - (BOOL)mouseDownCanMoveWindow {
 NativeBrowserViewMac::NativeBrowserViewMac(
     InspectableWebContents* inspectable_web_contents)
     : NativeBrowserView(inspectable_web_contents) {
-  auto* view =
-      GetInspectableWebContentsView()->GetNativeView().GetNativeNSView();
+  auto* iwc_view = GetInspectableWebContentsView();
+  if (!iwc_view)
+    return;
+  auto* view = iwc_view->GetNativeView().GetNativeNSView();
   view.autoresizingMask = kDefaultAutoResizingMask;
 }
 
@@ -186,14 +188,18 @@ - (BOOL)mouseDownCanMoveWindow {
         NSViewMaxYMargin | NSViewMinYMargin | NSViewHeightSizable;
   }
 
-  auto* view =
-      GetInspectableWebContentsView()->GetNativeView().GetNativeNSView();
+  auto* iwc_view = GetInspectableWebContentsView();
+  if (!iwc_view)
+    return;
+  auto* view = iwc_view->GetNativeView().GetNativeNSView();
   view.autoresizingMask = autoresizing_mask;
 }
 
 void NativeBrowserViewMac::SetBounds(const gfx::Rect& bounds) {
-  auto* view =
-      GetInspectableWebContentsView()->GetNativeView().GetNativeNSView();
+  auto* iwc_view = GetInspectableWebContentsView();
+  if (!iwc_view)
+    return;
+  auto* view = iwc_view->GetNativeView().GetNativeNSView();
   auto* superview = view.superview;
   const auto superview_height = superview ? superview.frame.size.height : 0;
   view.frame =
@@ -202,8 +208,10 @@ - (BOOL)mouseDownCanMoveWindow {
 }
 
 gfx::Rect NativeBrowserViewMac::GetBounds() {
-  NSView* view =
-      GetInspectableWebContentsView()->GetNativeView().GetNativeNSView();
+  auto* iwc_view = GetInspectableWebContentsView();
+  if (!iwc_view)
+    return gfx::Rect();
+  NSView* view = iwc_view->GetNativeView().GetNativeNSView();
   const int superview_height =
       (view.superview) ? view.superview.frame.size.height : 0;
   return gfx::Rect(
@@ -213,17 +221,22 @@ - (BOOL)mouseDownCanMoveWindow {
 }
 
 void NativeBrowserViewMac::SetBackgroundColor(SkColor color) {
-  auto* view =
-      GetInspectableWebContentsView()->GetNativeView().GetNativeNSView();
+  auto* iwc_view = GetInspectableWebContentsView();
+  if (!iwc_view)
+    return;
+  auto* view = iwc_view->GetNativeView().GetNativeNSView();
   view.wantsLayer = YES;
   view.layer.backgroundColor = skia::CGColorCreateFromSkColor(color);
 }
 
 void NativeBrowserViewMac::UpdateDraggableRegions(
     const std::vector<gfx::Rect>& drag_exclude_rects) {
+  auto* iwc_view = GetInspectableWebContentsView();
+  auto* web_contents = GetWebContents();
+  if (!(iwc_view && web_contents))
+    return;
   NSView* web_view = GetWebContents()->GetNativeView().GetNativeNSView();
-  NSView* inspectable_view =
-      GetInspectableWebContentsView()->GetNativeView().GetNativeNSView();
+  NSView* inspectable_view = iwc_view->GetNativeView().GetNativeNSView();
   NSView* window_content_view = inspectable_view.superview;
   const auto window_content_view_height = NSHeight(window_content_view.bounds);
 

diff --git a/shell/browser/native_browser_view_views.cc b/shell/browser/native_browser_view_views.cc
@@ -26,7 +26,10 @@ void NativeBrowserViewViews::SetAutoResizeProportions(
     const gfx::Size& window_size) {
   if ((auto_resize_flags_ & AutoResizeFlags::kAutoResizeHorizontal) &&
       !auto_horizontal_proportion_set_) {
-    auto* view = GetInspectableWebContentsView()->GetView();
+    auto* iwc_view = GetInspectableWebContentsView();
+    if (iwc_view)
+      return;
+    auto* view = iwc_view->GetView();
     auto view_bounds = view->bounds();
     auto_horizontal_proportion_width_ =
         static_cast<float>(window_size.width()) /
@@ -37,7 +40,10 @@ void NativeBrowserViewViews::SetAutoResizeProportions(
   }
   if ((auto_resize_flags_ & AutoResizeFlags::kAutoResizeVertical) &&
       !auto_vertical_proportion_set_) {
-    auto* view = GetInspectableWebContentsView()->GetView();
+    auto* iwc_view = GetInspectableWebContentsView();
+    if (iwc_view)
+      return;
+    auto* view = iwc_view->GetView();
     auto view_bounds = view->bounds();
     auto_vertical_proportion_height_ =
         static_cast<float>(window_size.height()) /
@@ -51,7 +57,10 @@ void NativeBrowserViewViews::SetAutoResizeProportions(
 void NativeBrowserViewViews::AutoResize(const gfx::Rect& new_window,
                                         int width_delta,
                                         int height_delta) {
-  auto* view = GetInspectableWebContentsView()->GetView();
+  auto* iwc_view = GetInspectableWebContentsView();
+  if (iwc_view)
+    return;
+  auto* view = iwc_view->GetView();
   const auto flags = GetAutoResizeFlags();
   if (!(flags & kAutoResizeWidth)) {
     width_delta = 0;
@@ -92,17 +101,26 @@ void NativeBrowserViewViews::ResetAutoResizeProportions() {
 }
 
 void NativeBrowserViewViews::SetBounds(const gfx::Rect& bounds) {
-  auto* view = GetInspectableWebContentsView()->GetView();
+  auto* iwc_view = GetInspectableWebContentsView();
+  if (!iwc_view)
+    return;
+  auto* view = iwc_view->GetView();
   view->SetBoundsRect(bounds);
   ResetAutoResizeProportions();
 }
 
 gfx::Rect NativeBrowserViewViews::GetBounds() {
-  return GetInspectableWebContentsView()->GetView()->bounds();
+  auto* iwc_view = GetInspectableWebContentsView();
+  if (!iwc_view)
+    return gfx::Rect();
+  return iwc_view->GetView()->bounds();
 }
 
 void NativeBrowserViewViews::SetBackgroundColor(SkColor color) {
-  auto* view = GetInspectableWebContentsView()->GetView();
+  auto* iwc_view = GetInspectableWebContentsView();
+  if (!iwc_view)
+    return;
+  auto* view = iwc_view->GetView();
   view->SetBackground(views::CreateSolidBackground(color));
   view->SchedulePaint();
 }

diff --git a/spec-main/fixtures/crash-cases/api-browser-destroy/index.js b/spec-main/fixtures/crash-cases/api-browser-destroy/index.js
@@ -0,0 +1,25 @@
+const { app, BrowserWindow, BrowserView } = require('electron');
+const { expect } = require('chai');
+
+function createWindow () {
+  // Create the browser window.
+  const mainWindow = new BrowserWindow({
+    width: 800,
+    height: 600,
+    webPreferences: {
+      nodeIntegration: true
+    }
+  });
+  const view = new BrowserView();
+  mainWindow.addBrowserView(view);
+  view.webContents.destroy();
+  view.setBounds({ x: 0, y: 0, width: 0, height: 0 });
+  const bounds = view.getBounds();
+  expect(bounds).to.deep.equal({ x: 0, y: 0, width: 0, height: 0 });
+  view.setBackgroundColor('#56cc5b10');
+}
+
+app.on('ready', () => {
+  createWindow();
+  setTimeout(() => app.quit());
+});