1849686 - Add http-on-dispatching-transaction event

Status: ASSIGNED

(Core :: Networking, task, P2)

Description

[Julian Descottes [:jdescottes]]

In order to enable proper request blocking and interception for both DevTools and WebDriver BiDi, the NetworkObserver should create network events when catching the http-on-modify-request observer.

Right now we use a collection of various entry points to create the events, hopefully this observer can replace them all. However the challenge will be to make sure we have enough information at this point in time in order to populate the DevTools UI and to emit the WebDriver BiDi network.beforeRequestSent event.

See more details at Bug 1848156 comment 8.

Comment 1

[Julian Descottes [:jdescottes]]

A better phase to use is "http-on-before-connect"

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Julian, would you mind to add some more context about what's actually necessary here to get this bug fixed? I assume it's something that we can / should do in M10?

Comment 3

[Julian Descottes [:jdescottes]]

I assume it's something that we can / should do in M10?

Yes, I might try to start it next week but this will most likely slip to M10. It's blocking proper network interception in the beforeRequestSent phase.

Julian, would you mind to add some more context about what's actually necessary here to get this bug fixed?

See below.

---

To fix this bug, we need to modify the DevTools NetworkObserver to observe http-on-before-connect and create the network events at this point, which should synchronously emit the beforeRequestSent event for BiDi.

Thanks to this, if an intercept is set for beforeRequestSent, it will suspend the channel before the request was sent to the server. At the moment, even if you intercept requests on beforeRequestSent, the request still goes to the server, which is incorrect.

However, when moving the creation of the network request to http-on-before-connect, we are missing some information that we currently collect for our network events.

First raw headers: the raw headers size is expected as part of the network.beforeRequestSent payload (network.RequestData.headersSize). This is currently collected thanks to the activity observer, on ACTIVITY_SUBTYPE_REQUEST_HEADER, which is also where we create most of our network events. We can add a new addRawHeaders API on the network event owner interface, but this would still only be provided after beforeRequestSent is emitted so it's useless. Here we need a way to retrieve the raw headers when we receive http-on-before-connect

Then there are 2 other areas to investigate, but I don't remember if there really was an impact

• computing the fromCache/fromServiceWorker flags
• computing the blockedReason

For those 2, we were computing them at the same time as we were creating the events, but that's because the codepaths for such requests did not go through ACTIVITY_SUBTYPE_REQUEST_HEADER. We need to check if that's still true when we create the request in http-on-before-connect. I don't remember exactly but I think I spotted some requests where this was not true. Then we either need to be able to add the information to the network event after its creation (meaning: add new API to network event owner) or find a way to retrieve the information on http-on-before-connect. I think the blockedReason is only used in devtools, so it's not a big issue if we only have the information a bit later. For fromCache, we use it also in BiDi, but only in the Network.ResponseData, so it's also ok if we get the information later.

The following patch is a partial WIP: https://hg.mozilla.org/try/rev/b4691ee7c5d160861982cf741c374ce0a81c313b . It creates the events earlier, but I haven't tried to fix the devtools code in order to handle getting blockedReason/fromCache later.
Consequently it leads to several devtools test failures. It passes BiDi tests, but this is only because we never assert that headersSize is > 0 in any test.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thanks Julian! That's kinda helpful. As it looks like we seem to be not blocked on any missing network feature. Or should it be possible to enhance network to give raw headers for http-on-before-connect?

Nevertheless this sounds not trivial and there are risks for regressions. As such I would immediately move it to M10.

Comment 5

[Julian Descottes [:jdescottes]]

Yes we might be blocked on getting necko support to get the raw headers, or at least the raw headers size. But I agree there are risks of regressions. We could have this "early event" mode as optional to avoid devtools impact, but it will make maintenance harder in the long run.

Comment 6

[Julian Descottes [:jdescottes]]

Reviving the patch here, I'm also checking some issues we had back when I tested http-on-modify-request and which we discussed at https://bugzilla.mozilla.org/show_bug.cgi?id=1848156#c10

Even when using http-on-before-connect, there are still some headers which are missing compared to when we used the ACTIVITY_SUBTYPE_REQUEST_HEADER activity: Pragma and Cache-Control. I can see some code setting those headers when creating the transaction at https://searchfox.org/mozilla-central/rev/43c40b22c776d098afb89d5237da3464a7545532/netwerk/protocol/http/nsHttpChannel.cpp#1310-1334, but that seems related to reloads, whereas the test case where I spotted this is just doing an XHR.

For reference, we fetch the headers using channel.visitRequestHeaders.

Valentin, do you know if those headers might be added only when the transaction is actually created (and therefore would be missing with the observer approach)?

Comment 7

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

You're right, there are some headers we set in nsHttpChannel::SetupTransaction, which occurs after http-on-before-connect
Would adding yet another observer notification help?

Comment 8

[Julian Descottes [:jdescottes]]

(In reply to Valentin Gosu [:valentin] (he/him) from comment #7)

You're right, there are some headers we set in nsHttpChannel::SetupTransaction, which occurs after http-on-before-connect
Would adding yet another observer notification help?

Thanks for checking! Yes another observer notification would work, as long as it's early enough to cancel the request before it is sent to the server.
Before doing that let me check the other discrepancies I can find, in case there are other things we can fix.

Comment 9

[Julian Descottes [:jdescottes]]

I tried to add a notification during SetupTransaction, right after the headers are added. This fixes the issues I had with http-on-before-connect (missing headers, and order of preflight requests), but I'm not sure how to use CallOrWaitForResume in order to stop the request from being sent for now.

So the timing seems right to get the missing info but I still need to check if this is early enough in order to properly block the request.

Comment 10

[Julian Descottes [:jdescottes]]

Valentin, can you help me with adding this observer notification? So far, I didn't manage to use CallOrWaitForResume and split SetupTransaction in two in order to properly suspend the request, the browser crashes when a request is suspended. It might be the wrong approach or I'm missing some context on how to do it properly.

Also the last item that would be missing after that would be to have access to the raw headers, and especially the raw headers size. Do you think this is something we can expose?

Comment 11

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

Attachment: Bug 1849686 - Add http-on-dispatching-transaction event r=#necko

While the http-on-before-connect event is dispatched before
the DNS resolution had completed, it was apparent that the channel also
set multiple headers and flags during nsHttpChannel::SetupTransaction
when the transaction was actually dispatched. This meant that
any JS consumers that were looking to suspend or cancel the channel
based on various headers would not have the full set of headers available
when receiving the http-on-before-connect notification.

This patch adds a new http-on-dispatching-transaction notification that
is emitted just before the transaction is dispatched to the socket thread.

Comment 12

[Julian Descottes [:jdescottes]]

Still having an issue with service worker requests, will add details here.

Comment 13

[Julian Descottes [:jdescottes]]

I haven't had time to investigate in details, but with the new observer notification there are some issues with service worker requests, when trying to setup the response listener at https://searchfox.org/mozilla-central/rev/734887cfb193b5697e59a857d394e8d4245996db/devtools/shared/network-observer/NetworkObserver.sys.mjs#933-939

"Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsITraceableChannel.setNewListener]"

To check it out, you can pull the devtools changes from https://treeherder.mozilla.org/jobs?repo=try&revision=b88f276d64ad49f7a87de136572a3851f7e006b8 and run mach test devtools/client/netmonitor/test/browser_net_block-serviceworker.js.

Or open a page with a service worker performing requests (eg https://mdn.github.io/dom-examples/service-worker/simple-service-worker/) , open the netmonitor, reload and it should trigger the same error.

Comment 14

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

The only way setNewListener can return NS_ERROR_UNEXPECTED is from here:

https://searchfox.org/mozilla-central/rev/896042a1a71066254ceb5291f016ca3dbca21cb7/netwerk/protocol/http/HttpBaseChannel.cpp#4391

NS_ENSURE_STATE(mListener);

That means the function got called either too early or too late.
Since it's happening on SW tests, I assume the new devtools code is probably calling that for a channel that was already redirected to the intercepted channel, and now has a null listener?

I'll try to land attachment 9375960 [details] and we can iterate on the devtools code.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/32a53e4ded84
Add http-on-dispatching-transaction event r=necko-reviewers,kershaw

Comment 16

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/32a53e4ded84

Comment 17

[pstanciu]

Backed out as requested by Aryx for possibly causing Bug 1879917

• Backout link

Comment 18

[Julian Descottes [:jdescottes]]

Move to Necko and keep a separate bug for the DevTools/BiDi fixes.

Comment 19

[Julian Descottes [:jdescottes]]

Valentin, will you have time to investigate the backout? If not we should probably de-prioritize the bugs depending on this one for now.

Comment 20

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

I haven't managed to figure it out this week, but by all indications there's something wrong in my patch.
I'll do my best to fix and land it next week. Please hold me to that 🙂

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/74b319e23e72
Add http-on-dispatching-transaction event r=necko-reviewers,kershaw

Comment 22

[Cristina Horotan [:chorotan]]

Backed out changeset 74b319e23e72 (bug 1849686) for causing wpt failures at cross-origin-subresource-redirect-with-fp-delegation.https.html

Backout: https://hg.mozilla.org/integration/autoland/rev/badb2e5ed6981d0a36824afb5710907afd1d545e

Failure push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=74b319e23e7273cdfea8c10a05403188d09eb0ca

Failure log: https://treeherder.mozilla.org/logviewer?job_id=449967449&repo=autoland&lineNumber=2174

Comment 23

[J taghdir]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:valentin, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Hi Valentin, are you going to take another look at the patch and what caused the backout? From a quick look it seems that the bustage was triggered by another commit, which got backed out. So maybe we can just reland?

Comment 25

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

(In reply to Cristina Horotan [:chorotan] from comment #22)

Failure log: https://treeherder.mozilla.org/logviewer?job_id=449967449&repo=autoland&lineNumber=2174

Unfortunately there's another issue here:

MOZ_ASSERT(mFirstResponseSource != RESPONSE_FROM_CACHE) [@ mozilla::net::nsHttpChannel::ReadFromCache] | /client-hints/accept-ch-stickiness/cross-origin-subresource-redirect-with-fp-delegation.https.html

This is caused by a bad interaction with Race cache with network.
I'll have a look on monday if I can possibly improve things.

Comment 26

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

Attachment: Bug 1849686 - Refactor nsHttpChannel to add all headers before dispatching transaction r=#necko

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5377c5664962
Refactor nsHttpChannel to add all headers before dispatching transaction r=necko-reviewers,kershaw

Comment 28

[Aron Cseh]

https://hg.mozilla.org/mozilla-central/rev/5377c5664962

Comment 29

[Julian Descottes [:jdescottes]]

Hi Valentin!

I see that the refactor landed successfully on central, and you also updated the patch to add the notification. Do you think we can try landing this again?

Comment 30

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

Thank you for the try push with the failing test.
That helped me find the issue.
I'm not 100% sure if this fixes the problem that was causing bug 1879917, but I guess we'll see.

Comment 31

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

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

Comment 32

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

The latest try push triggered bug 1879917 again link.
I thought this could be related to race-cache-with-network, but I'm not sure why this patch specifically would trigger the bug.