1286258 - HttpChannelChild::ResetInterception() ignores errors from ContinueAsyncOpen()

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

Currently HttpChannelChild::ResetInterception() does this:

  // Continue with the original cross-process request
  nsresult rv = ContinueAsyncOpen();
  NS_ENSURE_SUCCESS_VOID(rv);

This leaves the channel in a half-opened state and dangling.  We should really abort the channel in this case.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Abort http channels that fail ContinueAsyncOpen() during service worker han dling. r=valentin

This fixes the leak for me.  It simply aborts the channel in cases where we get a failed ContinueAsyncOpen().  Previously we were leaving the channels in a stuck state.

Comment 2

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8770154 [details] [diff] [review]
Abort http channels that fail ContinueAsyncOpen() during service worker han dling. r=valentin

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

Looks good.
Thanks!

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c297b5c49ed
Abort http channels that fail ContinueAsyncOpen() during service worker handling. r=valentin

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8770154 [details] [diff] [review]
Abort http channels that fail ContinueAsyncOpen() during service worker han dling. r=valentin

Approval Request Comment
[Feature/regressing bug #]: Service workers
[User impact if declined]: If an error is hit while opening a network channel on a site controlled by a service worker we may not handle the error correctly.  The result could be a leaked window (like bug 1286008) or possibly just an infinitely spinning navigation request.  This patch makes us correctly abort the channel so our normal about:neterror page is shown.
[Describe test coverage new/current, TreeHerder]: Existing mochitests and wpt tests.  No new tests since its difficult to trigger an error consistently for these paths.
[Risks and why]: Minimal.  We are simply connecting existing, well tested error handling to an error path that was added for service workers.
[String/UUID change made/needed]: None.

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3c297b5c49ed

Comment 7

[Gerry Chang [:gchang]]

Comment on attachment 8770154 [details] [diff] [review]
Abort http channels that fail ContinueAsyncOpen() during service worker han dling. r=valentin

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

This patch fixes a error handling issue in service worker. Take it 48 beta 8 and aurora.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e98dd5b41a69

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2ba3feb910fb