From 77849a17bef1e56f8319b015c4f89c2518e07de3 Mon Sep 17 00:00:00 2001 From: "trop[bot]" <37223003+trop[bot]@users.noreply.github.com> Date: Thu, 24 Sep 2020 11:26:58 -0700 Subject: [PATCH] fix: prevent destroyed view references from causing crashes (#25511) --- .../api/electron_api_top_level_window.cc | 19 ++++++---- .../browser/api/electron_api_web_contents.cc | 3 +- 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 ++++++++++++--- shell/browser/native_window_mac.mm | 14 +++++-- shell/browser/native_window_views.cc | 13 +++++-- .../crash-cases/api-browser-destroy/index.js | 27 ++++++++++++++ 9 files changed, 125 insertions(+), 37 deletions(-) create mode 100644 spec-main/fixtures/crash-cases/api-browser-destroy/index.js diff --git a/shell/browser/api/electron_api_top_level_window.cc b/shell/browser/api/electron_api_top_level_window.cc index 1ec7ea2a8e60d..67999722e93c4 100644 --- a/shell/browser/api/electron_api_top_level_window.cc +++ b/shell/browser/api/electron_api_top_level_window.cc @@ -741,8 +741,10 @@ void TopLevelWindow::AddBrowserView(v8::Local value) { gin::ConvertFromV8(isolate(), value, &browser_view)) { auto get_that_view = browser_views_.find(browser_view->weak_map_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()) { + window_->AddBrowserView(browser_view->view()); + browser_view->web_contents()->SetOwnerWindow(window_.get()); + } browser_views_[browser_view->weak_map_id()].Reset(isolate(), value); } } @@ -754,9 +756,10 @@ void TopLevelWindow::RemoveBrowserView(v8::Local value) { gin::ConvertFromV8(isolate(), value, &browser_view)) { auto get_that_view = browser_views_.find(browser_view->weak_map_id()); if (get_that_view != browser_views_.end()) { - window_->RemoveBrowserView(browser_view->view()); - browser_view->web_contents()->SetOwnerWindow(nullptr); - + if (browser_view->web_contents()) { + window_->RemoveBrowserView(browser_view->view()); + browser_view->web_contents()->SetOwnerWindow(nullptr); + } (*get_that_view).second.Reset(isolate(), value); browser_views_.erase(get_that_view); } @@ -1054,8 +1057,10 @@ void TopLevelWindow::ResetBrowserViews() { v8::Local::New(isolate(), item.second), &browser_view) && !browser_view.IsEmpty()) { - window_->RemoveBrowserView(browser_view->view()); - browser_view->web_contents()->SetOwnerWindow(nullptr); + if (browser_view->web_contents()) { + window_->RemoveBrowserView(browser_view->view()); + browser_view->web_contents()->SetOwnerWindow(nullptr); + } } item.second.Reset(); diff --git a/shell/browser/api/electron_api_web_contents.cc b/shell/browser/api/electron_api_web_contents.cc index 6707ff5bf2940..f35b77c5b9b68 100644 --- a/shell/browser/api/electron_api_web_contents.cc +++ b/shell/browser/api/electron_api_web_contents.cc @@ -698,7 +698,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. diff --git a/shell/browser/native_browser_view.cc b/shell/browser/native_browser_view.cc index ff948e4839da8..b906308bb4088 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 ac2dff293bbff..8d5623a53e1c3 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 4caca846213d2..b844281f059f8 100644 --- 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& 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 8fead14c23054..cd1a2382f79cb 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/shell/browser/native_window_mac.mm b/shell/browser/native_window_mac.mm index 9c01d9b87ea47..f8e9617bb4805 100644 --- a/shell/browser/native_window_mac.mm +++ b/shell/browser/native_window_mac.mm @@ -1247,8 +1247,11 @@ void ViewDidMoveToSuperview(NSView* self, SEL _cmd) { } add_browser_view(view); - auto* native_view = - view->GetInspectableWebContentsView()->GetNativeView().GetNativeNSView(); + auto* iwc_view = view->GetInspectableWebContentsView(); + if (!iwc_view) + return; + + auto* native_view = iwc_view->GetNativeView().GetNativeNSView(); [[window_ contentView] addSubview:native_view positioned:NSWindowAbove relativeTo:nil]; @@ -1266,8 +1269,11 @@ void ViewDidMoveToSuperview(NSView* self, SEL _cmd) { return; } - [view->GetInspectableWebContentsView()->GetNativeView().GetNativeNSView() - removeFromSuperview]; + auto* iwc_view = view->GetInspectableWebContentsView(); + if (!iwc_view) + return; + + [iwc_view->GetNativeView().GetNativeNSView() removeFromSuperview]; remove_browser_view(view); [CATransaction commit]; diff --git a/shell/browser/native_window_views.cc b/shell/browser/native_window_views.cc index 81b09b6b2f3c5..5fa2e30f3e607 100644 --- a/shell/browser/native_window_views.cc +++ b/shell/browser/native_window_views.cc @@ -1065,8 +1065,10 @@ void NativeWindowViews::AddBrowserView(NativeBrowserView* view) { add_browser_view(view); - content_view()->AddChildView( - view->GetInspectableWebContentsView()->GetView()); + auto* iwc_view = view->GetInspectableWebContentsView(); + if (!iwc_view) + return; + content_view()->AddChildView(iwc_view->GetView()); } void NativeWindowViews::RemoveBrowserView(NativeBrowserView* view) { @@ -1077,8 +1079,11 @@ void NativeWindowViews::RemoveBrowserView(NativeBrowserView* view) { return; } - content_view()->RemoveChildView( - view->GetInspectableWebContentsView()->GetView()); + auto* iwc_view = view->GetInspectableWebContentsView(); + if (!iwc_view) + return; + + content_view()->RemoveChildView(iwc_view->GetView()); remove_browser_view(view); } 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 0000000000000..19d92fe84cf45 --- /dev/null +++ b/spec-main/fixtures/crash-cases/api-browser-destroy/index.js @@ -0,0 +1,27 @@ +const { app, BrowserWindow, BrowserView } = require('electron'); +const { expect } = require('chai'); +const assert = require('assert'); + +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(); + app.quit(); +});