No compatible mapping for onBeforeSendHeaders and onSendHeaders

NEW
Unassigned

Status

()

Toolkit
WebExtensions: Request Handling
P3
normal
2 months ago
a month ago

People

(Reporter: mixedpuppy, Unassigned)

Tracking

(Blocks: 1 bug)

49 Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: triaged)

(Reporter)

Description

2 months ago
The notification we use prior to a network connection is http-on-modify.  WebRequest onBeforeRequest, onBeforeSendHeaders and onSendHeaders all happen during this notification as we have no notifications that match the latter two.

This has compatibility implications and limits some advanced use.

It seems to me we could add new notifications that better match the timing for these.

onSendHeaders is non-blocking and informational only, cannot modify/cancel anything, so an observer notification fired in nsHttpChannel::ContinueConnect just prior to SetupTransaction could work for that.

onBeforeSendHeaders is a little harder to identify where a reasonable notification could exist.  It can block and modification is allowed.  But it seems it should happen later in the request cycle than http-on-modify.  

Looking for feedback on the potential for better supporting these listeners, whether something exists that may match a bit better, or we can add new notifications.
(Reporter)

Updated

2 months ago
Flags: needinfo?(bzbarsky)

Comment 1

2 months ago
This is a question for the necko folks, not me.

That said, I believe http-on-modify-request is the last thing that really happens in Gecko before we start putting data on the wire, so I'm pretty sure it should match up to onBeforeSendHeaders.  But I could be totally off.
Flags: needinfo?(bzbarsky) → needinfo?(mcmanus)
Flags: needinfo?(mcmanus) → needinfo?(dd.mozilla)

Updated

2 months ago
Priority: -- → P3
Whiteboard: triaged

Updated

2 months ago
See Also: → bug 1353489
(In reply to Shane Caraveo (:mixedpuppy) from comment #0)
> The notification we use prior to a network connection is http-on-modify. 
> WebRequest onBeforeRequest, onBeforeSendHeaders and onSendHeaders all happen
> during this notification as we have no notifications that match the latter
> two.
> 
> This has compatibility implications and limits some advanced use.
> 
> It seems to me we could add new notifications that better match the timing
> for these.
> 
> onSendHeaders is non-blocking and informational only, cannot modify/cancel
> anything, so an observer notification fired in
> nsHttpChannel::ContinueConnect just prior to SetupTransaction could work for
> that.
> 
> onBeforeSendHeaders is a little harder to identify where a reasonable
> notification could exist.  It can block and modification is allowed.  But it
> seems it should happen later in the request cycle than http-on-modify. 

I think onBeforeSendHeaders best matches http-on-modify. After http-on-modify we only do the classification (is it tracking url...).

For onSendHeaders, should this be when nsHttpChannel makes a transaction or later? Depending whether we have http1 or http2 connection a transaction can spend some time in a queue before being dispatched. Probably it is better to add it before SetupTransaction, because the point when we really send headers can happen more than one time if TLS1.3 early-data fails (or we want that information?). 

Could onBeforeRequest correspond to http-on-opening-request which is in nsHttpChannel::AsyncOpen or is this too early?

> 
> Looking for feedback on the potential for better supporting these listeners,
> whether something exists that may match a bit better, or we can add new
> notifications.
Flags: needinfo?(dd.mozilla)
(Reporter)

Updated

2 months ago
Blocks: 1353489
(Reporter)

Comment 3

2 months ago
(In reply to Dragana Damjanovic [:dragana] from comment #2)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #0)

> I think onBeforeSendHeaders best matches http-on-modify. After
> http-on-modify we only do the classification (is it tracking url...).

It looks like quite a bit happens in there between on-modify and making a connection, including adding headers, setting up HSTS, possibly changing url from HTTP to HTTPS, etc.  onBeforeSendHeaders needs to happen after all that.

> For onSendHeaders, should this be when nsHttpChannel makes a transaction or
> later? 

onSendHeaders should basically happen after all onBeforeSendHeaders listeners are called, otherwise it doesn't have a specific timing requirement.   The purpose for onSendHeaders is to allow addons to see what will actually get sent without allowing them to make changes.  Timing should look like:

onBeforeRequest -> http-on-modify-request

onBeforeSendHeaders -> after on-modify and after an modifications (headers, etc) made by firefox, but before sending headers across the wire and before it is too late to make changes to headers.  

onSendHeaders -> after all addons with blocking onBeforeSendHeaders listeners are finished

> add it before SetupTransaction, because the point when we really send
> headers can happen more than one time if TLS1.3 early-data fails (or we want
> that information?). 

Only if it's going to change request data (e.g. new header added)

After thinking this through more:

I think "before SetupTransaction" may be the place (assuming we can still change headers) to call onBeforeSendHeaders.  We would only need one notification from httpchannel, onSendHeaders can be handled in webextension code without any specific support from httpchannel.

After thinking it through again, I essentially need one new notification in httpchannel:

   http-on-opening-request
   http-on-modify-request
   httpchannel does additional work, adds some headers, does hsts, etc.
*  http-on-before-connect-request (just prior to SetupTransaction?)
   httpchannel makes or queues connection and sends headers
   etc.

During http-on-before-connect-request WebRequest would call all onBeforeSendHeaders listeners first, then call all onSendHeaders listeners.

> Could onBeforeRequest correspond to http-on-opening-request which is in
> nsHttpChannel::AsyncOpen or is this too early?

That wouldn't fix the problem with onBeforeSendHeaders, and might introduce other problems (I'd have to investigate).
Flags: needinfo?(dd.mozilla)
Should onBeforeSendHeaders be sent for request that are served from cache? SetupTransaction is triggered only for requests going to the network.

http-on-modify-request is used by some addons to inspect a request and redirect it if it needs to be blocked, do we still have something like that in webextensions?
Flags: needinfo?(dd.mozilla) → needinfo?(mixedpuppy)
(Reporter)

Comment 5

2 months ago
(In reply to Dragana Damjanovic [:dragana] from comment #4)
> Should onBeforeSendHeaders be sent for request that are served from cache?
> SetupTransaction is triggered only for requests going to the network.

Yeah, it probably should.

> http-on-modify-request is used by some addons to inspect a request and
> redirect it if it needs to be blocked, do we still have something like that
> in webextensions?

It is used, currently for three events in webextensions:

1. onBeforeRequest
2. onBeforeSendHeaders
3. onSendHeaders

The suggestion for http-on-before-connect-request would allow us to change the timing for the latter two.
Flags: needinfo?(mixedpuppy)
(Reporter)

Updated

2 months ago
Duplicate of this bug: 1367177
nsHttpChannel::Connect() is probably the right place to put an event for onBeforeSendHeaders. ( https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#377 )

SpeculativeConnect(); (line 438) start a connection speculatively.
cached is open in line 447.

Probably the base place to put it is around SpeculativeConnect.

Comment 8

a month ago
(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #1)
> That said, I believe http-on-modify-request is the last thing that really
> happens in Gecko before we start putting data on the wire, so I'm pretty
> sure it should match up to onBeforeSendHeaders.

For reference, Chrome documentation explicitly allows the connection to be established by the time onBeforeSendHeaders fires. So if setting up HSTS for example requires sending some data and only firing onBeforeSendHeaders then - that's completely in line with the spec. In fact, seeing whether HSTS applies is the use case I filed bug 1367177 about, current state of the Firefox implementation makes porting Enforce Encryption extension impossible.

(In reply to Dragana Damjanovic [:dragana] from comment #4)
> Should onBeforeSendHeaders be sent for request that are served from cache?

Yes, that's how it works in Chrome.
(Reporter)

Comment 9

a month ago
> (In reply to Dragana Damjanovic [:dragana] from comment #4)
> > Should onBeforeSendHeaders be sent for request that are served from cache?
> 
> Yes, that's how it works in Chrome.

I want to shoot for the best+latest moment at which a request can safely be modified and have good compatibility.  Things may not end up entirely compatible with chrome here because the internals may simply be different.
You need to log in before you can comment on or make changes to this bug.