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: add patch to prevent crash during frame swap with ctx isolation enabled #23684

Merged
merged 1 commit into from
Jun 2, 2020

Conversation

MarshallOfSound
Copy link
Member

Electron's Context Isolation implementation has a side-effect of initializing the isolated worlds WindowProxy during the initialization of the main world WindowProxy as a result of creating the isolated world inside the DidCreateScriptContext hook. This results in an assertion failing in Chromium during a frame swap where it expects to be able to set a new global_proxy in the WindowProxy of the isolated world BEFORE it is initialized.

To meet this assumption this patch splits SetGlobalProxy into two calls, SetGlobalProxyWithoutInitializing and InitializeIfNeeded which has the same resultant effect but means that all of the global_proxy objects are set BEFORE any WindowProxy's are initialized.

This could probably be upstreamed as it doesn't affect the way Chromium works but also it has no benefit for them at this time.

Notes: Fixed crash when navigating between origins in a child window with nativeWindowOpen and contextIsolation enabled

@MarshallOfSound MarshallOfSound requested a review from loc May 20, 2020 20:57
@MarshallOfSound MarshallOfSound requested a review from a team as a code owner May 20, 2020 20:57
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label May 20, 2020
@MarshallOfSound MarshallOfSound force-pushed the fix-swap-ctx-isolation branch 2 times, most recently from b99fd54 to 55c8f48 Compare May 20, 2020 21:03
@deepak1556
Copy link
Member

Is there a minimal repro for this, its hard to understand from the patch what kind of assertion failure is hit and which type of frame swap triggers this.

Curious, mainly because this implementation hasn't changed much in years, so why hasn't ci caught this ? Can we improve the test if so.

@MarshallOfSound
Copy link
Member Author

@deepak1556 I'm working on a test for this edge case, automating it out is quite complex but it should be possible. I've got a fiddle I'll DM you

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label May 21, 2020
@MarshallOfSound MarshallOfSound force-pushed the fix-swap-ctx-isolation branch 2 times, most recently from f45f1d7 to 70b8cd8 Compare June 1, 2020 23:36
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.

Oops this PR slipped through the cracks, verified the fix. @MarshallOfSound can you link the POC here so that other maintainers can look at it too. Thanks!

LGTM

@MarshallOfSound MarshallOfSound merged commit fbf397e into master Jun 2, 2020
@release-clerk
Copy link

release-clerk bot commented Jun 2, 2020

Release Notes Persisted

Fixed crash when navigating between origins in a child window with nativeWindowOpen and contextIsolation enabled

@trop
Copy link
Contributor

trop bot commented Jun 2, 2020

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

@MarshallOfSound
Copy link
Member Author

/trop run backport-to 10-x-y

@trop
Copy link
Contributor

trop bot commented Jun 2, 2020

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

@trop trop bot removed the target/9-x-y label Jun 2, 2020
@trop
Copy link
Contributor

trop bot commented Jun 2, 2020

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

@trop
Copy link
Contributor

trop bot commented Jun 2, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants