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.

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: workaround for hang when preventDefault-ing nativeWindowOpen (7-1-x) #21497

Merged
merged 3 commits into from Dec 13, 2019
Merged

fix: workaround for hang when preventDefault-ing nativeWindowOpen (7-1-x) #21497

merged 3 commits into from Dec 13, 2019

Conversation

loc
Copy link
Contributor

@loc loc commented Dec 12, 2019

Description of Change

Addresses the same issues as #21236, except without replacing the current API and cleaning up the involved code paths. Briefly, the issue is that canceling a nativeWindowOpen-ed window in the new-window event causes a hang because the WebContents is already initializing. This change adds a hook earlier, so we can cancel before it starts loading.

#21236 is the "right way", this change is because we need the workaround sooner than time allows. We'd prefer not to rush the addition of a new API!

Checklist

Release Notes

Notes: Added workaround for nativeWindowOpen hang.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Dec 12, 2019
nornagon
nornagon approved these changes Dec 12, 2019
View reviewed changes
Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

seems legit

lib/browser/api/web-contents.js Outdated Show resolved Hide resolved
lib/browser/api/web-contents.js Outdated Show resolved Hide resolved
nornagon
nornagon reviewed Dec 12, 2019
View reviewed changes
Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

also, could we get a test for this?

@loc loc force-pushed the loc/native-window-open-hang-7-1-x branch from 52b18a3 to 09b17c8 Compare December 12, 2019 23:01
MarshallOfSound
MarshallOfSound approved these changes Dec 12, 2019
View reviewed changes
Copy link
Member

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

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

Point of note, this is a deliberately undocumented temporary event that will be removed as soon as the real fix has landed. This is just a quick workaround to allow folks to adopt 7.1 and nativeWindowOpen. We intend to ship a real and documented fix as 7.2 (it will be semver/minor).

codebytere
codebytere previously approved these changes Dec 13, 2019
View reviewed changes
@codebytere codebytere dismissed their stale review December 13, 2019 00:40

sorry meant just to comment

@codebytere
Copy link
Member 

webContents module getAllWebContents() API returns an array of web contents - returns an array of web contents

has died, but will ✅ when green:)

@MarshallOfSound MarshallOfSound added the fast-track 🚅 Indicates that this PR is intended to bypass the 24 hour rule. Needs approval from Releases label Dec 13, 2019
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Dec 13, 2019
@MarshallOfSound MarshallOfSound merged commit 1edfffa into 7-1-x Dec 13, 2019
@release-clerk
Copy link

release-clerk bot commented Dec 13, 2019

Release Notes Persisted

Added workaround for nativeWindowOpen hang.

@MarshallOfSound MarshallOfSound deleted the loc/native-window-open-hang-7-1-x branch December 13, 2019 16:06
@sofianguy sofianguy added this to Fixed in 7.1.5 in 7.2.x Jan 14, 2020
@loc
Copy link
Contributor Author

loc commented Mar 18, 2020

/trop run backport-to 8-x-y

@trop
Copy link
Contributor

trop bot commented Mar 18, 2020

@loc is not authorized to run PR backports.

@MarshallOfSound
Copy link
Member

/trop run backport-to 8-x-y

@trop
Copy link
Contributor

trop bot commented Mar 18, 2020

The backport process for this PR has been manually initiated -
sending your commits to "8-x-y"!

@MarshallOfSound
Copy link
Member

/trop run backport-to 9-x-y

@trop
Copy link
Contributor

trop bot commented Mar 18, 2020

The backport process for this PR has been manually initiated -
sending your commits to "9-x-y"!

@trop trop bot mentioned this pull request Mar 18, 2020
Merged
@trop
Copy link
Contributor

trop bot commented Mar 18, 2020

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

@trop trop bot added the in-flight/8-x-y label Mar 18, 2020
@trop trop bot mentioned this pull request Mar 18, 2020
Merged
@trop
Copy link
Contributor

trop bot commented Mar 18, 2020

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

@trop trop bot added the in-flight/9-x-y label Mar 18, 2020
@loc loc mentioned this pull request Mar 24, 2020
Merged
@trop
Copy link
Contributor

trop bot commented Mar 24, 2020

@loc has manually backported this PR to "master", please check out #22825

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nornagon nornagon nornagon approved these changes

@codebytere codebytere codebytere approved these changes

@MarshallOfSound MarshallOfSound MarshallOfSound approved these changes

Assignees
No one assigned
Labels
fast-track 🚅 Indicates that this PR is intended to bypass the 24 hour rule. Needs approval from Releases
Projects
No open projects
7.2.x
Fixed in 7.1.5
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@loc @codebytere @MarshallOfSound @nornagon