RESOLVED FIXED in Firefox 64

Status

()

enhancement
P5
normal
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: twisniewski, Assigned: twisniewski)

Tracking

Trunk
mozilla64
Points:
---

Firefox Tracking Flags

(firefox64 fixed)

Details

(Whiteboard: [necko-triaged])

Attachments

(1 attachment)

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)
> 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
QA Contact: jduell.mcbugs

Comment 2

9 months ago
necko-triaged rubberstamp
Priority: -- → P5
Whiteboard: [necko-triaged]
Assignee

Comment 3

9 months 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

9 months ago
reject Fetch promises with (informative) TypeErrors when decoding fails, per spec
Assignee

Comment 5

9 months ago
Thanks, baku! I've rebased the patch, requesting check-in.
Assignee: nobody → twisniewski
Keywords: checkin-needed

Comment 6

9 months ago
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

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0c2d8b9fe4a7
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.