Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#660 closed change (fixed)

Switch to version 2 of the HTTP cache API

Reported by: trev Assignee: saroyanm
Priority: P2 Milestone: Adblock-Plus-2.6.4-for-Firefox
Module: Adblock-Plus-for-Firefox Keywords:
Cc: greiner, honzab, manvel@… Blocked By:
Blocking: Platform: Firefox
Ready: yes Confidential: no
Tester: Verified working: no
Review URL(s):

http://codereview.adblockplus.org/6241284884791296/
http://codereview.adblockplus.org/4875867805188096/

Description (last modified by 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.

Change History (17)

comment:1 Changed 6 years ago by philll

  • Cc honzab added

comment:2 Changed 6 years ago by honzab

Some more info:

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/

Last edited 6 years ago by honzab (previous) (diff)

comment:3 Changed 6 years ago by trev

  • Description modified (diff)

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.

comment:4 Changed 6 years ago by 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

comment:5 Changed 5 years ago by saroyanm

  • Owner set to saroyanm

comment:6 Changed 5 years ago by saroyanm

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:7 follow-up: Changed 5 years ago by 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) {}

comment:8 Changed 5 years ago by 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.

comment:9 Changed 5 years ago by saroyanm

  • Cc manvel@… added
  • Platform set to Unknown
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:10 in reply to: ↑ 7 ; follow-up: Changed 5 years ago by 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.

Last edited 5 years ago by saroyanm (previous) (diff)

comment:11 in reply to: ↑ 10 Changed 5 years ago by 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.

comment:12 follow-up: Changed 5 years ago by trev

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

comment:13 in reply to: ↑ 12 Changed 5 years ago by 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.

comment:14 Changed 5 years ago by saroyanm

  • Review URL(s) modified (diff)
  • Status changed from reopened to reviewing

comment:15 Changed 5 years ago by saroyanm

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:16 Changed 5 years ago by trev

  • Milestone set to Adblock-Plus-for-Firefox-next
  • Platform changed from Unknown to Firefox/Firefox Mobile

comment:17 Changed 5 years ago by philll

  • Platform changed from Firefox/Firefox Mobile to Firefox

Made Firefox and Firefox mobile available as seperate platforms.

Note: See TracTickets for help on using tickets.