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: capture the promise global to avoid userland mutation #20925

Merged
merged 1 commit into from Nov 4, 2019

Conversation

@MarshallOfSound
Copy link
Member

MarshallOfSound commented Nov 2, 2019

A common module Bluebird overrides the Promise global with a non A+ implementation that doesn't support thennables (Promise.resolve({ then: () => Promise.resolve(123) })).

This modifies Electrons code to make it impossible for us to rely on the global, instead we cache the global at runtime in the capturedGlobals object that is auto-injected by webpack where it is used.

There is also an eslint rule to prevent accidental re-introduction of this issue.

This is kinda similar to node's primordials but doesn't strip prototypes and deep clone, it just retains a reference to the original constructor.

Notes: Fixed issue where proxied remote promises might not resolve if Bluebird was installed in the renderer

@MarshallOfSound

This comment has been minimized.

Copy link
Member Author

MarshallOfSound commented Nov 2, 2019

This was caught as we adopted the promisified methods and noticed them not resolving in the renderer

@electron-cation electron-cation bot removed the new-pr 🌱 label Nov 3, 2019
Copy link
Member

felixrieseberg left a comment

Looks clean enough!

lib/renderer/api/remote.js Outdated Show resolved Hide resolved
@MarshallOfSound MarshallOfSound force-pushed the capture-the-promise-global branch from ae1cef9 to 1d08b8a Nov 4, 2019
@MarshallOfSound MarshallOfSound merged commit 2678218 into master Nov 4, 2019
11 of 16 checks passed
11 of 16 checks passed
appveyor: win-ia32-testing Waiting for AppVeyor build to complete
Details
appveyor: win-woa-testing Waiting for AppVeyor build to complete
Details
appveyor: win-x64-testing Waiting for AppVeyor build to complete
Details
Backportable? - 7-0-x Backport Failed
Details
Backportable? - 8-x-y Backport Failed
Details
Artifact Comparison No Changes
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing-pr 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 #20191104.19 succeeded
Details
electron-arm64-testing Build #20191104.19 succeeded
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

release-clerk bot commented Nov 4, 2019

Release Notes Persisted

Fixed issue where proxied remote promises might not resolve if Bluebird was installed in the renderer

@MarshallOfSound MarshallOfSound deleted the capture-the-promise-global branch Nov 4, 2019
@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Nov 4, 2019

I was unable to backport this PR to "7-0-x" cleanly;
you will need to perform this backport manually.

@trop trop bot added needs-manual-bp/7-0-x and removed target/7-1-x labels Nov 4, 2019
@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Nov 4, 2019

I was unable to backport this PR to "8-x-y" cleanly;
you will need to perform this backport manually.

@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Nov 4, 2019

@MarshallOfSound has manually backported this PR to "8-x-y", please check out #20946

@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Nov 4, 2019

@MarshallOfSound has manually backported this PR to "7-0-x", please check out #20947

@trop trop bot removed the needs-manual-bp/7-0-x label Nov 4, 2019
@trop trop bot added the in-flight/7-0-x label Nov 4, 2019
@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Nov 4, 2019

@MarshallOfSound has manually backported this PR to "7-1-x", please check out #20947

@trop trop bot added the in-flight/7-1-x label Nov 4, 2019
MarshallOfSound added a commit that referenced this pull request Nov 4, 2019
@trop trop bot added merged/8-x-y and removed in-flight/8-x-y labels Nov 4, 2019
MarshallOfSound added a commit that referenced this pull request Nov 4, 2019
@trop trop bot added merged/7-1-x and removed in-flight/7-1-x labels Nov 4, 2019
@sofianguy sofianguy added this to Fixed in 7.1.0 in 7.1.x Nov 6, 2019
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.0
4 participants
You can’t perform that action at this time.