-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Moving the error handling out of network loader... #8851
Conversation
r? @jdm I'm sorry it took so long (unanticipated circumstances) :) |
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): components/net_traits/lib.rs, line 164 [r1] (raw file): components/net_traits/lib.rs, line 419 [r1] (raw file): Comments from the review on Reviewable.io |
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): 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): Comments from the review on Reviewable.io |
☔ The latest upstream changes (presumably #8862) made this pull request unmergeable. Please resolve the merge conflicts. |
b55f688
to
9581dd3
Compare
(just made the Review status: 0 of 13 files reviewed at latest revision, 3 unresolved discussions. components/net_traits/lib.rs, line 419 [r1] (raw file): Comments from the review on Reviewable.io |
☔ The latest upstream changes (presumably #8867) made this pull request unmergeable. Please resolve the merge conflicts. |
@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. |
And yes, we'll need to make the SSL error appear again before this PR can merge. |
-S-awaiting-review +S-needs-code-changes Reviewed 2 of 13 files at r1, 11 of 11 files at r2. components/net/http_loader.rs, line 140 [r2] (raw file): components/net_traits/lib.rs, line 164 [r1] (raw file): Comments from the review on Reviewable.io |
9581dd3
to
8bbd294
Compare
8bbd294
to
eb755c4
Compare
@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): components/script/dom/servohtmlparser.rs, line 244 [r3] (raw file): components/script/dom/servohtmlparser.rs, line 322 [r3] (raw file): Comments from the review on Reviewable.io |
-S-awaiting-review +S-needs-code-changes Reviewed 13 of 13 files at r3. components/script/dom/htmllinkelement.rs, line 279 [r3] (raw file): components/script/dom/servohtmlparser.rs, line 244 [r3] (raw file): components/script/dom/servohtmlparser.rs, line 322 [r3] (raw file): Comments from the review on Reviewable.io |
This is almost there! I'd also like to add a reference test for this:
|
33d48bb
to
297afa8
Compare
addressed the code changes, haven't added the reftest yet... |
Note that the unit tests still need to be updated to compile. |
-S-awaiting-review +S-needs-code-changes Reviewed 4 of 19 files at r5, 13 of 13 files at r6. components/script/dom/servohtmlparser.rs, line 252 [r4] (raw file): components/script/dom/xmlhttprequest.rs, line 796 [r6] (raw file): Comments from the review on Reviewable.io |
297afa8
to
440b34f
Compare
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): (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 |
Review status: 11 of 21 files reviewed at latest revision, 11 unresolved discussions. components/net/resource_thread.rs, line 142 [r8] (raw file): Comments from the review on Reviewable.io |
1c9d475
to
5bec4d4
Compare
Okay, the test still doesn't pass :/ |
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... |
☔ The latest upstream changes (presumably #9780) made this pull request unmergeable. Please resolve the merge conflicts. |
superseded by #9942 |
Moving the error handling out of network loader Rebase of #8851. Fixes #8678. Fixes #9944. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9942) <!-- Reviewable:end -->
Fixes #8678