1945419 - Integrate the script cache into the cdp events, or loosen the test expectation

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Tooru Fujisawa [:arai]]

CDP implementation is observing the following notification for the network activity, and testcases such as browser_requestWillBeSent.js fails if the script cache is used, given those notifications are handled by http channels

https://searchfox.org/mozilla-central/rev/b2693d317fbcf5ff17e438a2c2833fda0b05fdf4/remote/cdp/observers/NetworkObserver.sys.mjs#309-321

Services.obs.addObserver(this._onRequest, "http-on-modify-request");
Services.obs.addObserver(
  this._onExamineResponse,
  "http-on-examine-response"
);
Services.obs.addObserver(
  this._onCachedResponse,
  "http-on-examine-cached-response"
);
Services.obs.addObserver(
  this._onCachedResponse,
  "http-on-examine-merged-response"
);

https://searchfox.org/mozilla-central/rev/b2693d317fbcf5ff17e438a2c2833fda0b05fdf4/netwerk/protocol/http/nsHttpHandler.h#105-108,365-367,394-396,404-406,421-423

class nsHttpHandler final : public nsIHttpProtocolHandler,
                            public nsIObserver,
                            public nsSupportsWeakReference,
                            public nsISpeculativeConnect {
...
  void OnModifyRequest(nsIHttpChannel* chan) {
    NotifyObservers(chan, NS_HTTP_ON_MODIFY_REQUEST_TOPIC);
  }
...
  void OnExamineResponse(nsIHttpChannel* chan) {
    NotifyObservers(chan, NS_HTTP_ON_EXAMINE_RESPONSE_TOPIC);
  }
...
  void OnExamineMergedResponse(nsIHttpChannel* chan) {
    NotifyObservers(chan, NS_HTTP_ON_EXAMINE_MERGED_RESPONSE_TOPIC);
  }
...
  void OnExamineCachedResponse(nsIHttpChannel* chan) {
    NotifyObservers(chan, NS_HTTP_ON_EXAMINE_CACHED_RESPONSE_TOPIC);
  }

https://searchfox.org/mozilla-central/rev/b2693d317fbcf5ff17e438a2c2833fda0b05fdf4/netwerk/protocol/http/nsHttpHandler.h#421-423

void OnExamineCachedResponse(nsIHttpChannel* chan) {
  NotifyObservers(chan, NS_HTTP_ON_EXAMINE_CACHED_RESPONSE_TOPIC);
}

I guess we need notify "http-on-examine-cached-response" in the same way as "http-on-resource-cache-response",
but the problem here is that the CDP's implementation is observing them in different process than ScriptLoader works,
and iiuc, the observer notification doesn't work across processes.

https://searchfox.org/mozilla-central/rev/b2693d317fbcf5ff17e438a2c2833fda0b05fdf4/dom/script/ScriptLoader.cpp#1097-1098

obsService->NotifyObservers(channel, "http-on-resource-cache-response",
                            nullptr);

Comment 1

[Tooru Fujisawa [:arai]]

Tests failing with the combination of this bug and + script preload handling (bug 1902953 patch) are the following:

• browser_requestWillBeSent.js
• browser_responseReceived.js

Given they're not part of wpt, maybe the expectation could be too strict?
https://github.com/web-platform-tests/wpt/issues/48446 suggests some cached request doesn't reach cdp handling in Chrome.
It would be nice to check if this test's expectation is fulfilled in Chrome.

Comment 2

[Julian Descottes [:jdescottes]]

Given that CDP is scheduled for removal in Bug 1882096 (including the failing tests here), I think we can definitely skip / delete the tests early if it's blocking you.

In parallel we can add a wdspec test using BiDi to assess the cache behavior.

I guess we need notify "http-on-examine-cached-response" in the same way as "http-on-resource-cache-response",
but the problem here is that the CDP's implementation is observing them in different process than ScriptLoader works,
and iiuc, the observer notification doesn't work across processes.

We shouldn't try to modify the CDP network observer. The BiDi implementation takes care of this properly, we observe http-on-examine-cached-response in the parent process via the DevTools' NetworkObserver, and we have a custom listener to monitor http-on-resource-cache-response in content processes.

Also we shouldn't assume that the Chrome CDP codepath is similar to the Firefox CDP one. Firefox CDP codebase was a rushed effort to try to match Chrome, but we didn't reverse engineer Chrome CDP to get there, we just tried to pass the same tests.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Bug 1945419 - Clear the script cache before each test to avoid the effect of not-well-specfied cache behavior. r?jdescottes!

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/051e1be69440
Clear the script cache before each test to avoid the effect of not-well-specfied cache behavior. r=jdescottes,webdriver-reviewers

Comment 5

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/051e1be69440