Emit network.responseCompleted and fetchError from onStopRequest
Categories
(Remote Protocol :: WebDriver BiDi, task, P2)
Tracking
(firefox130 fixed)
Tracking | Status | |
---|---|---|
firefox130 | --- | fixed |
People
(Reporter: jdescottes, Assigned: jdescottes)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [webdriver:m12][webdriver:relnote])
Attachments
(3 files)
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
.
Assignee | ||
Comment 1•6 months ago
|
||
Updated•6 months ago
|
Assignee | ||
Comment 2•6 months ago
|
||
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.
Assignee | ||
Comment 3•6 months ago
|
||
Check if decoded body size is a hard requirement for HAR files and file bug to get a notification from the content process.
Assignee | ||
Comment 4•6 months ago
|
||
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.
Assignee | ||
Comment 5•6 months ago
|
||
Test the patch from Bug 1884645 to check if we can get the decodedBodySize in time.
Assignee | ||
Comment 6•6 months ago
•
|
||
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.
Assignee | ||
Updated•2 months ago
|
Assignee | ||
Updated•2 months ago
|
Assignee | ||
Comment 7•2 months ago
|
||
Depends on D203134
Assignee | ||
Comment 8•2 months ago
|
||
Depends on D203134
Pushed by jdescottes@mozilla.com: 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•2 months ago
|
||
bugherder |
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•1 month ago
|
||
(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?
Assignee | ||
Comment 12•1 month ago
|
||
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•1 month ago
|
||
Ah good point. Did you already create the other PR for test expectations as well so that we can reference it here?
Assignee | ||
Comment 14•1 month ago
|
||
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.
Assignee | ||
Updated•1 month ago
|
Comment 15•1 month ago
|
||
(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?
Updated•13 days ago
|
Description
•