Closed
Bug 1496621
Opened 6 years ago
Closed 6 years ago
Failing to reject with a TypeError in http://w3c-test.org/fetch/content-encoding/bad-gzip-body.any.html
Categories
(Core :: DOM: Networking, enhancement, P5)
Core
DOM: Networking
Tracking
()
RESOLVED
FIXED
mozilla64
Tracking | Status | |
---|---|---|
firefox64 | --- | fixed |
People
(Reporter: twisniewski, Assigned: twisniewski)
Details
(Whiteboard: [necko-triaged])
Attachments
(1 file)
In most of the test-cases in the WPT org/fetch/content-encoding/bad-gzip-body, we're rejecting the promises as we should, but with an Abort error. We should be rejecting with a TypeError.
This basically boils down to having to reject with the right error here: https://searchfox.org/mozilla-central/source/dom/fetch/FetchConsumer.cpp#753
An NS_BINDING_FAILED error is being caught by this case, and turned into an Abort rejection. We could simply convert it into a type-error with a more descriptive message, like this:
> if (aStatus == NS_BINDING_FAILED) {
> // Decoding errors should reject with a TypeError
> IgnoredErrorResult rv;
> rv.ThrowTypeError<MSG_DOM_DECODING_FAILED>();
> localPromise->MaybeReject(rv);
> } else {
> // Other errors should reject with that error
> localPromise->MaybeReject(aStatus);
> }
A try-run shows that this approach passes the WPTs that fail: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b93c3bc9cf3760c936f0b3c7d1083f2e03ae5f7
But I'm not sure if it's quite the right fix (or a least a complete one). It seems to be what we *should* be doing, according to the spec-text:
>When read is fulfilled with an object whose done property is false and whose value property is a Uint8Array object, append the value property to bytes and run the above step again.
>When read is fulfilled with an object whose done property is true, resolve promise with bytes.
>When read is fulfilled with a value that matches with neither of the above patterns, reject promise with a TypeError.
>When read is rejected with an error, reject promise with that error.
(From https://fetch.spec.whatwg.org/#concept-read-all-bytes-from-readablestream)
That is, the stream did read successfully, but not decode successfully, so we'd hit the third case, at least according to the failing WPTs.
But are there other internal errors that we would need to handle this way, or is there a better way to go about this?
What are your thoughts, baku?
Flags: needinfo?(amarchesini)
Comment 1•6 years ago
|
||
> What are your thoughts, baku?
This is not the right fix. The bug is here:
https://searchfox.org/mozilla-central/rev/924e3d96d81a40d2f0eec1db5f74fc6594337128/dom/fetch/FetchDriver.cpp#1188-1191
Here we use NS_BINDING_FAILED ignoring the real 'aStatusCode'. Probably we should do:
outputStream->CloseWithStatus(NS_FAILED(aStatusCode) ? aStatusCode : NS_BINDING_FAILED)
Then, in FetchConsumer.cpp we should check this:
if (aStatus == NS_ERROR_INVALID_CONTENT_ENCODING) {
... your code here.
Flags: needinfo?(amarchesini)
QA Contact: jduell.mcbugs
Updated•6 years ago
|
QA Contact: jduell.mcbugs
Assignee | ||
Comment 3•6 years ago
|
||
Agreed, baku. I have a patch that seems fine on try for that approach: https://treeherder.mozilla.org/#/jobs?repo=try&revision=52fb210a87019f38450ef59e2a73b28977937ff2
Assignee | ||
Comment 4•6 years ago
|
||
reject Fetch promises with (informative) TypeErrors when decoding fails, per spec
Assignee | ||
Comment 5•6 years ago
|
||
Thanks, baku! I've rebased the patch, requesting check-in.
Assignee: nobody → twisniewski
Keywords: checkin-needed
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0c2d8b9fe4a7
reject Fetch promises with (informative) TypeErrors when decoding fails, per spec; r=baku
Keywords: checkin-needed
Comment 7•6 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in
before you can comment on or make changes to this bug.
Description
•