1946348 - Scripts cached by SharedScriptCache don't trigger the WebRequest.onBeforeRequest callbacks

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, task, P3)

Description

[Tooru Fujisawa [:arai]]

This issue happens on https://searchfox.org/mozilla-central/rev/94f8fc2185ca48c98020beeff129c4d89a776af9/browser/extensions/webcompat/tests/browser/browser_smartblockembeds.js if the preload handling is enabled by bug 1902953

The testcase uses the WebExtensions' WebRequest.onBeforeRequest listener to observe the load on scripts.

The onBeforeRequest seems to be implemented with "http-on-modify-request" observer notification.

https://searchfox.org/mozilla-central/rev/94f8fc2185ca48c98020beeff129c4d89a776af9/toolkit/components/extensions/webrequest/WebRequest.sys.mjs#676

Services.obs.addObserver(this, "http-on-modify-request");

https://searchfox.org/mozilla-central/rev/94f8fc2185ca48c98020beeff129c4d89a776af9/toolkit/components/extensions/webrequest/WebRequest.sys.mjs#756,759-760

observe(subject, topic, data) {
...
    case "http-on-modify-request":
      this.runChannelListener(channel, "onBeforeRequest");

As explained in bug 1945419, "http-on-modify-request" observer notification is implemented inside the network code.

Without the navigation cache, all script loads reach the network code, and onBeforeRequest callback is called for each request.

With the navigation cache, cached scripts don't reach the network code, and the onBeforeRequest callback is not called.

This is already the case for stylesheets, and also Chromium doesn't call the onBeforeRequest listener for cached scripts.

The questions would be:

• whether this is JS-specific issue, or the SharedSubResourceCache's issue shared across stylesheets (and maybe images)
• whether the onBeforeRequest call is required for cached scripts

Comment 1

[Tooru Fujisawa [:arai]]

The smart block's case needs some more fix around trackingProtection experimental API, which uses urlclassifier-before-block-channel notification.
I'll file another bug

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: WIP: Bug 1946348 - Clear shared sub resource cache when setting WebRequest listeners. r?#extension-reviewers!

In order to make the requests reach the network handling that triggers the
observer notifications used by WebRequest APIs, clear the in-memory cache for
stylesheets and scripts.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

I do think it's probably not worth dealing with this any differently than we're doing with stylesheets, but the trade-off of clearing the cache when setting the listener might be ok... Still probably can cause a bit of erratic / harder to explain behavior, in particular when multiple extensions interact, right?

Comment 4

[Tooru Fujisawa [:arai]]

Good point, I'm not sure what happens if multiple extensions tries to modify single requests. I'll experiment some.

Then, yes, this is a workaround only for the case where "a in-memory cache is created before the listeners are added", and this is only for the very next request.
Subsequent requests after that depends on what the server and extensions returns as the response for the "next request".
If the next request's response is cacheable (so, no extension made the response not-cacheable, or one extension explicitly made the response cacheable), then the subsequent requests will use the in-memory cache, and the WebRequest listeners won't be triggered.

Without this patch, if the extension wants to get the listener triggered after adding the listener, the thing that the extensions can do is either "clear the cache for given hostname (with browsingData.removeCache)", or "perform force reload (with tabs.reload with bypassCache option)", or "ask users to perform force reload", or something along that line, and if "clear the cache for given hostname" is too problematic for user experience, then this patch might workaround the issue. But yes, still there's no guarantee that the WebRequest listeners are always called for scripts.

Comment 5

[Tooru Fujisawa [:arai]]

Actually there's already an API for it, in bug 1657575.
And I see there's already some issues reported against stylesheets,
so addressing them at once (stylesheets, scripts, and images) by supporting the API and announcing the requirement would be fine.

I'll work on the bug.