Make notifications respect channel.suspend

RESOLVED FIXED in Firefox 66

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: mixedpuppy, Assigned: kershaw)

Tracking

(Blocks 1 bug)

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

(Whiteboard: [necko-triaged])

Attachments

(3 attachments)

Reporter

Description

2 years ago
Much like bug 1345893, there are more htto-on-* notifications that do not wait/suspend if channel.suspend is called during the notification.
Jason, can you find someone to take this?
Assignee: nobody → jduell.mcbugs
Whiteboard: [necko-triaged]
Priority: -- → P3
Blocks: 1447551
No longer blocks: webextensions-startup

Updated

8 months ago
Assignee: jduell.mcbugs → nobody
Flags: needinfo?(dd.mozilla)
Reporter

Updated

8 months ago
Blocks: 1494033
Reporter

Updated

8 months ago
See Also: → 1501159
Reporter

Comment 2

8 months ago
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?
Flags: needinfo?(sdeckelmann)
Flags: needinfo?(honzab.moz)
Kershaw, can you take a look?
Flags: needinfo?(dd.mozilla) → needinfo?(kershaw)
Assignee

Updated

8 months ago
Assignee: nobody → kershaw
Flags: needinfo?(kershaw)
Priority: P3 → P2
Assignee

Comment 4

8 months ago
@mixedpuppy,
Could you provide me a simple test case or detailed steps for reproducing this issue?
Thanks.
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(sdeckelmann)
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.
Flags: needinfo?(honzab.moz)
Reporter

Comment 6

8 months ago
Sorry I didn't get back sooner, meant to look for an example.  Thanks Honza.
Flags: needinfo?(mixedpuppy)
Assignee

Comment 7

8 months ago
(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.
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!
Reporter

Comment 9

8 months ago
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.
Reporter

Comment 10

8 months ago
(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.
Assignee

Comment 11

8 months ago
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.
Assignee

Comment 12

7 months ago
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.
Assignee

Comment 14

7 months ago
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().
Assignee

Comment 15

6 months ago
Flags: needinfo?(honzab.moz)
Assignee

Comment 16

6 months ago

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

Flags: needinfo?(honzab.moz)

Comment 18

5 months ago
Pushed by kjang@mozilla.com:
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

5 months ago
bugherder
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.