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

Perform XHR in the main context #7818

Merged
merged 1 commit into from
Jan 30, 2019
Merged

Perform XHR in the main context #7818

merged 1 commit into from
Jan 30, 2019

Conversation

kkaefer
Copy link
Contributor

@kkaefer kkaefer commented Jan 25, 2019

...rather than the worker's to preserve referrer information.

IE11 doesn't send the correct referrer for XHRs in a web worker. Instead, we're now doing the request from the main context and transfering the result back to the worker. Most modern browsers will continue to use the Fetch API introduced in #7371 and will not be affected by this change.

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page

I looked at the impact of transferring the ArrayBuffer/Image object in IE and it's very small, typically below 1ms. In any case, this code path will only be triggered in IE11 (and other older browsers that don't support the Fetch API).

I added a Cancelable mode to the Actor as well so that we can cancel requests across workers.

src/util/ajax.js Outdated Show resolved Hide resolved
@kkaefer kkaefer force-pushed the ie-referrers branch 2 times, most recently from 23a17ff to 68d2171 Compare January 25, 2019 15:27
Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

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

Pending resolution of comment

…referrer information

IE11 doesn't send the correct referrer for XHRs in a web worker. Instead, we're now doing the request from the main context and transfering the result back to the worker. Most modern browsers will continue to use the Fetch API and will not be affected by this change.
@kkaefer
Copy link
Contributor Author

kkaefer commented Jan 30, 2019

Also fixes #7603

@kkaefer kkaefer merged commit 390695a into master Jan 30, 2019
@kkaefer kkaefer deleted the ie-referrers branch January 30, 2019 16:19
// that we can get an accruate referrer header.
// - Requests for resources with the file:// URI scheme don't work with the Fetch API either. In
// this case we unconditionally use XHR on the current thread since referrers don't matter.
if (!/^file:/.test(requestParameters.url)) {

Choose a reason for hiding this comment

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

Doesn't work with relative URLs

Choose a reason for hiding this comment

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

I can confirm this, you have to specify the absolute path with the file protocol before passing it in, else it thinks it is an http request and throws an error about the file protocol like before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't support relative URLs intentionally — see the discussion at #3636 and linked tickets. That said, #8612 should now correctly classify these URLs as file URLs.

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.

5 participants