Closed Bug 1368527 Opened 8 years ago Closed 7 years ago

No compatible mapping for onBeforeSendHeaders and onSendHeaders

Categories

(WebExtensions :: Request Handling, enhancement, P3)

49 Branch
enhancement

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: triaged)

Attachments

(2 files, 3 obsolete files)

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.
Flags: needinfo?(bzbarsky)
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)
Priority: -- → P3
Whiteboard: triaged
See Also: → 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)
Blocks: 1353489
(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)
(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)
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.
(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.
> (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.
Comment on attachment 8891090 [details] Bug 1368527 add new on-before-connect notification to httpChannel Looking for feedback on this proposal for a new http-on-before-connect notification with better timing for webRequest.OnBeforeSendHeaders and webRequest.OnSendHeaders. I initially did this as part of SpeculativeConnect, but moved to SetupTransaction as that is happening later, after hsts and cors priming. I still have a patch that implements during SpeculativeConnect. For WebRequest, There is one difference in flow is introduced with this patch when an HSTS upgrade occurs. The redirection to https happens early and we get an event sequence that looks like this: OnBeforeRequest OnBeforeRedirect (HSTS upgrade) OnBeforeRequest OnBeforeSendHeaders ... The nice thing about this is that it is easy to identify the fact that the upgrade is happening. If an addon does not see the redirect, and subsequent https url in OnBeforeSendHeaders, it can react. The not-so-nice thing is that the flow for an hsts upgrade is now different than the documented webrequest flow[1]. This flow difference does not occur with the other version I did using SpeculativeConnect. [1]https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest
Attachment #8891090 - Flags: feedback?(kmaglione+bmo)
Attachment #8891090 - Flags: feedback?(dd.mozilla)
Assignee: nobody → mixedpuppy
Comment on attachment 8891090 [details] Bug 1368527 add new on-before-connect notification to httpChannel https://reviewboard.mozilla.org/r/162268/#review169802 This is the first pass throught, I will need to do a deeper review of this. There are some comments bellow, but a larger issue that needs to be resolved is what to do when data are fetched only from the cache. If data are fetch only from cache OnBeforeRequest will not be called. (Look for OpenCacheEntry call, after that we check whether to hit the network or to wait for the cache). It can also happen that we check the cache and hit the network at the same time. Sorry for a delay. ::: netwerk/protocol/http/nsHttpChannel.cpp:1083 (Diff revision 1) > > NS_ENSURE_TRUE(!mTransaction, NS_ERROR_ALREADY_INITIALIZED); > > nsresult rv; > > mUsedNetwork = 1; You will need to move this into the new SetupTrasaction. ::: netwerk/protocol/http/nsHttpChannel.cpp:1153 (Diff revision 1) > > mRequestHead.SetRequestURI(*requestURI); > > // set the request time for cache expiration calculations > mRequestTime = NowInSeconds(); > mRequestTimeInitialized = true; this probably should go to the new SetupTransaction too. ::: netwerk/protocol/http/nsHttpChannel.cpp:1250 (Diff revision 1) > + mTransactionObserver = nullptr; > > if (mPushedStream) { > mTransaction->SetPushedStream(mPushedStream); > mPushedStream = nullptr; > } I will ask Nick to confirme whether this is fine with push streams. ::: netwerk/protocol/http/nsHttpChannel.cpp:8251 (Diff revision 1) > } else { > LOG((" connection made non-restartable")); > mCaps &= ~NS_HTTP_CONNECTION_RESTARTABLE; > } > > // and create a new one... move comment down just before SetupTransaction. ::: netwerk/protocol/http/nsHttpChannel.cpp:8267 (Diff revision 1) > + > + if (mSuspendCount) { > + // We abandon the connection here if there was one. > + LOG(("Waiting until resume SetupTransaction [this=%p]\n", this)); > + MOZ_ASSERT(!mCallOnResume); > + mCallOnResume = &nsHttpChannel::HandleSetupTransaction; This is not going to work (probably actually it will, but the functionallyty will be different. HandleSetupTransaction does not set conn that is set here a couple of lines down.
(In reply to Dragana Damjanovic [:dragana] from comment #13) > Comment on attachment 8891090 [details] > Bug 1368527 add new on-before-connect notification to httpChannel > > https://reviewboard.mozilla.org/r/162268/#review169802 > > This is the first pass throught, I will need to do a deeper review of this. > There are some comments bellow, but a larger issue that needs to be resolved > is what to do when data are fetched only from the cache. > If data are fetch only from cache OnBeforeRequest will not be called. (Look > for OpenCacheEntry call, after that we check whether to hit the network or > to wait for the cache). It can also happen that we check the cache and hit > the network at the same time. OnBeforeRequest hasn't changed, it's still called from on-modify-request. Is this a new issue you're seeing?
Comment on attachment 8891090 [details] Bug 1368527 add new on-before-connect notification to httpChannel https://reviewboard.mozilla.org/r/162268/#review170326 ::: netwerk/protocol/http/nsHttpChannel.cpp:8267 (Diff revision 1) > + > + if (mSuspendCount) { > + // We abandon the connection here if there was one. > + LOG(("Waiting until resume SetupTransaction [this=%p]\n", this)); > + MOZ_ASSERT(!mCallOnResume); > + mCallOnResume = &nsHttpChannel::HandleSetupTransaction; Yeah, that is an item I looked at for a while. It will work, but a new connection is used, not really what we want. I figured a feedback loop would be good before figuring out what to do about this. Suggestions welcome.
(In reply to Shane Caraveo (:mixedpuppy) from comment #14) > (In reply to Dragana Damjanovic [:dragana] from comment #13) > > Comment on attachment 8891090 [details] > > Bug 1368527 add new on-before-connect notification to httpChannel > > > > https://reviewboard.mozilla.org/r/162268/#review169802 > > > > This is the first pass throught, I will need to do a deeper review of this. > > There are some comments bellow, but a larger issue that needs to be resolved > > is what to do when data are fetched only from the cache. > > If data are fetch only from cache OnBeforeRequest will not be called. (Look > > for OpenCacheEntry call, after that we check whether to hit the network or > > to wait for the cache). It can also happen that we check the cache and hit > > the network at the same time. > > OnBeforeRequest hasn't changed, it's still called from on-modify-request. > Is this a new issue you're seeing? I meant to write onBeforeSendHeaders, sorry.
(In reply to Shane Caraveo (:mixedpuppy) from comment #15) > Comment on attachment 8891090 [details] > Bug 1368527 add new on-before-connect notification to httpChannel > > https://reviewboard.mozilla.org/r/162268/#review170326 > > ::: netwerk/protocol/http/nsHttpChannel.cpp:8267 > (Diff revision 1) > > + > > + if (mSuspendCount) { > > + // We abandon the connection here if there was one. > > + LOG(("Waiting until resume SetupTransaction [this=%p]\n", this)); > > + MOZ_ASSERT(!mCallOnResume); > > + mCallOnResume = &nsHttpChannel::HandleSetupTransaction; > > Yeah, that is an item I looked at for a while. It will work, but a new > connection is used, not really what we want. I figured a feedback loop > would be good before figuring out what to do about this. Suggestions > welcome. Just to explain you why we need the old connection. We need it for authentication, e.g. ntlm. A server sends 401 or 407 and we respond on the same connection with some credentials.
Attached patch v2 beforeconnect version (obsolete) — Splinter Review
Given issues in the first patch (e.g. auth retry connection and cache issue), I thought I'd upload this one as well. It's a simpler patch, and probably more correct for event sequence but still different than mdn docs. This patch has a flow: onBeforeRequest onBeforeSendHeaders onSendHeaders onBeforeRedirect (if upgraded, different from docs) ...
Attachment #8895107 - Flags: feedback?(kmaglione+bmo)
Attachment #8895107 - Flags: feedback?(dd.mozilla)
(In reply to Shane Caraveo (:mixedpuppy) from comment #18) > Created attachment 8895107 [details] [diff] [review] > v2 beforeconnect version > > Given issues in the first patch (e.g. auth retry connection and cache > issue), I thought I'd upload this one as well. It's a simpler patch, and > probably more correct for event sequence but still different than mdn docs. > > This patch has a flow: > > onBeforeRequest > onBeforeSendHeaders > onSendHeaders > onBeforeRedirect (if upgraded, different from docs) > ... do we prefer the first version that is more similar to the docs? my question it: should we try to resolve issues in the first version?
(In reply to Dragana Damjanovic [:dragana] from comment #19) > (In reply to Shane Caraveo (:mixedpuppy) from comment #18) > > do we prefer the first version that is more similar to the docs? > > my question it: should we try to resolve issues in the first version? Sorry, probably some confusion here. The second version is slightly closer to the docs than the first (see below), but both introduce some variance. I think this is ok, likely unavoidable without a lot more work. The thing I like about the first patch is that upgrades are very obvious due to the out-of-order redirect. The second patch is also obvious, but you're unable to know for certain since OnSendHeaders is typically the last event prior to receiving a response (OnHeadersReceived). I like the second patch because it is simpler, and happens after cache open IIRC. I'm thinking about exposing some more of nsILoadInfo to webrequest, such as isHSTSPriming and isHSTSPrimingUpgrade. That would provide better indication to addons like httpseverywhere that an upgrade will/has occurred, and make the ordering here less an issue. Docs show: OnBeforeRequest OnBeforeSendHeaders OnSendHeaders OnHeadersReceived OnBeforeRedirect ... Patch1: OnBeforeRequest OnBeforeRedirect <- hsts upgrade OnBeforeRequest OnBeforeSendHeaders OnSendHeaders ... Patch2 OnBeforeRequest OnBeforeSendHeaders OnSendHeaders OnBeforeRedirect <- hsts upgrade ...
The second patch is a better solution. And you can decide if you want version 1(Called Patch1 in comment 20) or version 2(patch2). If you move you "Cut" just under NS_ShouldSecureUpgrade you will get: OnBeforeRequest OnBeforeRedirect <- hsts upgrade OnBeforeRequest OnBeforeSendHeaders OnSendHeaders My apologies that I have not read spec in details, but on a quick look it does not say much about hsts redirect. If we do not want to expose them we can always hide them, but we will still have 2 OnBeforeRequest. the patch looks good. I will take one more look on the finale patch.
Attachment #8895107 - Flags: feedback?(dd.mozilla) → feedback+
(In reply to Dragana Damjanovic [:dragana] from comment #21) > The second patch is a better solution. And you can decide if you want > version 1(Called Patch1 in comment 20) or version 2(patch2). I'll take door #2. > If you move you "Cut" just under NS_ShouldSecureUpgrade you will get: I'll look into that. > OnBeforeRequest > OnBeforeRedirect <- hsts upgrade > OnBeforeRequest > OnBeforeSendHeaders > OnSendHeaders > > > My apologies that I have not read spec in details, but on a quick look it > does not say much about hsts redirect. If we do not want to expose them we > can always hide them, but we will still have 2 OnBeforeRequest. It doesn't but I'm not too concerned with that, we're going beyond what google did here. > the patch looks good. I will take one more look on the finale patch. Thanks!
Attachment #8891090 - Attachment is obsolete: true
Attachment #8891090 - Flags: feedback?(kmaglione+bmo)
Attachment #8891090 - Flags: feedback?(dd.mozilla)
Attachment #8891091 - Attachment is obsolete: true
Comment on attachment 8895107 [details] [diff] [review] v2 beforeconnect version moved to reviewboard
Attachment #8895107 - Attachment is obsolete: true
Attachment #8895107 - Flags: feedback?(kmaglione+bmo)
Keywords: dev-doc-needed
Comment on attachment 8896427 [details] Bug 1368527 add new on-before-connect notification to httpChannel, Adding Kris for webrequest parts.
Attachment #8896427 - Flags: review?(kmaglione+bmo)
Comment on attachment 8896427 [details] Bug 1368527 add new on-before-connect notification to httpChannel, https://reviewboard.mozilla.org/r/167658/#review172988 r=me for the WebRequest parts. ::: netwerk/protocol/http/nsHttpChannel.cpp:501 (Diff revision 1) > + gHttpHandler->OnBeforeConnect(this); > + > + // Check if request was cancelled during http-on-before-connect. > + if (mCanceled) { > + return mStatus; > + } > + > + if (mSuspendCount) { > + // We abandon the connection here if there was one. > + LOG(("Waiting until resume OnBeforeConnect [this=%p]\n", this)); > + MOZ_ASSERT(!mCallOnResume); > + mCallOnResume = &nsHttpChannel::OnBeforeConnectContinue; > + return NS_OK; > + } > + > + return Connect(); > +} > + > +void > +nsHttpChannel::OnBeforeConnectContinue() > +{ > + NS_PRECONDITION(!mCallOnResume, "How did that happen?"); > + > + if (mSuspendCount) { > + LOG(("Waiting until resume OnBeforeConnect [this=%p]\n", this)); > + mCallOnResume = &nsHttpChannel::OnBeforeConnectContinue; > + return; > + } This is starting to become enough of a pattern that it probably warrants a helper of some sort... ::: netwerk/protocol/http/nsHttpChannel.cpp:531 (Diff revision 1) > + mCallOnResume = &nsHttpChannel::OnBeforeConnectContinue; > + return; > + } > + > + LOG(("nsHttpChannel::OnBeforeConnectContinue [this=%p]\n", this)); > + nsresult rv = Connect(); Nit: Please always declare `nsresult rv` separate from assigning to it. Same below. ::: toolkit/modules/addons/WebRequest.jsm:642 (Diff revision 1) > } > }, > > observe(subject, topic, data) { > let channel = subject.QueryInterface(Ci.nsIHttpChannel); > + let loadContext; Please add braces to the affected case blocks and declare this separately for each. Or, even better, just get rid of the `loadContext` variable altogether and inline the `getLoadContext` call, instead.
Attachment #8896427 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8896428 [details] Bug 1368527 test event timing using hsts, https://reviewboard.mozilla.org/r/167660/#review172990 ::: toolkit/components/extensions/test/mochitest/mochitest-common.ini:51 (Diff revision 2) > file_ext_test_api_injection.js > file_permission_xhr.html > file_teardown_test.js > return_headers.sjs > webrequest_worker.js > + hsts.sjs Please keep sorted... ish
Attachment #8896428 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8896427 [details] Bug 1368527 add new on-before-connect notification to httpChannel, https://reviewboard.mozilla.org/r/167658/#review174570 r+ for necko part
Attachment #8896427 - Flags: review?(dd.mozilla) → review+
Comment on attachment 8896428 [details] Bug 1368527 test event timing using hsts, https://reviewboard.mozilla.org/r/167660/#review174572 Sorry for the delay.
Attachment #8896428 - Flags: review?(dd.mozilla) → review+
Pushed by mixedpuppy@gmail.com: https://hg.mozilla.org/integration/autoland/rev/f2a208d9d4bc add new on-before-connect notification to httpChannel, r=dragana,kmag https://hg.mozilla.org/integration/autoland/rev/eddf184c4d74 test event timing using hsts,r=dragana,kmag
for dev-doc. https://developer.mozilla.org/en-US/docs/Observer_Notifications New notification "http-on-before-connect" happens after "http-on-modify-request", at the latest moment prior to making a connection. Additional information, request headers related to HSTS, etc. may be available at this time.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
For the WebExtension docs specifically, is the update just that in some circumstances (HSTS upgrade) the order of events is different in Firefox (as in comment 20's "patch 2")? So we need to update the diagram?
Flags: needinfo?(mixedpuppy)
The test[1] has the order, but here it is for simplicity. + ["onBeforeRequest", "onBeforeRedirect", "onBeforeRequest", + "onBeforeSendHeaders", "onSendHeaders", "onHeadersReceived", + "onResponseStarted", "onCompleted"]); I think that the diagram at on mdn[2] only illustrates a typical path, not all potential paths. I seem to recall (but haven't verified) that proxy authorization may also have a different flow sometimes. It would probably be fine to document that diagram as a "typical flow" and use hsts upgrade as an example where the flow would be different. So you could leave the diagram as is and just add some further explanatory notes. [1] https://hg.mozilla.org/mozilla-central/rev/eddf184c4d74#l3.100 [2] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest
Flags: needinfo?(mixedpuppy)
Thanks Shane. I've added a note about this after the diagram. Marking dev-doc-complete, but please let me know if you need anything else.
Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe verify-“ flag. Thanks!
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(mixedpuppy) → qe-verify-
Product: Toolkit → WebExtensions
Depends on: 1494033
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: