fix: prevent destroyed view references from causing crashes #25411

mlaurencin · 2020-09-10T19:41:00Z

Description of Change

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.

Checklist

PR description included and stakeholders cc'd
npm test passes
tests are changed or added
relevant documentation is changed or added
PR title follows semantic commit guidelines
PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Fixed crashes caused by attempting to modify destroyed views

spec/api-browser-view-destroy.js

shell/browser/native_browser_view_views.cc

codebytere

This is a great start! as the crash in setBounds is but a symptom of a larger set of potential crashes, though, we'll want to make sure we address all similar potential crashes as well as try to add regression tests for all related crashes to really close up this bug :)

shell/browser/native_browser_view_views.cc

codebytere · 2020-09-16T01:14:51Z

Looks like lint is still failing: see https://app.circleci.com/pipelines/github/electron/electron/29870/workflows/28683667-5c33-4ba1-a255-70e8d981c345/jobs/658611 for details :) You can check these locally with npm run lint

mlaurencin · 2020-09-16T01:18:02Z

Looks like lint is still failing: see https://app.circleci.com/pipelines/github/electron/electron/29870/workflows/28683667-5c33-4ba1-a255-70e8d981c345/jobs/658611 for details :) You can check these locally with npm run lint

Great! Pushing the fix now. I was actually using the notes from the errors in CI to make the fixes. I am getting local errors when running npm run lint saying "'gn' is not recognized as an internal or external command"

MarshallOfSound · 2020-09-16T04:14:01Z

I am getting local errors when running npm run lint saying "'gn' is not recognized as an internal or external command"

Ah that's because you're exclusively on build tools I think. @codebytere we should either make e lint or come up with a better way of making those commands available.

@mlaurencin for now e d npm run lint would probably work (I know it looks funky but it works) 😆

shell/browser/native_browser_view_mac.mm

mlaurencin · 2020-09-16T18:11:02Z

e d npm run lint

I am getting local errors when running npm run lint saying "'gn' is not recognized as an internal or external command"

Ah that's because you're exclusively on build tools I think. @codebytere we should either make e lint or come up with a better way of making those commands available.

@mlaurencin for now e d npm run lint would probably work (I know it looks funky but it works) 😆

I tried running that and ended up with this new error ERROR Failed to run command, exit code was "null", error was 'Error: spawnSync npm ENOENT' 😅 @codebytere

trop · 2020-09-17T00:11:31Z

I have automatically backported this PR to "9-x-y", please check out #25510

trop · 2020-09-17T00:11:34Z

I have automatically backported this PR to "10-x-y", please check out #25511

…#25411) Closes electron#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.

(cont.) references from causing crashes Referenced PR: electron#25411

trop · 2020-09-23T20:34:33Z

@mlaurencin has manually backported this PR to "9-x-y", please check out #25609

Fixes: electron#40567 `null` was introduced as a possibility for destroyed webContents in this PR: electron#25411

electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Sep 10, 2020

groundwater reviewed Sep 10, 2020

View reviewed changes

spec/api-browser-view-destroy.js Outdated Show resolved Hide resolved

MarshallOfSound reviewed Sep 11, 2020

View reviewed changes

shell/browser/native_browser_view_views.cc Outdated Show resolved Hide resolved

electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Sep 11, 2020

codebytere reviewed Sep 14, 2020

View reviewed changes

shell/browser/native_browser_view_views.cc Outdated Show resolved Hide resolved

codebytere reviewed Sep 14, 2020

View reviewed changes

shell/browser/native_browser_view_views.cc Outdated Show resolved Hide resolved

codebytere reviewed Sep 16, 2020

View reviewed changes

shell/browser/native_browser_view_mac.mm Outdated Show resolved Hide resolved

codebytere mentioned this pull request Sep 16, 2020

Create e lint to make linting available to users using exclusively build-tools electron/build-tools#179

Open

mlaurencin changed the title ~~[WIP] fix: prevent destroyed view references from causing crashes~~ fix: prevent destroyed view references from causing crashes Sep 16, 2020

mlaurencin added 13 commits September 16, 2020 11:59

[WIP] fix: prevent destroyed view references from causing crashes

4c860f6

Remove extraneous comments

2533551

Relocate crash test to proper location

b77f186

Add WebContentsObserver

39ed4e6

Add nullptr check and inspectable observer

c41606d

Remote initial test file

4801298

Add test cases to test file

5137bb8

Rename and move testing file

29ce39f

Fix linting errors

0e4a1f7

Make functions exit early on check

2b0953f

Fix setBackgroundColor function call in test file

e0da5b2

Fix styling of test file

ae365b3

Fix mac name mismatch and make variable name more clear

5128cb0

mlaurencin force-pushed the view-destruction branch from 4112399 to 5128cb0 Compare September 16, 2020 19:06

codebytere marked this pull request as ready for review September 16, 2020 20:34

codebytere approved these changes Sep 16, 2020

View reviewed changes

MarshallOfSound approved these changes Sep 16, 2020

View reviewed changes

trop bot added in-flight/11-x-y and removed target/11-x-y labels Sep 17, 2020

This was referenced Sep 17, 2020

fix: prevent destroyed view references from causing crashes #25510

Closed

fix: prevent destroyed view references from causing crashes #25511

Merged

trop bot added in-flight/9-x-y and removed target/9-x-y labels Sep 17, 2020

codebytere mentioned this pull request Sep 17, 2020

App crashes when BrowserView tries to both resize and destroy itself (almost) simultaneously #21410

Closed

3 tasks

trop bot added merged/11-x-y and removed in-flight/11-x-y labels Sep 21, 2020

mlaurencin added a commit to mlaurencin/electron that referenced this pull request Sep 23, 2020

fix: handle backporting prevent destroyed view

a21990e

(cont.) references from causing crashes Referenced PR: electron#25411

mlaurencin mentioned this pull request Sep 23, 2020

fix: prevent destroyed view references from causing crashes #25609

Merged

4 tasks

trop bot added merged/10-x-y and removed in-flight/10-x-y labels Sep 24, 2020

codebytere mentioned this pull request Sep 25, 2020

Trop can get confused about in-flight/merged sometimes electron/trop#162

Closed

codebytere removed the in-flight/9-x-y label Sep 25, 2020

codebytere mentioned this pull request Oct 10, 2020

BrowserView's setAutoResize no longer works (since Electron 10.1.3) #25728

Closed

3 tasks

vyunikov mentioned this pull request Oct 26, 2020

Electron main process crashes on exit (version 10.1.3) #26019

Closed

3 tasks

codebytere mentioned this pull request Oct 29, 2020

BrowserView crashes the app after resizing the BrowserWindow #19921

Closed

3 tasks

Juice10 mentioned this pull request Nov 20, 2023

[Bug]: BrowserView.webContents can be null, but typescript types are incorrect #40567

Open

3 tasks

Juice10 added a commit to Juice10/electron that referenced this pull request Nov 20, 2023

Add null as type for browserView.webContents

74ecec1

Fixes: electron#40567 `null` was introduced as a possibility for destroyed webContents in this PR: electron#25411

Juice10 mentioned this pull request Nov 20, 2023

docs: browserView.webContents can be null #40569

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: prevent destroyed view references from causing crashes #25411

fix: prevent destroyed view references from causing crashes #25411

mlaurencin commented Sep 10, 2020 •

edited by codebytere

codebytere left a comment

codebytere commented Sep 16, 2020

mlaurencin commented Sep 16, 2020 •

edited

MarshallOfSound commented Sep 16, 2020

mlaurencin commented Sep 16, 2020

trop bot commented Sep 17, 2020

trop bot commented Sep 17, 2020

trop bot commented Sep 23, 2020

fix: prevent destroyed view references from causing crashes #25411

fix: prevent destroyed view references from causing crashes #25411

Conversation

mlaurencin commented Sep 10, 2020 • edited by codebytere

Description of Change

Checklist

Release Notes

codebytere left a comment

Choose a reason for hiding this comment

codebytere commented Sep 16, 2020

mlaurencin commented Sep 16, 2020 • edited

MarshallOfSound commented Sep 16, 2020

mlaurencin commented Sep 16, 2020

trop bot commented Sep 17, 2020

trop bot commented Sep 17, 2020

trop bot commented Sep 23, 2020

mlaurencin commented Sep 10, 2020 •

edited by codebytere

mlaurencin commented Sep 16, 2020 •

edited