1407384 - Make notifications respect channel.suspend

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement, P2)

Description

[Shane Caraveo (:mixedpuppy)]

Much like bug 1345893, there are more htto-on-* notifications that do not wait/suspend if channel.suspend is called during the notification.

Comment 1

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

Jason, can you find someone to take this?

Comment 2

[Shane Caraveo (:mixedpuppy)]

This is becoming a larger issue for extensions (specifically proxy and network extensions, ad blockers, privacy/security extensions, etc) and I suspect is the cause of bug 1501159.  Is there any way to bump this to P1?

Comment 3

[Dragana Damjanovic [:dragana]]

Kershaw, can you take a look?

Comment 4

[Kershaw Chang [:kershaw]]

@mixedpuppy,
Could you provide me a simple test case or detailed steps for reproducing this issue?
Thanks.

Comment 5

[Honza Bambas (:mayhemer)]

Kershaw, the goal here is to inspect the code of nsHttpChannel and make sure we handle possible Suspend() or Cancel() call properly*) from all on-modify-* handlers.  A good example patch is e.g. here: https://hg.mozilla.org/mozreview/gecko/rev/2b351c691e7a112eef2ab2c908bc1547d6dbcd9d#index_header

*) Most cases may be as simple as the example above, but could be some not that simple to split (we may need to take some spacial care or something) because of the stupid super complexity of the multi-async-loop opening of http channel.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Sorry I didn't get back sooner, meant to look for an example.  Thanks Honza.

Comment 7

[Kershaw Chang [:kershaw]]

(In reply to Honza Bambas (:mayhemer) from comment #5)
> Kershaw, the goal here is to inspect the code of nsHttpChannel and make sure
> we handle possible Suspend() or Cancel() call properly*) from all
> on-modify-* handlers.  A good example patch is e.g. here:
> https://hg.mozilla.org/mozreview/gecko/rev/
> 2b351c691e7a112eef2ab2c908bc1547d6dbcd9d#index_header
> 
> *) Most cases may be as simple as the example above, but could be some not
> that simple to split (we may need to take some spacial care or something)
> because of the stupid super complexity of the multi-async-loop opening of
> http channel.

We already handled Suspend()/Cancel() for:
 - http-on-modify-request
 - http-on-before-connect
 - http-on-examine-response

It looks like we didn't do this for:
 - http-on-examine-merged-response
 - http-on-examine-cached-response
 - http-on-useragent-request

So, the goal of this bug is to handle the above three http-on-* notifications properly.
Please correct me if my understanding is wrong. Thanks.

Comment 8

[Honza Bambas (:mayhemer)]

There is also a notification in nsHttpChannel::DoAuthRetry that I'm not sure now if has been fixed or not, but maybe it's already included in your list, Kershaw :)  Thanks!

Comment 9

[Shane Caraveo (:mixedpuppy)]

There are places where we do not handle async (rather than suspend/cancel) for stuff you listed.  Bug 1494033 is an example of that, where in DoAuthRetry we do not handle on-modify-request and we do not call on-before-connect (which technically sounds strange there, but would fix the request flow as documented, also fixing compatibility with Chrome).

useragent-request doesn't matter, we don't allow blocking on that.  The important aspect is to support async actions on any notification that is also exposed via webextensions.  The webRequest *blocking* APIs match this way:

onBeforeRequest
* works on primary code paths, but not all code paths
- http-on-modify-request

onBeforeSendHeaders
* works on primary code paths, but not all code paths
- http-on-before-connect

onHeadersReceived
* works only with http-on-examine-response
- http-on-examine-response
- http-on-examine-cached-response
- http-on-examine-merged-response

onAuthRequired * 
* works fine everywhere I am aware of
- any calls to nsIAuthPrompt2 APIs
- http-on-examine-response
- http-on-examine-cached-response
- http-on-examine-merged-response

* we do not call onAuthRequired in response to those notifications, but they are used to hook up the auth listener.  As such, webRequest doesn't need them to be blocking for onAuthRequired, but it does for onHeadersReceived.

Comment 10

[Shane Caraveo (:mixedpuppy)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #9)
> There are places where we do not handle async (rather than suspend/cancel)

I make that distinction from a webrequest perspective because other things can happen, such as setting headers.  In nsHttpChannel, this is really about handling suspend.  webrequest will suspend the channel when it needs to block.

Comment 11

[Kershaw Chang [:kershaw]]

Attachment: Bug 1407384 - P1: Notify "http-on-before-connect" and "http-on-modify-request" observers in DoAuthRetry

The goal in this patch is to notify "http-on-before-connect" and "http-on-modify-request" observers in DoAuthRetry and also handle the case when the channel is canceled or suspended by observer.
This patch includes:
1. A general class AsyncChannelResumeHandler to handle the case when the channel is canceled or suspended.
2. Split the origional DoAuthRetry into 3 functions: DoAuthRetry, ContinueDoAuthRetry, and DoConnect.
3. Notify "http-on-modify-request" observers in DoAuthRetry.
4. Notify "http-on-before-connect" observers in ContinueDoAuthRetry.

Comment 12

[Kershaw Chang [:kershaw]]

Attachment: Bug 1407384 - P2: test for suspending the channel in DoAuthRetry

Test steps:
1. Register the observers for http-on-modify-request and http-on-before-connect.
2. There are 3 cases to be tested.
   - suspend in http-on-modify-request
   - suspend in http-on-before-connect
   - suspend in both observers
3. See if the channel can be resumed correctly.

Comment 13

[Kershaw Chang [:kershaw]]

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

Comment 14

[Kershaw Chang [:kershaw]]

Attachment: Bug 1407384 - P3: Handle the case if the channel is suspended or canceled by "http-on-examine-merged-response" observer

1. Split ProcessPartialContent() into 2 functions: ProcessPartialContent() and ContinueProcessPartialContent()
2. Use CallOrWaitForResume() as the bridge between these two functions.
3. The same logic goes to ProcessNotModified().

Comment 15

[Kershaw Chang [:kershaw]]

ni for https://phabricator.services.mozilla.com/D10741#399839

Thanks.

Comment 16

[Kershaw Chang [:kershaw]]

(In reply to Kershaw Chang [:kershaw] from comment #15)

ni for https://phabricator.services.mozilla.com/D10741#399839

Thanks.

Remove ni since Honza already replied on phabricator.

Comment 17

[Kershaw Chang [:kershaw]]

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

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7e8129336381
P1: Notify "http-on-before-connect" and "http-on-modify-request" observers in DoAuthRetry r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/18abdcc812a6
P2: test for suspending the channel in DoAuthRetry r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/f5b0ec66117d
P3: Handle the case if the channel is suspended or canceled by "http-on-examine-merged-response" observer r=mayhemer

Comment 19

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/7e8129336381
https://hg.mozilla.org/mozilla-central/rev/18abdcc812a6
https://hg.mozilla.org/mozilla-central/rev/f5b0ec66117d