1882803 - Emit network.responseCompleted and fetchError from onStopRequest

Status: RESOLVED FIXED

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

Description

[Julian Descottes [:jdescottes]]

Currently the network.responseCompleted and network.fetchError events are triggered when we receive addResponseContent from the devtools' NetworkObserver, which is only called after the response's input stream is closed and we retrieved the security information.

This introduces delays which means we emit the event later than if we were listening to onStopRequest. It can be accessed via webextension's ChannelWrapper in JS, using the "stop" event. We can also listen to the "error" event to be notified canceled requests.

However, we rely on the devtools' approach to provide an accurate decoded body size (used for event.response.content.size). The channel provides a decodedBodySize property, but it will invariably be 0 in the content process.

We could try to migrate our network code to use this, which should help ensuring we emit responseCompleted before the domContentLoaded and load events.

For the decoded body size, a proposed solution would be to add another observer notification in the content process, emitted before onStopRequest. We would send this notification to the parent process via IPC, and hopefully we should still retain a correct event order compared to domContentLoaded and load.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1882803 - [bidi] Emit network.responseCompleted and fetchError from onStopRequest

Comment 2

[Julian Descottes [:jdescottes]]

Try seems relatively green with the proposed change: https://treeherder.mozilla.org/jobs?repo=try&revision=05498094474e65cdb810b225b65ec272a73b50f2

Note that we never assert the various size properties of the response data (beyond checking they're integers). Any test checking that those fields are > 0 would fail because with this patch event.response.content.size will always be omitted.

Comment 3

[Julian Descottes [:jdescottes]]

Check if decoded body size is a hard requirement for HAR files and file bug to get a notification from the content process.

Comment 4

[Julian Descottes [:jdescottes]]

We do expose the decoded body size for HAR files, but browsertime doesn't seem to rely on it. Instead they get this information for the navigation request from the actual page using window.performance.getEntriesByType('navigation')[0].decodedBodySize.

Meaning we could probably regress and send null for that field temporarily.

Nevertheless, let's file a bug to get a notification about the decoded body size from the content process.

Comment 5

[Julian Descottes [:jdescottes]]

Test the patch from Bug 1884645 to check if we can get the decodedBodySize in time.

Comment 6

[Julian Descottes [:jdescottes]]

For the record, try push with a changeset to pipe the decodedBodySize from content to parent via a windowglobal module: https://treeherder.mozilla.org/jobs?repo=try&revision=536a59b2168e5596f98e3e995585ce6a67516c6c

Seems to work fine locally, but let's see if we spot issues on try.

Edit: Quite a few oranges, for redirects or errors mostly. My current patch is blindly waiting for the decodedBodySize before emitting responseCompleted, but there are probably several cases where the event will not be generated.

Comment 7

[Julian Descottes [:jdescottes]]

Attachment: Bug 1882803 - [puppeteer] Enable all puppeteer tests flaky due to late responseCompleted events

Depends on D203134

Comment 8

[Julian Descottes [:jdescottes]]

Attachment: Bug 1882803 - [puppeteer] Update puppeteer test to ignore cached favicon requests

Depends on D203134

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3f38d02d2706
[bidi] Emit network.responseCompleted and fetchError from onStopRequest r=webdriver-reviewers,devtools-reviewers,Sasha,bomsy
https://hg.mozilla.org/integration/autoland/rev/cdcac728979c
[puppeteer] Update puppeteer test to ignore cached favicon requests r=webdriver-reviewers,Sasha
https://hg.mozilla.org/integration/autoland/rev/15c25626a4fa
[puppeteer] Enable all puppeteer tests flaky due to late responseCompleted events r=webdriver-reviewers,Sasha

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3f38d02d2706
https://hg.mozilla.org/mozilla-central/rev/cdcac728979c
https://hg.mozilla.org/mozilla-central/rev/15c25626a4fa

Comment 11

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

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

Created attachment 9416201 [details]
Bug 1882803 - [puppeteer] Update puppeteer test to ignore cached favicon requests

Hi Julian. Are you going to create an upstream PR for these changes as well?

Comment 12

[Julian Descottes [:jdescottes]]

Already merged https://github.com/puppeteer/puppeteer/pull/12828

The test expectations will need to wait until 130 is in the release channel since puppeteer should stick to 129 and then release now.

Comment 13

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

Ah good point. Did you already create the other PR for test expectations as well so that we can reference it here?

Comment 14

[Julian Descottes [:jdescottes]]

I can create one, I feel like it's going to be very stale by the time we can merge it though.
Also thinking that this will also make the synchronisation harder for us, because the upstream test expectations will miss all the tests we enabled locally. It would be great if they still had a way to run against nightly upstream.

Comment 15

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

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

Also thinking that this will also make the synchronisation harder for us, because the upstream test expectations will miss all the tests we enabled locally. It would be great if they still had a way to run against nightly upstream.

Yeah we probably should discuss that. Detecting regressions as early as possible would be great. Otherwise we might ship those especially on platforms like MacOS and Windows that we do not test in our own CI as well. Maybe you could create a new issue upstream to discuss the strategy?