999577 - HTTP cache v2: disable addon access to cache v1 IDLs

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Jason Duell]

We need to keep the cache1 IDLs internally (the appcache still uses them), but we should turn them off as soon as it looks like cache2 is going to stick, since they will no longer actually access anything but appcache entries.  Better to break addons then have them "work" in a broken way.

Comment 1

[Honza Bambas (:mayhemer)]

Plan:
- let fail any attempt for sessions and eviction other then of STORE_OFFLINE policy
- make visitEntries throw (no control over what storage policy you want to visit)
- introduce nsICacheService.visitOfflineCache, and make it only visit the offline device (~7 places in the code base to fix)
- remove all netwerk/test/Test*.* working with the old cache

This can be easily controlled by the use_new_backend pref and I think would mostly be just enough to make this no longer work and catch some attention.

Comment 2

[Honza Bambas (:mayhemer)]

Attachment: wip1

Roughly tested patch.

https://tbpl.mozilla.org/?tree=Try&rev=856279926b28

Comment 3

[Honza Bambas (:mayhemer)]

Comment on attachment 8429616 [details] [diff] [review]
wip1

Michal, please check on this as well, Jason is not a deep cache1 expert - no offense :)

Comment 4

[Honza Bambas (:mayhemer)]

Attachment: wip1 -w [for simpler review]

Here you have a whitespace ignored patch for simpler overlook.

Comment 5

[Honza Bambas (:mayhemer)]

Michal, as discussed with Jason yesterday and outlined at https://etherpad.mozilla.org/Cache2Tasks I am working on a different patch.

Comment 6

[Honza Bambas (:mayhemer)]

Attachment: v2

- nsICacheService methods throw unconditionally when cache2=on
- internal methods on nsCacheService class introduced, cache2 calls on them directly
- rest of the browser/etc code touching appcache migrated to the new cache api (mostly those are just wrappers, so functionality is the same)

Don't worry, most of the patch are just removals :)  I believe the changes are functionally identical so that there is no need for a review from module peers we touch and reviews from both of you are OK.

https://tbpl.mozilla.org/?tree=Try&rev=bd66b2b457e8

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: v2 -w

Comment 8

[Jason Duell]

Comment on attachment 8434359 [details] [diff] [review]
v2

Review of attachment 8434359 [details] [diff] [review]:
-----------------------------------------------------------------

The parts I looked over seem good.  I don't know the API changes from old->new cache as well as Michal, so I'll leave the parts that switch those APIs to him to review.

::: browser/devtools/shared/AppCacheUtils.jsm
@@ +27,5 @@
>  
>  const { classes: Cc, interfaces: Ci, utils: Cu } = Components;
>  
>  let { XPCOMUtils } = Cu.import("resource://gre/modules/XPCOMUtils.jsm", {});
> +let { Services }   = Cu.import("resource://gre/modules/Services.jsm", {});LoadContextInfo

Is the "LoadContextInfo" bareword here a typo?  My JS sucks but this looks wrong.

::: netwerk/cache/nsICacheService.idl
@@ +24,5 @@
>  interface nsICacheService : nsISupports
>  {
>      /**
> +     * @throws NS_ERROR_NOT_IMPLEMENTED when the cache v2 is prefered to use
> +     *         and the storagePolicy is not STORE_OFFLINE.

Looks like you don't actually check + permit STORE_OFFLINE. Instead you're making code call *Internal functions.  Which is fine :) but maybe change comment to remove part about STORE_OFFLINE?

::: netwerk/cache2/OldWrappers.cpp
@@ +620,5 @@
>    nsCOMPtr<nsICacheSession> session;
> +  rv = nsCacheService::GlobalInstance()->CreateSessionInternal(clientId.get(),
> +                                                               storagePolicy,
> +                                                               nsICache::STREAM_BASED,
> +                                                               getter_AddRefs(session));

I didn't scan code past patch context, but maybe you don't need 'serv' variable any more now that you use GlobalInstance?

Comment 9

[Honza Bambas (:mayhemer)]

(In reply to Jason Duell (:jduell) from comment #8)
> Comment on attachment 8434359 [details] [diff] [review]
> v2

Thanks!

> 
> Is the "LoadContextInfo" bareword here a typo?  My JS sucks but this looks
> wrong.

Yep, that is wrong... Try seems to be quiet.

> but maybe change
> comment to remove part about STORE_OFFLINE?

Leftover from the first version.

> 
> ::: netwerk/cache2/OldWrappers.cpp
> @@ +620,5 @@
> >    nsCOMPtr<nsICacheSession> session;
> > +  rv = nsCacheService::GlobalInstance()->CreateSessionInternal(clientId.get(),
> > +                                                               storagePolicy,
> > +                                                               nsICache::STREAM_BASED,
> > +                                                               getter_AddRefs(session));
> 
> I didn't scan code past patch context, but maybe you don't need 'serv'
> variable any more now that you use GlobalInstance?

Oh, I do!  We need to protect GlobalInstance() from going away e.g. during some shutdown glitch and also make sure it actually exists.  This is the most simple and safest way to have GlobalInstance up and running in the block.

Comment 10

[Honza Bambas (:mayhemer)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d405928cb934

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Backed out for xpcshell failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0dbdb35eace

https://tbpl.mozilla.org/php/getParsedLog.php?id=41129773&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=41128719&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=41129570&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=41129856&tree=Mozilla-Inbound

Comment 12

[Honza Bambas (:mayhemer)]

Relanded as https://hg.mozilla.org/integration/mozilla-inbound/rev/c0323d9a7ea3 with dependency on bug 938186 removed.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c0323d9a7ea3