8000 fix: SimpleURLLoaderWrapper redirects by lutzroeder · Pull Request #21630 · electron/electron · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: SimpleURLLoaderWrapper redirects #21630

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.

Sign up for GitHub

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
Merged
merged 1 commit into from
Dec 30, 2019
Merged

fix: SimpleURLLoaderWrapper redirects #21630

merged 1 commit into from
Dec 30, 2019

Conversation

lutzroeder
Copy link
Contributor
@lutzroeder lutzroeder commented Dec 28, 2019 •
edited by nornagon
Loading

Description of Change

Fix #21566.

PR #21304 regressed electron-updater as it raises an error if request.abort() is called in a redirect handler. For all apps that shipped with electron-updater since 7.1.3+ auto-update is not working and users will be stranded with outdated bits. Fix is to not raise an error if the request was aborted within the handler.

@nornagon @MarshallOfSound, I was unable to build and verify this change (mac stuck for many hours in gclient sync). Since you introduced this regression, can you help or commit the correct fix and publish an Electron 7.1.x release including the fix?

Checklist

  • PR description included and stakeholders cc'd
  • npm test passes

@nornagon @MarshallOfSound

Notes: Fixed an issue in the net module where aborting a request during a redirect could cause an error to be thrown.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Dec 28, 2019
nornagon
nornagon approved these changes Dec 28, 2019
View reviewed changes
Copy link
Contributor
@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.

This seems like a reasonable fix, thanks for looking into this.

@nornagon
Copy link
Contributor

Also, it'd be great if you could add a test for this behavior so we can avoid regressing it in future!

@lutzroeder
Copy link
Contributor Author
lutzroeder commented Dec 28, 2019 •
edited
Loading

@nornagon It would be great if you can take over this PR, create a validation build, add the necessary tests and port the fix to 7-1-x. As mentioned in the PR description my machine hangs trying to build Electron locally and I've spent enough time tracking down the regression you introduced.

@sidneys
Copy link
Contributor
sidneys commented Dec 29, 2019

This fix is urgently needed, as nearly all Electron In-App updates via GitHub using electron-updater are broken as of v7.1.3.

Please fast-track this.
Happy near year!

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label Dec 29, 2019
@nornagon nornagon merged commit ed6a387 into electron:master Dec 30, 2019
@welcome
Copy link
welcome bot commented Dec 30, 2019

Congrats on merging your first pull request! 🎉🎉🎉

@release-clerk
Copy link
release-clerk bot commented Dec 30, 2019

Release Notes Persisted

Fixed an issue in the net module where aborting a request during a redirect coul 8000 d cause an error to be thrown.

This was referenced Dec 30, 2019
Merged
Merged
@trop
Copy link
Contributor
trop bot commented Dec 30, 2019

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

@trop
Copy link
Contributor
trop bot commented Dec 30, 2019

I have automatically backported this PR to "7-1-x", please check out #21645

@ckerr ckerr changed the title fix: SimpleURLLoaderWrapper redirects (#21566) fix: SimpleURLLoaderWrapper redirects Apr 29, 2020
@dg1981 dg1981 mentioned this pull request Feb 20, 2021
Closed
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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SimpleURLLoaderWrapper broken with redirects in > 7.1.2
3 participants
@lutzroeder @nornagon @sidneys
0