1198544 - FetchDriver can call OnResponseAvailable multiple times, leading to crashes

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: gdb log

OnStopRequest should probably not call OnResponseAvailable.  But it does need to report the error somehow, and it's not clear to me where that should go since InternalResponse::mTerminationReason appears to be completely unused.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

In my opinion, OnStopRequest should abort the pipe with an error code causing the body to become invalid.  Its too late to return an Error Response object.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

And then call OnResponseEnd().

Comment 3

[Josh Matthews [:jdm]]

Attachment: Prevent FetchDriver from creating multiple responses if OnStopRequest yields a failing status code

The test asserts without my fix and passes with it.

Comment 4

[Josh Matthews [:jdm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=99fccece6d11

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8659251 [details] [diff] [review]
Prevent FetchDriver from creating multiple responses if OnStopRequest yields a failing status code

Review of attachment 8659251 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8659251 [details] [diff] [review]
Prevent FetchDriver from creating multiple responses if OnStopRequest yields a failing status code

Review of attachment 8659251 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/fetch/FetchDriver.cpp
@@ +883,5 @@
> +    nsCOMPtr<nsIAsyncOutputStream> outputStream = do_QueryInterface(mPipeOutputStream);
> +    if (outputStream) {
> +      outputStream->CloseWithStatus(NS_BINDING_FAILED);
> +    }
> +    SucceedWithResponse();

Call ContinueHttpFetchAfterNetworkFetch()?

Can you add a comment that we resolve with success even though we failed since a successful Response has already been delivered in OnStartRequest().

::: dom/workers/test/serviceworkers/fetch/interrupt.sjs
@@ +10,5 @@
> +  response.write("Content-Type: text/plain; charset=utf-8\r\n");
> +  response.write("Cache-Control: no-cache, must-revalidate\r\n");
> +  response.write("\r\n");
> +
> +  response.write(body);

Nit: for loop with N = 10 and using N in the Content-Length :)

Comment 7

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/132aa442af953fab46644e34c60c6782350b703b
Bug 1198544 - Prevent FetchDriver from creating multiple responses if OnStopRequest yields a failing status code. r=nsm

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for linux debug bc5 failures like https://treeherder.mozilla.org/logviewer.html#?job_id=13975361&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ef95c4879fa

Comment 9

[Josh Matthews [:jdm]]

Attachment: interdiff

This fixes the crash observed on inbound, stemming from the changes requested during the review.

Comment 10

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8660150 [details] [diff] [review]
interdiff

Review of attachment 8660150 [details] [diff] [review]:
-----------------------------------------------------------------

Ah... I'd rather you revert the patch to call SucceedWithResponse() as before rather than introduce this variable. Sorry.

Comment 11

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5af5ff26d3858268b19ddfbd15306b6ed07a3816
Bug 1198544 - Prevent FetchDriver from creating multiple responses if OnStopRequest yields a failing status code. r=nsm

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5af5ff26d385