Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: crash in BrowserWindow destructor after win.webContents.destroy() #18686

Merged
merged 1 commit into from Jun 14, 2019
Merged

fix: crash in BrowserWindow destructor after win.webContents.destroy() #18686

merged 1 commit into from Jun 14, 2019

Conversation

miniak
Copy link
Contributor

@miniak miniak commented Jun 7, 2019 •
edited by alexeykuzmin

Description of Change

Fixes #18685

Make api_web_contents_ a weak pointer to avoid accessing a destroyed api::WebContents instance.

/cc @zcbenz

Checklist

Release Notes

Notes: Fixed crash in BrowserWindow destructor after win.webContents.destroy().

@miniak miniak requested a review from zcbenz June 7, 2019 14:05
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jun 7, 2019
@miniak miniak self-assigned this Jun 7, 2019
@zcbenz
Copy link
Member

zcbenz commented Jun 7, 2019

I believe this can also fix #16580.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jun 8, 2019
@miniak miniak force-pushed the miniak/fix-webcontents-destroy-crash branch from b1e9fd1 to ef8be62 Compare June 9, 2019 08:37
@miniak miniak requested a review from zcbenz June 9, 2019 08:39
@miniak
Copy link
Contributor Author

miniak commented Jun 9, 2019

@zcbenz can you please review again? I think it's cleaner to make api_web_contents_ a weak pointer.

deepak1556
deepak1556 requested changes Jun 10, 2019
View reviewed changes
Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the relationship of a BrowserWindow and WebContents, the reason for depending on a bare pointer was that a window will always outlive the webContents it embeds and ensure its destruction. The bug this PR fixes explicitly calls destroy which is not the right shutdown sequence for a webContents, its more of a force destroy scenario. The destroy api was never intended for public use because it can mess up destruction sequence. While I am neutral with the changes in this PR, a little concerned about the relationship change.

/cc @zcbenz

atom/browser/api/atom_api_browser_window.cc
@@ -67,7 +67,7 @@ BrowserWindow::BrowserWindow(v8::Isolate* isolate,
}

web_contents_.Reset(isolate, web_contents.ToV8());
api_web_contents_ = web_contents.get();
api_web_contents_ = web_contents->GetWeakPtr();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If its a weak ptr, should null check in other places of access too.

@miniak
Copy link
Contributor Author

miniak commented Jun 10, 2019 •
edited

@deepak1556 Similar issue seems to manifest not only when the webContents is destroyed explicitly. See #16580 mentioned by @zcbenz. Converting a raw ptr to weak ptr should not change the relationship, or I am missing something?

@deepak1556
Copy link
Member

Converting a raw ptr to weak ptr should not change the relationship, or I am missing something?

weakptr has this implicit notion, that objects accessing it doesn't care about the lifetime and can handle them safely. Most useful when posting tasks.

The raw ptr here , was to explicitly state that webContents will outlive the BrowserWindow so its safe to handle it. But thats kinda broken now, while I understand the problem this PR is trying to solve. Just want to make sure the lifetime is handled correctly.

@zcbenz has worked recently on refactoring the destruction sequence, would like his opinion before merging.

@zcbenz
Copy link
Member

zcbenz commented Jun 11, 2019

The lifetime management of WebContents is a mess, even after I tried to clean it up. For #16580, I encountered it occasionally when running small tests, I believe we have code in Electron that incorrectly destroys WebContents before destroying BrowserWindow, but I could not find it.

I would say this PR is a hack rather than a proper fix, but before we find one, I think this can be accepted as a special hack to fix crash when destroying window.

We should probably add a FIXME comment in ~BrowserWindow explaining it is a hack that should be revisited in future.

@deepak1556
Copy link
Member

@miniak can you update following cheng's comment, I can approve it after that. Thanks!

@miniak miniak force-pushed the miniak/fix-webcontents-destroy-crash branch from ef8be62 to 6b5268c Compare June 13, 2019 16:19
@miniak miniak requested a review from deepak1556 June 13, 2019 16:19
@miniak
Copy link
Contributor Author

miniak commented Jun 13, 2019

@deepak1556 FIXME comment added

@miniak miniak mentioned this pull request Jun 14, 2019
Merged
4 tasks
@trop
Copy link
Contributor

trop bot commented Jun 14, 2019

A maintainer has manually backported this PR to "5-0-x", please check out #18794

@miniak miniak mentioned this pull request Jun 14, 2019
Merged
4 tasks
@trop
Copy link
Contributor

trop bot commented Jun 14, 2019

A maintainer has manually backported this PR to "4-2-x", please check out #18795

@miniak miniak mentioned this pull request Jun 14, 2019
Merged
4 tasks
@trop
Copy link
Contributor

trop bot commented Jun 14, 2019

A maintainer has manually backported this PR to "3-1-x", please check out #18796

codebytere pushed a commit that referenced this pull request Jun 14, 2019
@miniak @codebytere
fix: crash in BrowserWindow destructor after win.webContents.destroy() (
47827ed 
#18686) (#18793)
zcbenz pushed a commit that referenced this pull request Jun 15, 2019
@miniak @zcbenz
fix: crash in BrowserWindow destructor after win.webContents.destroy() (
b04eaab 
#18686) (#18794)
zcbenz pushed a commit that referenced this pull request Jun 15, 2019
@miniak @zcbenz
fix: crash in BrowserWindow destructor after win.webContents.destroy() (
2dd2566 
#18686) (#18795)
zcbenz pushed a commit that referenced this pull request Jun 15, 2019
@miniak @zcbenz
fix: crash in BrowserWindow destructor after win.webContents.destroy() (
77e18b5 
#18686) (#18796)
@sofianguy sofianguy added this to Fixed in 6.0.0-beta.8 in 6.1.x Jun 17, 2019
@sofianguy sofianguy added this to Fixed in 5.0.5 in 5.0.x Jun 21, 2019
@aldenquimby aldenquimby mentioned this pull request Jun 27, 2019
Closed
3 tasks
nornagon
nornagon reviewed Jun 28, 2019
View reviewed changes
atom/browser/api/atom_api_browser_window.cc
@@ -93,7 +93,9 @@ BrowserWindow::BrowserWindow(v8::Isolate* isolate,
}

BrowserWindow::~BrowserWindow() {
api_web_contents_->RemoveObserver(this);
// FIXME This is a hack rather than a proper fix preventing shutdown crashes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the long-term plan for fixing this?

@sofianguy sofianguy added this to Fixed (3.1.12) in 3.0.x / 3.1.x Jul 22, 2019
@codebytere codebytere mentioned this pull request Aug 28, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nornagon nornagon nornagon left review comments

@alexeykuzmin alexeykuzmin alexeykuzmin approved these changes

@zcbenz zcbenz zcbenz approved these changes

@deepak1556 deepak1556 deepak1556 approved these changes

@codebytere codebytere codebytere approved these changes

Assignees

@miniak miniak

Labels
None yet
Projects
No open projects
3.0.x / 3.1.x
Fixed (3.1.12)
5.0.x
Fixed in 5.0.5
6.1.x
Fixed in 6.0.0-beta.8
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

win.webContents.destroy() causes a crash
6 participants
@miniak @zcbenz @deepak1556 @nornagon @alexeykuzmin @codebytere