Closed Bug 1496621 Opened 3 years ago Closed 3 years ago


(Core :: DOM: Networking, enhancement, P5)




Tracking Status
firefox64 --- fixed


(Reporter: twisniewski, Assigned: twisniewski)


(Whiteboard: [necko-triaged])


(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:

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:

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.


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:

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:

  ... your code here.
Flags: needinfo?(amarchesini)
QA Contact: jduell.mcbugs
QA Contact: jduell.mcbugs
necko-triaged rubberstamp
Priority: -- → P5
Whiteboard: [necko-triaged]
Agreed, baku. I have a patch that seems fine on try for that approach:
reject Fetch promises with (informative) TypeErrors when decoding fails, per spec
Thanks, baku! I've rebased the patch, requesting check-in.
Assignee: nobody → twisniewski
Keywords: checkin-needed
Pushed by
reject Fetch promises with (informative) TypeErrors when decoding fails, per spec; r=baku
Keywords: checkin-needed
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.