1982557 - Response of cached script is empty when dom.script_loader.navigation_cache=true

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P3)

Description

[Julian Descottes [:jdescottes]]

STRs:

• set dom.script_loader.navigation_cache to true
• open a page which uses cached scripts (immutable / 200 OK), eg https://www.wikipedia.org/
• open devtools / netmonitor (make sure cache is NOT disabled)
• reload the page to see a 200 OK cached script request
• open the Response tab in the netmonitor side panel

ER: Should see the script content
AR: "No response data available for this request"

Comment 1

[Julian Descottes [:jdescottes]]

There are pointers from :arai on how to retrieve the data for scripts at https://bugzilla.mozilla.org/show_bug.cgi?id=1981164#c3

Note that in general we don't need to bundle the script content in the observer notification, because the content is only retrieved lazily when we need to display the Response tab side panel.

Comment 2

[Julian Descottes [:jdescottes]]

Arai, is there a bug we can track to know when dom.script_loader.navigation_cache will be enabled?

Comment 3

[Tooru Fujisawa [:arai]]

We don't yet have a specific bug to flip the pref, but bug 1670403 is the meta bug for it.

Comment 4

[Julian Descottes [:jdescottes]]

Thanks, let's block it then.

Comment 5

[Alexandre Poirot [:ochameau]]

https://phabricator.services.mozilla.com/D272226 is about to disable a few assertions in https://searchfox.org/firefox-main/source/devtools/shared/commands/resource/tests/browser_resources_network_events_navigation.js
which should be fixed and re-enabled.

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: WIP: Bug 1982557 - WIP

Comment 7

[Tooru Fujisawa [:arai]]

Prepared a patch that let NetworkEventActor.getResponseContent to communicate with the SharedScriptCache.
(it's a squashed patch of 8 patches)

It passes a serialized ScriptHashKey to the http-on-resource-cache-response notification, to let the devtools side to query the cached script on demand.
I guess the channel passed to the notification itself also has subset of the equivalent information (and also if it's missing, we should populate), so the information that needs to be serialized into the key can be reduced, but so far it's working.

Also, while this patch solves the immediate access to the getResponseContent(), the "persist" setting doesn't seem to work.
The following calls are still behind the flag:

https://searchfox.org/firefox-main/rev/d5fa7178279c137900f4d0c46793e5ec1df3fb1b/devtools/shared/commands/resource/tests/browser_resources_network_events_navigation.js#183-187,190-194

// FIXME: bug 1982557
if (!isNavigationCacheEnabled) {
  const jsContent3 = await getResponseContent(jsRequest2);
  is(jsContent3, JS_CONTENT);
}
...
// FIXME: bug 1982557
if (!isNavigationCacheEnabled) {
  const iframeJsContent3 = await getResponseContent(iframeJsRequest2);
  is(iframeJsContent3, IFRAME_JS_CONTENT);
}

if I remove the condition, they fails with the following, so maybe that needs separate fix.

  FAIL devtools/shared/commands/resource/tests/browser_resources_network_events_navigation.js - Uncaught exception in test - {"from":"server0.conn0.watcher2.process11//netEvent93","error":"noSuchActor","message":"No such actor for ID: server0.conn0.watcher2.process11//netEvent93"}
  FAIL devtools/shared/commands/resource/tests/browser_resources_network_events_navigation.js - finished in 1579ms

Comment 8

[Tooru Fujisawa [:arai]]

The error for the persist case seems to be caused by the differences how the actors are managed, between the in-memory cache case vs regular case.
In the regular case, the jsRequest2 has ID=server0.conn0.netEvent14, and it works after the navigation.
In the in-memory cached case, the jsRequest2 has ID=server0.conn0.watcher2.process10//netEvent93, and the following line fails to find the pool.

https://searchfox.org/firefox-main/rev/cced10961b53e0d29e22e635404fec37728b2644/devtools/server/devtools-server-connection.js#172-173

getActor(actorID) {
  const pool = this.poolFor(actorID);

So, it's likely a restriction with the cached resources, and not something specific to script.
I'll keep them behind a pref branch.

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Bug 1982557 - Retrieve the script source from SharedScriptCache. r?jdescottes!

Comment 10

[Julian Descottes [:jdescottes]]

(In reply to Tooru Fujisawa [:arai] from comment #8)

The error for the persist case seems to be caused by the differences how the actors are managed, between the in-memory cache case vs regular case.
In the regular case, the jsRequest2 has ID=server0.conn0.netEvent14, and it works after the navigation.
In the in-memory cached case, the jsRequest2 has ID=server0.conn0.watcher2.process10//netEvent93, and the following line fails to find the pool.

https://searchfox.org/firefox-main/rev/cced10961b53e0d29e22e635404fec37728b2644/devtools/server/devtools-server-connection.js#172-173

getActor(actorID) {
  const pool = this.poolFor(actorID);

So, it's likely a restriction with the cached resources, and not something specific to script.
I'll keep them behind a pref branch.

Haven't looked at the patch yet, but I think in general it's acceptable if response content is no longer accessible after navigation. Otherwise we would have to preserve the response content of all cached responses in the parent processes by default, which would probably require a lot of IPC (and consequently a lot of overhead).

Comment 11

[Tooru Fujisawa [:arai]]

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

Haven't looked at the patch yet, but I think in general it's acceptable if response content is no longer accessible after navigation. Otherwise we would have to preserve the response content of all cached responses in the parent processes by default, which would probably require a lot of IPC (and consequently a lot of overhead).

Thanks.
Then the patch here should implement what we need.
(there may be small change in the API parameter tho)

Then it sounds like the bug 2017219's prototype for (b) is problematic in terms of IPC and overhead.
I'll look into option (a) there.

Comment 12

[Nicolas B. Pierron [:nbp] {away}]

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

Haven't looked at the patch yet, but I think in general it's acceptable if response content is no longer accessible after navigation. Otherwise we would have to preserve the response content of all cached responses in the parent processes by default, which would probably require a lot of IPC (and consequently a lot of overhead).

Wouldn't this be necessary in case of early eviction of the navigation cache, such as by an addon which would update the filtering policies?

Comment 13

[Julian Descottes [:jdescottes]]

(In reply to Nicolas B. Pierron [:nbp] from comment #12)

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

Haven't looked at the patch yet, but I think in general it's acceptable if response content is no longer accessible after navigation. Otherwise we would have to preserve the response content of all cached responses in the parent processes by default, which would probably require a lot of IPC (and consequently a lot of overhead).

Wouldn't this be necessary in case of early eviction of the navigation cache, such as by an addon which would update the filtering policies?

I'll let Arai answer whether this is the case or not for the current implementation, but from a user point of view, if you are actively purging your cache while using devtools and you can't access your cached content in devtools I feel like this is acceptable. But maybe one more reason to show an explicit status when we can't read from the cache.

Comment 14

[Tooru Fujisawa [:arai]]

If the requirement is to show the response content only if it's available, then not showing it when the cache is cleared would be reasonable.

In terms of addons, there are two cases:

• (a) webRequest.handlerBehaviorChanged explicitly cleared the cache
• (b) declarativeNetRequest API invalidate the cache

For (a), the SharedScriptCache no longer holds any cached content, and the query with ChromeUtils.getCachedJavaScriptSource after that returns undefined. Given that this can happen without explicit interaction (addons can automatically call those APIs), we'll need to show a message that the cached response body is no longer available.
For (b), the SharedScriptCache still holds the cache, with the dirty flag set. ChromeUtils.getCachedJavaScriptSource doesn't check the dirty flag (I should add this detail to the comment), and DevTools should still be able to show the response body

Similarly, if the user explicitly cleared the cache (from the Clear the cache dialog), SharedScriptCache no longer holds any cached content, and the query with ChromeUtils.getCachedJavaScriptSource after that returns undefined. But this is more explicit case than (a) above, so it would be more acceptable I think.

Also, the in-memory cache entries are cleared when the PC falls into the low-memory situation,
and I think this is an exceptional case, and freeing up the memory is more important.

Comment 15

[Julian Descottes [:jdescottes]]

For info we discussed a bit the new limitation (bodies for cached JS no longer accessible after navigation), and the overall feeling from the team is that we have a whole class of objects and data which get stale/unusable after navigation (eg objects logged in the console). Rather than having workarounds specific to each and every actor/object/data, a platform or framework approach would be preferred.

Long story short, we're ok with the limitations here to preserve the implementation simpler.

Going back to the cache clearing topic, I think we agree that showing a clear message is preferable. Having a cache key which leads to an undefined return value is a strong enough signal to display that message.

If the requirement is to show the response content only if it's available, then [...] it would be reasonable

We don't really have specs or clear requirements for devtools features in general, especially around the lifecycle of the objects. Ideally the data from the all captured network requests would be fully available forever, but again in that case we want to avoid too much overhead or having a workaround dedicated to only this use case, so the limitation is fine (we should file an enhancement to capture this though).

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/0b9bd1ae33db
https://hg.mozilla.org/integration/autoland/rev/18e9ce3b5f27
Retrieve the script source from SharedScriptCache. r=jdescottes,devtools-reviewers,bomsy

Comment 17

[agoloman]

https://hg.mozilla.org/mozilla-central/rev/18e9ce3b5f27

Comment 18

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/18e9ce3b5f27