1900461 - Read request cache status once in devtools NetworkObserver

Status: NEW

(DevTools :: Netmonitor, task)

Description

[Julian Descottes [:jdescottes]]

Follow up to https://phabricator.services.mozilla.com/D211939

The cache status of a request, stored as fromCache on the activity object of the network observer, is only based on the fact that request is detected in http-on-examine-cached-response. Later, we also fallback to channel.isFromCache(), but in our test suite there is no request where this turns out to be true for a request which had fromCache false (ie not detected in http-on-examine-cached-response).

On the other hand we have one request in a test where fromCache is true, but channel.isFromCache() is false.

We could either try to stop using isFromCache(), since it doesn't seem to add more information compared to our existing fromCache flag. Or we can try to see if isFromCache can be changed to match our needs. Ideally, we would stop relying on too much "devtools-only" state, and use platform APIs instead, so we would prefer the second solution if possible.

Comment 1

[Julian Descottes [:jdescottes]]

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

On the other hand we have one request in a test where fromCache is true, but channel.isFromCache() is false.

This request is a request to http://example.com/browser/devtools/client/netmonitor/test/sjs_status-codes-test-server.sjs?sts=redirect&cached in the test devtools/client/netmonitor/test/browser_net_cached-status.js. The request is handled in the sjs at devtools/client/netmonitor/test/sjs_status-codes-test-server.sjs. Long story short, it is a cached 301 Moved Permanently. The url it redirects to is a 404, in case it matters.

It would have been great to be able to rely on a platform API to know if a request is from the cache, instead of inferring this status based on where we detected the request.

But maybe our definition of being from the cache differs from the platform one? For this request, maybe channel.isFromCache is false because the actual content is not really read from the cache, it comes from the redirect (which itself is not necessarily cached?).

Valentin: do you know if for this specific use case channel.isFromCache() should actually return true? And more generally do you know if there are other cases where a channel will be notified via http-on-examine-cached-response, but isFromCache() would be false?

Comment 2

[Valentin Gosu [:valentin] (he/him)]

OnExamineCachedResponse is emitted from here:

https://searchfox.org/mozilla-central/rev/1d1ca77dd7ca10953c6f5f1b77b98e4eb528274e/netwerk/protocol/http/nsHttpChannel.cpp#991-1003

if (mCachedContentIsValid) {
  nsRunnableMethod<nsHttpChannel>* event = nullptr;
  nsresult rv;
  if (!LoadCachedContentIsPartial()) {
    rv = AsyncCall(&nsHttpChannel::AsyncOnExamineCachedResponse, &event);
    if (NS_FAILED(rv)) {
      LOG(("  AsyncCall failed (%08x)", static_cast<uint32_t>(rv)));
    }
  }
  rv = ReadFromCache();
  if (NS_FAILED(rv) && event) {
    event->Revoke();
  }

So it seems if ReadFromCache returns NS_OK we will send the notification.

There seem to be two cases that might happen incorrectly:

1. If we've already read from the cache. Not sure how common that can be.
   https://searchfox.org/mozilla-central/rev/1d1ca77dd7ca10953c6f5f1b77b98e4eb528274e/netwerk/protocol/http/nsHttpChannel.cpp#4770

NS_ENSURE_TRUE(!mCachePump, NS_OK);  // already opened

2. When this is a cached redirect
   https://searchfox.org/mozilla-central/rev/1d1ca77dd7ca10953c6f5f1b77b98e4eb528274e/netwerk/protocol/http/nsHttpChannel.cpp#4838-4844

if (WillRedirect(*mResponseHead)) {
  // TODO: Bug 759040 - We should call HandleAsyncRedirect directly here,
  // to avoid event dispatching latency.
  MOZ_ASSERT(!mCacheInputStream);
  LOG(("Skipping skip read of cached redirect entity\n"));
  return AsyncCall(&nsHttpChannel::HandleAsyncRedirect);
}

Here we'll return NS_OK when AsyncCall succeeds, but if I were to call nsHttpChannel::IsFromCache it would return false for the channel, because we never open the cache pump to read.
I think cached redirects are probably a case where IsFromCache should return true, as cached redirects don't have a body.