Moving the error handling out of network loader...

[wafflespeanut]

Fixes #8678

[wafflespeanut]

r? @jdm

I'm sorry it took so long (unanticipated circumstances) :)

[wafflespeanut]

This is a half-baked PR, btw. It doesn't currently handle the propagated errors, and we also have to update the unit tests (just checking if the changes are okay).

---

Review status: 0 of 13 files reviewed at latest revision, 3 unresolved discussions.

---

components/net/http_loader.rs, line 147 [r1] (raw file):
Oops, forgot to remove this :)

---

components/net_traits/lib.rs, line 164 [r1] (raw file):
also, enlighten me - why we should take a Result<Metadata, LoadError> here?

---

components/net_traits/lib.rs, line 419 [r1] (raw file):
I know, this is ugly! I dunno how else we could achieve this. I decided to go for this, because send_error takes another argument for URL, which is unnecessary, because almost all the LoadError variants have Url in them, which we can utilize. It's also probably useful in the future while displaying errors, but I'm not sure whether the current design is good...

---

Comments from the review on Reviewable.io

[eefriedman]

Currently fails tidy.

I'm not sure what the point of this is: it's not like anything is actually going to branch on the error kind. The fetch algorithm requires treating all network errors exactly the same way.

---

Review status: 0 of 13 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

components/net_traits/lib.rs, line 164 [r1] (raw file):
Metadata contains a bunch of stuff which doesn't actually make sense unconditionally in this context: headers, the status code, even the correct final URL aren't really known in the case of a network error.

I guess you could argue that we should simply skip calling headers_available in the case of a network error.

---

components/net_traits/lib.rs, line 419 [r1] (raw file):
Make LoadError a struct containing an enum, and put the URL in there?

---

Comments from the review on Reviewable.io

[bors-servo]

☔ The latest upstream changes (presumably #8862) made this pull request unmergeable. Please resolve the merge conflicts.

[wafflespeanut]

(just made the LoadError as struct, and fixed the conflicts)
TODO: handling SSL validation and updating unit tests

---

Review status: 0 of 13 files reviewed at latest revision, 3 unresolved discussions.

---

components/net_traits/lib.rs, line 419 [r1] (raw file):
Nice :)

---

Comments from the review on Reviewable.io

[bors-servo]

☔ The latest upstream changes (presumably #8867) made this pull request unmergeable. Please resolve the merge conflicts.

[jdm]

@eefriedman There's going to be code somewhere that does report useful errors for page loads - consider how web browsers report that a URL didn't resolve, or a page is in an infinite redirect loop, or an SSL certificate didn't validate, etc. I agree that the fetch spec doesn't permit differentiating; that's something we'll need to bring up.

[jdm]

And yes, we'll need to make the SSL error appear again before this PR can merge.

[jdm]

-S-awaiting-review +S-needs-code-changes

---

Reviewed 2 of 13 files at r1, 11 of 11 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

components/net/http_loader.rs, line 140 [r2] (raw file):
I think it is better to leave this in, and have separate internal vs. external network errors. For the immediate case, the external set could simply include an SSL case and an InternalFailure, reducing the potential scope of errors for consumers to deal with until we figure out what that set should be.

---

components/net_traits/lib.rs, line 164 [r1] (raw file):
I think right now the risk of skipping headers_available is that it would be too easy to write code that assumes that it was called and a Metadata value was obtained. I'd like to try have a Result value instead and see how that ends up in practice.

---

Comments from the review on Reviewable.io

[wafflespeanut]

@jdm Tiny clarifications. Once this is settled, I'll go ahead and update the tests :)

---

Review status: 0 of 13 files reviewed at latest revision, 5 unresolved discussions.

---

components/net/http_loader.rs, line 147 [r3] (raw file):
I'm not sure if this is okay, but I thought this would be neat, since we only want ConnectionAborted and Ssl variants, and everything else is passed to send_error anyway.

---

components/script/dom/servohtmlparser.rs, line 244 [r3] (raw file):
Is it correct to return if we encounter an error?

---

components/script/dom/servohtmlparser.rs, line 322 [r3] (raw file):
I dunno how the Internal and SslValidation variants should be handled here :/

---

Comments from the review on Reviewable.io

[jdm]

-S-awaiting-review +S-needs-code-changes

---

Reviewed 13 of 13 files at r3.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

components/script/dom/htmllinkelement.rs, line 279 [r3] (raw file):
Let's return if this is None.

---

components/script/dom/servohtmlparser.rs, line 244 [r3] (raw file):
We should modify page_fetch_complete to accept Option<Metadata> instead. Additionally, to avoid regressing existing behaviour we should synthesize a Metadata for the SSL failure case. We'll then want to synthesize the SSL failure page content like some of the Content-Type cases below. Does that make sense?

---

components/script/dom/servohtmlparser.rs, line 322 [r3] (raw file):
This is probably ok as-is.

---

Comments from the review on Reviewable.io

[jdm]

This is almost there! I'd also like to add a reference test for this:

• make the non-reference file have an iframe that loads an HTTPS document
• the reference file loads about:sslfail (and make that load the contents of badcert.html like about:failure)

[wafflespeanut]

addressed the code changes, haven't added the reftest yet...

[jdm]

Note that the unit tests still need to be updated to compile.

[jdm]

-S-awaiting-review +S-needs-code-changes

---

Reviewed 4 of 19 files at r5, 13 of 13 files at r6.
Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

components/script/dom/servohtmlparser.rs, line 252 [r4] (raw file):
and_then, in that case.

---

components/script/dom/xmlhttprequest.rs, line 796 [r6] (raw file):
Hmm, we should probably be calling process_partial_response before returning here.

---

Comments from the review on Reviewable.io

[wafflespeanut]

Review status: 11 of 21 files reviewed at latest revision, 10 unresolved discussions.

---

tests/wpt/mozilla/tests/mozilla/sslfail.html, line 8 [r8] (raw file):
@jdm Did I understand this correctly? Because, the test doesn't pass :/
Also, enlighten me - how does loading a HTTPS document in iframe generate an SSL error? Isn't that already allowed?

(one case I've seen is "mixed content", where the document is secure with "https", but the iframe tries to load a "http", but I dunno how I can write a test for that)

---

Comments from the review on Reviewable.io

[wafflespeanut]

Review status: 11 of 21 files reviewed at latest revision, 11 unresolved discussions.

---

components/net/resource_thread.rs, line 142 [r8] (raw file):
Oops, I've definitely screwed it here, haven't I? :)

---

Comments from the review on Reviewable.io

[wafflespeanut]

Okay, the test still doesn't pass :/

[jdm]

The HTTPS load is expected to fail because of #6919 right now. It's possible the test is failing because the load of badcert.html in the non-about:sslfail iframe includes a relative URL for the picture, and that would be treated as relative to the original failing URL...

[bors-servo]

☔ The latest upstream changes (presumably #9780) made this pull request unmergeable. Please resolve the merge conflicts.

[wafflespeanut]

superseded by #9942