Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @metajack (or someone else) soon.

Heads up! This PR modifies the following files:

• @fitzgen: components/script/dom/globalscope.rs, components/script/fetch.rs
• @KiChjang: components/script/dom/globalscope.rs, components/script/fetch.rs

Warning

• These commits modify script code, but no tests are modified. Please consider adding a test!

@bors-servo r+

📌 Commit af91972 has been approved by metajack

⌛ Testing commit af91972 with merge 43e4d17...

@metajack Hey! Thanks for the review but I think the PR needs some more work, I updated the description

@bors-servo r-

I'm in Tokyo at the CSS WG meeting, so probably one of @jdm or @asajeffrey or @Manishearth should provide the feedback you are asking for.

@metajack That's fine, thanks! Have a nice one.

💔 Test failed - mac-rel-wpt1

@brainlessdeveloper You'll also want to update the test results since presumably this made something pass now :)

After carefully reading the spec, I have two concerns to present to you. Maybe I am just extremely confused, but it seems to me that we can't just pass a ServoUrl to RequestInit.

I drew this little flow chart, I hope it doesn't confuse you:

When setting origin here, we instantiate a RequestInit from a Request. About this step, the spec says:

A request has an associated origin, which is "client" or an origin. Unless stated otherwise it is "client". "client" is changed to an origin during fetching. It provides a convenient way for standards to not have to set request’s origin. Request’s origin can be changed during redirects too.

Changed to an origin during fetching means we need to pass the enum ImmutableOrigin to RequestInit. As per a part of the spec, an origin can be either an opaque origin, or a tuple of a schema, a host, and a port (and a domain that's always null). This is reflected neatly by the ImmutableOrigin tuple. As far as I've understood, the origin will be an opaque origin if it's generated from a file path. The spec says that an opaque origin will always be a string containing null.

The ImmutableOrigin struct contains a couple of handy methods that deal with the logic to figure out if the origin is an opaque origin or if it's a tuple. Another method, the unicode_serialization method, returns a string from an ImmutableOrigin. This string will be either "null" if it's an opaque origin or contain the schema, host, and port if it's a tuple.

In my doodle, there's Q1 (in blue), representing my first concern: I can instantiate a Url struct from a tuple origin, and then create a ServoUrl from it, and pass it to RequestInit's origin field. But I can't do this with an opaque origin, because neither Url or ServoUrl will take a string "null" to be instantiated.

About my second concern (Q2 in blue in the doodle): the spec says a request's "client" is changed to an origin during fetching. But there's still the possibility that the Request from which we're trying to instantiate a RequestInit contains an Origin of type Client.

I think RequestInit should not take an origin of type ServoUrl, I think it should take... an origin of type Origin! And it should deal with transforming it to a string afterwards. The problem is, if you grep for RequestInit { in the project, it appears 105 times, and I'm quite sure all of them serve an origin in a different way, and none of them account for the fact that the origin should be "null" in some cases. So if I'm right, there's quite a bit of refactoring work.

Please, tell me if I've missed something. To me it seems just passing GlobalScope::current().get_url() blindly does not comply with the spec, and will lead to a lot of tears in the future.

You are correct, and that's what the TODO in request_init_from_request is referencing. Going back to my suggestion of adding an origin method to GlobalScope, we can have the Window implementation return the associated document's immutable origin, and for workers return an origin based on get_url().origin(). I suspect the 105 uses you see are overcounting; here's the list of the important ones:

components/gfx/font_cache_thread.rs
215:                let request = RequestInit {

components/net_traits/request.rs
164:    fn default() -> RequestInit {

components/script/dom/dedicatedworkerglobalscope.rs
176:            let request = RequestInit {

components/script/dom/eventsource.rs
333:    pub fn request(&self) -> RequestInit {
358:        let mut request = RequestInit {

components/script/dom/htmlimageelement.rs
261:        let request = RequestInit {

components/script/dom/htmlmediaelement.rs
542:            let request = RequestInit {

components/script/dom/htmlscriptelement.rs
240:    let request = RequestInit {

components/script/dom/serviceworkerglobalscope.rs
161:            let request = RequestInit {

components/script/dom/workerglobalscope.rs
206:            let request = NetRequestInit {

components/script/dom/xmlhttprequest.rs
582:        let mut request = RequestInit {

components/script/fetch.rs
44:    NetTraitsRequestInit {

components/script/layout_image.rs
71:    let request = FetchRequestInit {

components/script/script_thread.rs
2125:        let request = RequestInit {

components/script/stylesheet_loader.rs
240:        let request = RequestInit {

Then there are some in tests/unit/net/http_loader.rs as well, but those shouldn't require as much care.

So in summary I should change RequestInit to take only an Origin in the origin field, and then, in the uses that already there, instantiate an Origin from the URL each use case has implemented. The problem is the following: in some cases, that request may have been created from the browser console, in which case its origin field should be the string "null". If I instantiate an Origin from the URLs that are currently there, the case where the origin has to be "null" will be omitted.

Edit: I was wrong about that, read the following comments.

I'm not sure I understand the connection to the browser console here. If such a request occurred, I would expect it to use the origin of the page associated with the console, not null.

I'm sorry, I was wrong about the browser console, and you're right: if I open my console right now and go fetch('http://somewebsite.com'), the request's origin will be set to https://github.com.

What I was trying to explain was the possibility of the request origin being "null". It can happen in (AFAIK) two situations:

• The request was not done from a hosted website but from a file in the user's computer.
• If a cross-origin resource redirects to another resource at a new origin, (the value of origin should be set to "null" after redirecting).

Currently, all the uses of RequestInit neglect this possibility.

The redirection happens as part of the fetch implementation, so the value for RequestInit is not affected by that. file:// URLs are the easiest example of null origins, but it could also happen from blob:// URLs too I believe. If we encapsulate the global origin within the origin() method, we can fix those for free later on.

Seems good, I'll be working on it tonight. Thanks for your help!

Should I use the origin method from the document (this line), or should I implement a new origin method for GlobalScope ?

I suppose in the case of implementing a new method for GlobalScope, I would actually return the document's origin, something like this: window.Document().origin

...which makes it sort of futile to implement it, because I could just get the document and use its origin. What do you think?

Good point, it looks like what's happening is that the same-origin check is failing because the request origin is an opaque origin, rather than the origin that originated the request. Tracking this down, it looks like the culprit is https://github.com/brainlessdeveloper/servo/blob/5dc8fc99a89478a4827677fc256664b689a024fa/components/script/script_thread.rs#L2289:

    fn pre_page_load(&self, incomplete: InProgressLoad, load_data: LoadData) {
        let id = incomplete.pipeline_id.clone();
        let mut req_init = RequestInit {
            url: load_data.url.clone(),
            method: load_data.method,
            destination: Destination::Document,
            credentials_mode: CredentialsMode::Include,
            use_url_credentials: true,
            pipeline_id: Some(id),
            referrer_url: load_data.referrer_url,
            referrer_policy: load_data.referrer_policy,
            headers: load_data.headers,
            body: load_data.data,
            redirect_mode: RedirectMode::Manual,
            // TODO: Add a proper origin to pre_page_load
            // https://github.com/servo/servo/issues/17238
            .. RequestInit::default()
        };
    ...

Adding:

            origin: incomplete.origin.immutable().clone(),

Seems to do the trick.

@brainlessdeveloper: do you want to make this change and we'll see if this fixes everything?

I ran the wpt test suite locally. Results at https://gist.github.com/asajeffrey/fec16fc305de171b758eafe42c111333.

Eyeballing it, that looks like quite a few PASS [expected FAIL], the usual intermittents, and the issue discussed above in #16508 (comment).

So this is all looking good with that one-line change. (Usual story: days of debugging to find one line to change, sigh.)

@asajeffrey on it right now! :)

Thanks!

So, all the PASS, expected FAIL tests I see are in the quirks mode module (presumably nothing to do with this) and then the intermittent tests we dug up some days ago. The only new thing I see is this:

  ▶ Unexpected subtest result in /cors/allow-headers.htm:
  └ PASS [expected FAIL] Allow origin: _http://web-platform.test:8000___[tab]_

I guess I should change the expectation for this one?

Ok, the cors tests pass with no unexpected results, as well as referrer-policy and fetch/api (in this last one, excluding the intermittent ones ofc). So I think we should be good to go, hopefully.

@bors-servo: try

⌛ Trying commit 6032940 with merge 006e36e...

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
State: approved= try=True

Finally! :D

I'll have a last look at it when I'm in front of a computer, I'm not expecting any issues, we should be good to merge! Thanks for your patience.

Thanks for your patience too, especially considering the last failures were caused by things I typed, and I didn't manage to debug it 🙈

OK, we are good to go! @bors-servo r+

📌 Commit 6032940 has been approved by asajeffrey

⌛ Testing commit 6032940 with merge 2bb4f65...

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: asajeffrey
Pushing 2bb4f65 to master...

Woo hoo! Just shy of the three month anniversary. Thanks @brainlessdeveloper!

Thank you for your help! :)

I feel a bit guilty about the last one here because I've mentioned it in the linked issue here but I did not speak up about the probability of the root cause being that. Sorry.

@KiChjang np, it was a good exercise in figuring out how our fetch code works!