Switch to version 2 of the HTTP cache API

[trev]

Background

See ​http://www.janbambas.cz/http-cache-v1-api-disabled/, Mozilla is disabling version 1 of the HTTP cache API. We are currently using it in the blockable items list to determine whether we should show an image in the tooltip.

What to change

Switch to version 2 of the API, continue using version 1 as a fallback. Version 2 was enabled in ​https://bugzilla.mozilla.org/show_bug.cgi?id=913806, this one landed in Gecko 32. We will keep supporting older versions for a while still meaning that we need that fallback.

[honzab]

Some more info:

• The HTTP cache v2 API has been introduced in Firefox 27 (​https://bugzilla.mozilla.org/show_bug.cgi?id=913807)
• The HTTP cache v2 API has been enabled in Firefox 32 (​https://bugzilla.mozilla.org/show_bug.cgi?id=913806)
• The HTTP cache v1 API has been disabled in Firefox 32 as well (​https://bugzilla.mozilla.org/show_bug.cgi?id=999577)

There is currently no simple way to detect the new cache is running, except a kinda hacky way, see [1]. Note that cacheIOTarget of the cache service version 1 will not throw when cache1 is off. The code at [1] can (should) be used to decide which of the two APIs to use.

Watch my blog [2] for more info on this. Next step is to remove cache1 code and interfaces altogether.

[1] ​http://hg.mozilla.org/mozilla-central/annotate/18c21532848a/netwerk/test/unit/head_cache2.js#l6
[2] ​http://www.janbambas.cz/tag/mozilla/

[trev]

Thank you for the info. I was suspecting that HTTP cache v2 API wasn't usable immediately but I couldn't find the corresponding bug. So we can only drop the backwards compat code once we stop supporting anything below Gecko 32, good to know. Too bad that it has to happen in such a hacky way.

I guess we'll see your blog posts via Planet Mozilla, as before.

[honzab]

I didn't want to expose any nsICacheStorageService.cacheVersion attributes. The hack works well tho.

And yes, posts will be on planet. There is also a bug on removal of the old cache code: ​https://bugzilla.mozilla.org/show_bug.cgi?id=913828

[honzab]

Few nits on the patch (cannot use my google account to add inline comments in codereview):

+ if (Services.vc.compare(Utils.platformVersion, "32.0") >= 0)

Just a reminder this can be switched at runtime, you may rather want to use ​http://hg.mozilla.org/mozilla-central/annotate/18c21532848a/netwerk/test/unit/head_cache2.js#l6

+ cacheStorageSession = cacheService.diskCacheStorage(LoadContextInfo.default, false);

It would be nice if you could create the info from any nsILoadContext you may have available in the calling context, so that you at least respect the "private browsing" setting of the window

+ onCacheEntryAvailable: function(entry, accessGranted, status)

The signature is different for both the old and the new callback, if you don't use any other than the first argument (entry) just go with onCacheEntryAvailable: function(entry) {}

[honzab]

Re: Question: will this also work to look up cache entries only stored in memory (images with Cache-Control: no-store header)? This needs to be tested.

Yes, diskCacheStorage looks also to the memory only storage. Note that there is always just a single entry per URL regardless it's persistent or not.

[saroyanm]

Revision:
​https://hg.adblockplus.org/adblockplus/rev/2922c5b21c7d

[saroyanm]

Replying to honzab:
Sorry just noticed your replies.
Thanks for having look on patch.

Few nits on the patch (cannot use my google account to add inline comments in codereview):
+ if (Services.vc.compare(Utils.platformVersion, "32.0") >= 0)

Just a reminder this can be switched at runtime, you may rather want to use ​​http://hg.mozilla.org/mozilla-central/annotate/18c21532848a/netwerk/test/unit/head_cache2.js#l6

We actually decided to check for hardcoded version instead of the using that solution while the solution is looks to hacky and in future could cause issues, please check ​current discussion.

+ onCacheEntryAvailable: function(entry, accessGranted, status)
The signature is different for both the old and the new callback, if you don't use any other than the first argument (entry) just go with onCacheEntryAvailable: function(entry) {}

Already closed the issue, not sure if it make sense to reopen for this.

Re: Question: will this also work to look up cache entries only stored in memory (images with Cache-Control: no-store header)? This needs to be tested.
Yes, diskCacheStorage looks also to the memory only storage. Note that there is always just a single entry per URL regardless it's persistent or not.

That's good to know.

[honzab]

Replying to saroyanm:

Replying to honzab:
Sorry just noticed your replies.
Thanks for having look on patch.

Few nits on the patch (cannot use my google account to add inline comments in codereview):
+ if (Services.vc.compare(Utils.platformVersion, "32.0") >= 0)

Just a reminder this can be switched at runtime, you may rather want to use ​​http://hg.mozilla.org/mozilla-central/annotate/18c21532848a/netwerk/test/unit/head_cache2.js#l6

We actually decided to check for hardcoded version instead of the using that solution while the solution is looks to hacky and in future could cause issues, please check ​current discussion.

That's actually perfectly alright. I forgot how my own code works :) - you can use the new API since v27! It automatically forwards to the old service when the new cache is pref'ed off. I was advising wrong on this bit, sorry.

Re: Question: will this also work to look up cache entries only stored in memory (images with Cache-Control: no-store header)? This needs to be tested.

To look up also in the memory storage, according to the source code the diskCacheStorage second parameter looks like need to be set to true.

No, the second arg (to diskCacheStorage()) makes you look into the appcache - not memory cache. I strongly advice to leave it at false, mainly because you will cause large main thread IO and UI jankyness. It's all not what you want.

I'm only left concerned about the unfixed respect to private browsing flag, comment 7.

[trev]

Sorry, the Google-hosted Rietveld instance will only access Google accounts from our domain. We are working on setting up our own instance, then other people should be able to participate as well.

Reopening, it seems that we still have issues to address. Working with an undocumented API is really no fun.

No, the second arg (to diskCacheStorage()) makes you look into the appcache - not memory cache. I strongly advice to leave it at false, mainly because you will cause large main thread IO and UI jankyness. It's all not what you want.

I wasn't aware that it would have a performance impact, merely thought that we might miss cache entries without it. We should set the second diskCacheStorage() parameter back to false then.

It would be nice if you could create the info from any nsILoadContext you may have available in the calling context, so that you at least respect the "private browsing" setting of the window.

It seems that rather than LoadContextInfo.default we have to use:

LoadContextInfo.fromLoadContext(
   content.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
          .getInterface(Components.interfaces.nsIWebNavigation)
          .QueryInterface(Components.interfaces.nsILoadContext), false);

We also have to reset cacheStorage to null in handleLocationChange() - the cache storage is no longer independent of the selected tab.

+ if (Services.vc.compare(Utils.platformVersion, "32.0") >= 0)

Just realized that this check should result in false for the current Aurora builds - Utils.platformVersion is 32.0a2 which is considered lower than 32.0. We should compare against 32.0a1 here, to have Nightly and Aurora builds take this code path. I don't really want to use a lower version number here - even if the components fall back to the old cache implementation, they probably aren't stable in Firefox 27.

[honzab]

Replying to trev:

Sorry, the Google-hosted Rietveld instance will only access Google accounts from our domain. We are working on setting up our own instance, then other people should be able to participate as well.

Reopening, it seems that we still have issues to address. Working with an undocumented API is really no fun.

Undocumented? There are rich comments in idls as well as a documentation (tho, just in progress):
​https://developer.mozilla.org/en-US/docs/HTTP_Cache

No, the second arg (to diskCacheStorage()) makes you look into the appcache - not memory cache. I strongly advice to leave it at false, mainly because you will cause large main thread IO and UI jankyness. It's all not what you want.

I wasn't aware that it would have a performance impact, merely thought that we might miss cache entries without it. We should set the second diskCacheStorage() parameter back to false then.

It would be nice if you could create the info from any nsILoadContext you may have available in the calling context, so that you at least respect the "private browsing" setting of the window.

It seems that rather than LoadContextInfo.default we have to use:

LoadContextInfo.fromLoadContext(
   content.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
          .getInterface(Components.interfaces.nsIWebNavigation)
          .QueryInterface(Components.interfaces.nsILoadContext), false);

Exactly! That's it.

We also have to reset cacheStorage to null in handleLocationChange() - the cache storage is no longer independent of the selected tab.

Right. You should actually instantiate always a new one to be 100% safe, don't bother with performance, the storage class is very thin ; but up to you.

+ if (Services.vc.compare(Utils.platformVersion, "32.0") >= 0)

Just realized that this check should result in false for the current Aurora builds - Utils.platformVersion is 32.0a2 which is considered lower than 32.0. We should compare against 32.0a1 here, to have Nightly and Aurora builds take this code path. I don't really want to use a lower version number here - even if the components fall back to the old cache implementation, they probably aren't stable in Firefox 27.

They are pretty stable. The wrapping code has not changed since that time (no critical bug had to be fixed) and it was used for several months even on release channel with the old cache. You are safe here, actually safer since we stopped maintaining the old cache code long ago. However, up to you, 32 is going to work as well.

[saroyanm]

Revision:
​https://hg.adblockplus.org/adblockplus/rev/03478f74e611

[philll]

Made Firefox and Firefox mobile available as seperate platforms.