Closed Bug 1286258 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Networking, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox47 --- wontfix
firefox48 --- fixed
firefox49 --- fixed
firefox-esr45 --- disabled
firefox50 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

(Whiteboard: [necko-active])

Attachments

(1 file)

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.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Blocks: 1286008
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.
Attachment #8770154 - Flags: review?(valentin.gosu)
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!
Attachment #8770154 - Flags: review?(valentin.gosu) → review+
Whiteboard: [necko-active]
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c297b5c49ed
Abort http channels that fail ContinueAsyncOpen() during service worker handling. r=valentin
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.
Attachment #8770154 - Flags: approval-mozilla-beta?
Attachment #8770154 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/3c297b5c49ed
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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.
Attachment #8770154 - Flags: approval-mozilla-beta?
Attachment #8770154 - Flags: approval-mozilla-beta+
Attachment #8770154 - Flags: approval-mozilla-aurora?
Attachment #8770154 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.