Skip to content

fix: don't re-parse URL unnecessarily when handling dialogs#50062

Merged
jkleinsc merged 7 commits intoelectron:mainfrom
nmggithub:fix-dialogs-in-urlless-frames
Mar 20, 2026
Merged

fix: don't re-parse URL unnecessarily when handling dialogs#50062
jkleinsc merged 7 commits intoelectron:mainfrom
nmggithub:fix-dialogs-in-urlless-frames

Conversation

@nmggithub
Copy link
Contributor

@nmggithub nmggithub commented Mar 4, 2026 •
edited
Loading

Description of Change

Should resolve #50059. The crash was due to our code assuming that the frame where a dialog is being shown will always have a parseable URL.

This PR makes two changes:

1. it falls back to a generated URL (based on frame identity) when the frame URL is an empty string, and
2. it tightens up the origin calculation, falling back to the entire href when, by spec, the origin is a non-unique opaque origin (the string "null").

The only edge case I see here is if the frame URL is a non-empty string and is un-parsable, but that should be rare. I can add a try-catch if we decide it's worth it.

This PR removes an unnecessary new URL() call that could lead to crashing.

Checklist

Release Notes

Notes: Fixed crash when handling JavaScript dialogs from windows opened with invalid or empty URLs.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Mar 4, 2026
@ckerr
Copy link
Member

ckerr commented Mar 5, 2026

@nmggithub the bots want target/* labels. Which branches should this fix be applied to?

@nmggithub
Copy link
Contributor Author

nmggithub commented Mar 5, 2026 •
edited
Loading

@ckerr Apologies for the delay in getting back to you. Honestly, this should apply to any and all branches / versions. It appears to be a long-standing issue. Any cases where its not an issue would likely be different enough to where this diff wouldn't be viable.

deepak1556
deepak1556 requested changes Mar 6, 2026
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.

Minor nit, also can you add a crash test case to https://github.com/electron/electron/tree/main/spec/fixtures/crash-cases based on the gist attached to the issue.

@nmggithub nmggithub requested a review from deepak1556 March 6, 2026 21:39
deepak1556
deepak1556 reviewed Mar 7, 2026
View reviewed changes
@nmggithub nmggithub requested a review from deepak1556 March 7, 2026 01:08
deepak1556
deepak1556 approved these changes Mar 7, 2026
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.

Thanks!

@nmggithub nmggithub changed the title fix: fallback to opaque URL when needed inside dialog callback fix: don't re-parse URL unnecessarily when handling dialogs Mar 7, 2026
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label Mar 11, 2026
@jkleinsc jkleinsc added semver/patch backwards-compatible bug fixes target/39-x-y PR should also be added to the "39-x-y" branch. target/40-x-y PR should also be added to the "40-x-y" branch. target/41-x-y PR should also be added to the "41-x-y" branch. labels Mar 11, 2026
jkleinsc
jkleinsc requested changes Mar 11, 2026
View reviewed changes
Copy link
Member

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

Failing tests need to be addressed.

@nmggithub
Copy link
Contributor Author

nmggithub commented Mar 11, 2026

Failing tests need to be addressed.

Just now noticed this. I'll see what I can do, but I think this might also be some of the flakiness of safeDialogs that I've been sensing while working on this. This might require some more substantial work, but I could potentially work around it for now.

@nmggithub nmggithub force-pushed the fix-dialogs-in-urlless-frames branch from 8acc63b to 10b2ab5 Compare March 11, 2026 21:03
@nmggithub nmggithub requested a review from jkleinsc March 11, 2026 21:24
@github-actions github-actions bot added the target/42-x-y PR should also be added to the "42-x-y" branch. label Mar 13, 2026
@devinbinnie devinbinnie mentioned this pull request Mar 16, 2026
Merged
@jkleinsc jkleinsc mentioned this pull request Mar 18, 2026
Closed
3 tasks
@jkleinsc jkleinsc merged commit 8a0c204 into electron:main Mar 20, 2026
331 of 340 checks passed
@release-clerk
Copy link

release-clerk bot commented Mar 20, 2026

Release Notes Persisted

Fixed crash when handling JavaScript dialogs from windows opened with invalid or empty URLs.

This was referenced Mar 20, 2026
Merged
Merged
Merged
Merged
@trop
Copy link
Contributor

trop bot commented Mar 20, 2026

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

@trop
Copy link
Contributor

trop bot commented Mar 20, 2026

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

@trop trop bot removed the target/42-x-y PR should also be added to the "42-x-y" branch. label Mar 20, 2026
@trop
Copy link
Contributor

trop bot commented Mar 20, 2026

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

@trop
Copy link
Contributor

trop bot commented Mar 20, 2026

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

@trop trop bot added in-flight/41-x-y in-flight/39-x-y in-flight/40-x-y merged/40-x-y PR was merged to the "40-x-y" branch. merged/42-x-y PR was merged to the "42-x-y" branch. merged/41-x-y PR was merged to the "41-x-y" branch. merged/39-x-y PR was merged to the "39-x-y" branch. and removed target/41-x-y PR should also be added to the "41-x-y" branch. target/39-x-y PR should also be added to the "39-x-y" branch. target/40-x-y PR should also be added to the "40-x-y" branch. in-flight/40-x-y in-flight/42-x-y in-flight/41-x-y in-flight/39-x-y labels Mar 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jkleinsc jkleinsc jkleinsc approved these changes

@deepak1556 deepak1556 deepak1556 approved these changes

@codebytere codebytere codebytere approved these changes

Assignees

No one assigned

Labels

merged/39-x-y PR was merged to the "39-x-y" branch. merged/40-x-y PR was merged to the "40-x-y" branch. merged/41-x-y PR was merged to the "41-x-y" branch. merged/42-x-y PR was merged to the "42-x-y" branch. semver/patch backwards-compatible bug fixes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Calling `window.open('javascript:alert()') from the renderer causes unhandled exception

6 participants

@nmggithub @ckerr @jkleinsc @deepak1556 @codebytere @nikwen