From 53aaeb7a16af591f2591a3c72242f9470e83006d Mon Sep 17 00:00:00 2001 From: mlaurencin <35157522+mlaurencin@users.noreply.github.com> Date: Wed, 16 Sep 2020 17:10:49 -0700 Subject: [PATCH] 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. --- shell/browser/native_browser_view.cc | 12 +++++- shell/browser/native_browser_view.h | 7 +++- shell/browser/native_browser_view_mac.mm | 37 +++++++++++++------ shell/browser/native_browser_view_views.cc | 30 ++++++++++++--- .../crash-cases/api-browser-destroy/index.js | 25 +++++++++++++ 5 files changed, 90 insertions(+), 21 deletions(-) create mode 100644 spec-main/fixtures/crash-cases/api-browser-destroy/index.js diff --git a/shell/browser/native_browser_view.cc b/shell/browser/native_browser_view.cc index ff948e4839da..b906308bb408 100644 --- 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 index ac2dff293bbf..8d5623a53e1c 100644 --- 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 index 4caca846213d..b844281f059f 100644 --- a/shell/browser/native_browser_view_mac.mm +++ b/shell/browser/native_browser_view_mac.mm @@ -162,8 +162,10 @@ namespace electron { 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 @@ void NativeBrowserViewMac::SetAutoResizeFlags(uint8_t flags) { 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 @@ void NativeBrowserViewMac::SetBounds(const gfx::Rect& bounds) { } 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 @@ gfx::Rect NativeBrowserViewMac::GetBounds() { } 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& 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 index 8fead14c2305..cd1a2382f79c 100644 --- 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(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(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 new file mode 100644 index 000000000000..1e87b8d177a4 --- /dev/null +++ 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()); +});