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

Closes #21666. This PR is fixing crashes caused by referencing and attempting to modify previously destroyed views. Before, when a view was destroyed and then the contents were referenced for modification, the system would crash as undefined memory was accessed. This fix explicitly makes the pointer to the destroyed view's contents null, so that this will not happen.
electron · Sep 23, 2020 · 1d5eb77 · 1d5eb77
1 parent 6c3c9b5
commit 1d5eb77
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());
+});