1879402 - Fix network event order for CORS preflight requests

Status: NEW

(Remote Protocol :: WebDriver BiDi, task, P3)

Description

[Julian Descottes [:jdescottes]]

Currently Firefox and Chrome do not emit network events in the same order for CORS preflight requests.
Firefox first emits events for the actual request and then for the preflight request, whereas Chrome does the opposite.

This might change in Firefox based on which event/observer notification we use in order to detect the network events, which should change in Bug 1849686.
For instance, using http-on-before-connect leads to the same order as Chrome. However using the new http-on-dispatching-transaction notification keeps the same order as the one we have today, so it's not clear if this bug will make us consistent with chrome.

We should also check from a spec perspective which order is expected.

Comment 1

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

Which network events are exactly affected here? Is it more than network.beforeRequestSent?

Comment 2

[Julian Descottes [:jdescottes]]

All events in theory.

Comment 3

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

This is not a missing P2 feature for Puppeteer anymore.

Comment 4

[Julian Descottes [:jdescottes]]

After reviewing the current fetch spec + fetch spec PR, the CORS preflight request should be fully completed before the beforeRequestSent event for the actual request.

So in total we should expect:

• preflight beforeRequestSent
• preflight responseStarted
• preflight responseCompleted
• actual request beforeRequestSent
• actual request responseStarted
• actual request responseCompleted

Spec wise, based on https://whatpr.org/fetch/1540.html#concept-http-network-or-cache-fetch the requests start at:

Main Fetch 4.1.12

Let corsWithPreflightResponse be the result of running HTTP fetch given fetchParams and true.

Then the CORS preflight happens in HTTP Fetch 4.3.4.1.1

Let preflightResponse be the result of running CORS-preflight fetch given request.

And then the actual request, if the CORS preflight response is not an error, occurs in 4.3.4.3

Set response and internalResponse to the result of running HTTP-network-or-cache fetch given fetchParams.

I think this should match the current behavior from Firefox since we switched to http-on-before-connect. Will verify and add tests.

Comment 5

[Julian Descottes [:jdescottes]]

Actually the summary was probably wrong. With early events we detect the actual request before the preflight request. So in practice we have the following order:

• actual request beforeRequestSent
• preflight beforeRequestSent
• preflight responseStarted
• actual request responseStarted
• preflight responseCompleted
• actual request responseCompleted

It is a bit weird that we get responseStarted for the actual request before responseCompleted for the preflight. It can either be because our implementation emits responseCompleted slightly too late. Or because we commit the actual request to the network before we have the full response for the preflight.

Comment 6

[Julian Descottes [:jdescottes]]

Adding another detail from reading the spec: if the preflight CORS request failed, we should not get any event for the actual request based on the current spec.

For the ordering issue, in theory when we switch to http-on-dispatching-transaction (Bug 1849686) we should emit the actual request's beforeRequestSent after the preflight request is done. Based on what we saw here, I imagine we will still dispatch preflight responseCompleted after we start dispatching events for the actual request.

Comment 7

[Julian Descottes [:jdescottes]]

The wrong order of the responseStarted events should be addressed by Bug 1882803. I revived my patches there and it fixes that part of the issue. However for the beforeRequestSent event order, we still need either Bug 1849686 or a way to reorder the events manually.

Comment 8

[Julian Descottes [:jdescottes]]

If we want to reorder events, we should be able to detect that a request will require a preflight based on https://searchfox.org/mozilla-central/rev/f7b41fc41c5505db507d076c16961a8da67dd318/netwerk/protocol/http/HttpBaseChannel.cpp#6023.

If we expose this on the nsIHttpChannel, we should then be able to wait for an OPTIONS request to the same URL before we emit the beforeRequestSent event for the triggering request. However we would still have to run the intercepts and block the request if necessary.

Comment 9

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

Julian, given that you removed the bug from M12 could you please give more details why we should no longer track it for the current milestone? Thanks.

Comment 10

[Julian Descottes [:jdescottes]]

I removed it so that we can discuss it during triage.
Adding the whiteboard flag so that we don't miss it during today's triage session

Comment 11

[Julian Descottes [:jdescottes]]

The summary here is that the spec is clear about the expected event order, for which I am adding a wdspec test at Bug 1907847.
However in order to support it in our implementation we have 2 issues to fix:

• emit beforeRequestSent for the preflight before the beforeRequestSent for the triggering channel. The observer notification we currently use does not follow this. Either we wait for Bug 1849686 or we reorder events manually (but to do so we need an additional flag)
• emit the response events for the actual request after the response events for the preflight. This is blocked on Bug 1882803.

For the first part, I am a bit hesitant to introduce yet another reordering workaround, and I would rather wait for the new notification. For the second part, we can try to move forward with Bug 1882803 (which is currently still failing existing tests on Android).

Whatever we chose I think most of this bug is currently blocked on other bugs, but I don't think it should be too high priority. One of the goals here was to figure out the expected event order, which is done and gives us the test proposed at Bug 1907847.

Comment 12

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

Julian, I assume this bug is no longer blocked?

Comment 13

[Julian Descottes [:jdescottes]]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #12)

Julian, I assume this bug is no longer blocked?

IMO still blocked on Bug 1849686. We could try workarounds to avoid it, but I don't think it's worth it. The added value is relatively small, and I think the maintenance cost would be too high.

So let's wait until we have time to get Bug 1849686 to land, but I wouldn't make this a priority. IMO it's a candidate to go back to the backlog at this point.

Comment 14

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

Yes, sounds good. Thanks for checking.``

Comment 15

[Julian Descottes [:jdescottes]]

On top of the already failing test we could also add a test for interception that checks that if we use provideResponse to set a CORS preflight response which disallows the pending CORS request (via the various Access-Control-Allow-* headers), the request is not issued.