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

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

Merged
merged 3 commits into from Dec 13, 2019

Conversation

@loc
Copy link
Member

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.

@loc loc requested review from nornagon and MarshallOfSound Dec 12, 2019
Copy link
Member

nornagon left a comment

seems legit

lib/browser/api/web-contents.js Outdated Show resolved Hide resolved
lib/browser/api/web-contents.js Outdated Show resolved Hide resolved
Copy link
Member

nornagon left a comment

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 Dec 12, 2019
Copy link
Member

MarshallOfSound left a comment

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 dismissed their stale review Dec 13, 2019

sorry meant just to comment

@codebytere

This comment has been minimized.

Copy link
Member

codebytere commented Dec 13, 2019

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

has died, but will when green:)

@MarshallOfSound MarshallOfSound merged commit 1edfffa into 7-1-x Dec 13, 2019
17 of 19 checks passed
17 of 19 checks passed
electron-woa-testing Build #20191213.13 had test failures
Details
electron-woa-testing #20191213.13 failed
Details
Artifact Comparison No Changes
Details
Semantic Pull Request ready to be squashed
Details
Valid Backport Valid Backport
Details
WIP Ready for review
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-woa-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
electron-arm-testing Build #20191213.12 succeeded
Details
electron-arm-testing #20191213.12 succeeded
Details
electron-arm64-testing Build #20191213.12 succeeded
Details
electron-arm64-testing #20191213.12 succeeded
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

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 Dec 13, 2019
@sofianguy sofianguy added this to Fixed in 7.1.5 in 7.1.x Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
7.1.x
Fixed in 7.1.5
4 participants
You can’t perform that action at this time.