1959621 - No http-on-before-stop-request received for requests switching protocols

Status: NEW

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

Description

[Julian Descottes [:jdescottes]]

This makes the playwright tests in page/page-network-idle.spec.ts timeout.

The test page creates a request to http://localhost:8907/ws which will be a 101 switching protocols. For this request, there is no http-on-before-stop-request notification received. This means we wait indefinitely for the decoded body size and never emit the network.responseCompleted event.

Kershaw: the http-on-before-stop-request notification was added at https://phabricator.services.mozilla.com/D204482 . Do you know if we should only expect this notification for some requests? At the moment we always expect to get it, but looks like it's not the case for 101 requests, maybe for others.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1931334

Comment 2

[Kershaw Chang [:kershaw]]

(In reply to Julian Descottes [:jdescottes] from comment #0)

This makes the playwright tests in page/page-network-idle.spec.ts timeout.

The test page creates a request to http://localhost:8907/ws which will be a 101 switching protocols. For this request, there is no http-on-before-stop-request notification received. This means we wait indefinitely for the decoded body size and never emit the network.responseCompleted event.

Kershaw: the http-on-before-stop-request notification was added at https://phabricator.services.mozilla.com/D204482 . Do you know if we should only expect this notification for some requests? At the moment we always expect to get it, but looks like it's not the case for 101 requests, maybe for others.

I think this is a bug. We should try to make this work for all requests.
Do you know how I can reproduce this locally? I’m unable to find a test named page-network-idle.spec.ts.

Comment 3

[Julian Descottes [:jdescottes]]

I'll try to build a separate test case, page-network-idle.spec.ts is a playwright test, so you'd need to pull https://github.com/microsoft/playwright/ and then run this specific test against their experimental BiDi implementation.

Comment 4

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

(In reply to Julian Descottes [:jdescottes] from comment #3)

I'll try to build a separate test case, page-network-idle.spec.ts is a playwright test, so you'd need to pull https://github.com/microsoft/playwright/ and then run this specific test against their experimental BiDi implementation.

And the steps to run this test can be found in the user story of bug 1917540. You can use .it as suffix of the test name to only run this particular test.

Comment 5

[Kershaw Chang [:kershaw]]

ni myself to put this in my to-do list.

Comment 6

[Julian Descottes [:jdescottes]]

While writing a test case for this, I think I understood the root cause of the issue. It seems like the websocket upgrade happens completely in the parent process.

For the WS upgrade I see observer notifications for "http-on-before-stop-request" in the parent process, but not in the content process which opens the websocket. And the issue is that we are exclusively listening to http-on-before-stop-request in content processes.

Why do we avoid listening to this in the parent: for regular requests handled in the content process, we get two "http-on-before-stop-request" notifications: one in the parent process and one in the content process. Only the content process one contains a valid decodedBodySize. So we don't listen to this observer notification at all in the parent process, because otherwise we might pick up the parent process notification, and read the wrong decodedBodySize on it.

I'm not sure how we could handle that. If we need to listen for the notification in all processes, we probably need a way to know if a given channel is also handled in a content process, so that we can ignore the parent process observer notification. Would that be possible?

Comment 7

[Dianna Smith [:diannaS]]

As a reminder that last beta uplifts are this thursday. If this will just ride the trains when completed, please set firefox138 tracking flag to wontfix.

Comment 8

[Kershaw Chang [:kershaw]]

For WebSocket loads, we have WebSocketChannelChild in the content process. Maybe we could use it to send a notification from the content process.
What data do you need from the WebSocket channel? If needed, we could try passing that information from the parent process.

Also, do you need this bug to be addressed in 138?
Thanks.

Comment 9

[Julian Descottes [:jdescottes]]

(In reply to Kershaw Chang [:kershaw] from comment #8)

For WebSocket loads, we have WebSocketChannelChild in the content process. Maybe we could use it to send a notification from the content process.
What data do you need from the WebSocket channel? If needed, we could try passing that information from the parent process.

We need the decodedBodySize. WebDriver BiDi monitors channels in the parent process by default (same as for DevTools). The decodedBodySize is the only information missing for WebDriver BiDi's network.responseCompleted event from the parent process for regular http channels. So we added some logic to wait for "http-on-before-stop-request" from the content process before emitting the event.

To workaround the issue, we no longer block on this notification to emit the event. If we already received when the request is stopped, we use it, otherwise we just emit 0. Since we already monitor the channel in the parent process, the easiest solution here might just be to fallback to the parent process channel to read decodedBodySize rather than just using 0. This would work fine for channels which are handled in the parent process. I think this would be enough to fix this issue?

Is there a way to know if the valid decodedBodySize of a given channel is in the parent process or in the content process?
With this information, before emitting network.responseCompleted I could decide if I can read this on the parent process channel or if I need to wait for the content process notification. The goal is to have a reliable logic to retrieve the valid decodedBodySize for any channel.

Also, do you need this bug to be addressed in 138?

Not necessary. We have a mitigation in place from Bug 1959614, so we can take time to figure out a proper solution.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1931334

Comment 11

[Kershaw Chang [:kershaw]]

(In reply to Julian Descottes [:jdescottes] from comment #9)

(In reply to Kershaw Chang [:kershaw] from comment #8)

For WebSocket loads, we have WebSocketChannelChild in the content process. Maybe we could use it to send a notification from the content process.
What data do you need from the WebSocket channel? If needed, we could try passing that information from the parent process.

We need the decodedBodySize. WebDriver BiDi monitors channels in the parent process by default (same as for DevTools). The decodedBodySize is the only information missing for WebDriver BiDi's network.responseCompleted event from the parent process for regular http channels. So we added some logic to wait for "http-on-before-stop-request" from the content process before emitting the event.

To workaround the issue, we no longer block on this notification to emit the event. If we already received when the request is stopped, we use it, otherwise we just emit 0. Since we already monitor the channel in the parent process, the easiest solution here might just be to fallback to the parent process channel to read decodedBodySize rather than just using 0. This would work fine for channels which are handled in the parent process. I think this would be enough to fix this issue?

Is there a way to know if the valid decodedBodySize of a given channel is in the parent process or in the content process?
With this information, before emitting network.responseCompleted I could decide if I can read this on the parent process channel or if I need to wait for the content process notification. The goal is to have a reliable logic to retrieve the valid decodedBodySize for any channel.

So, whether decodedBodySize is valid depends on where content conversion happens.
In most cases where loads are triggered in the content process, the conversion also happens there. This means decodedBodySize is only valid in the content process.
You can see this logic here. We use HasAppliedConversion to determine whether to notify HttpChannelChild to perform the conversion.

If needed, we could expose HasAppliedConversion to help you determine where the decoded body size is valid.
Would that be enough for your use case?

Comment 12

[Julian Descottes [:jdescottes]]

(In reply to Kershaw Chang [:kershaw] from comment #11)

If needed, we could expose HasAppliedConversion to help you determine where the decoded body size is valid.
Would that be enough for your use case?

Thanks, yes I think that would be perfect here.