Closed Bug 999577 Opened 5 years ago Closed 5 years ago

HTTP cache v2: disable addon access to cache v1 IDLs

Categories

(Core :: Networking: Cache, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: jduell.mcbugs, Assigned: mayhemer)

References

(Blocks 1 open bug)

Details

(Keywords: addon-compat)

Attachments

(2 files, 2 obsolete files)

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.
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.
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Attached patch wip1 (obsolete) — Splinter Review
Roughly tested patch.

https://tbpl.mozilla.org/?tree=Try&rev=856279926b28
Attachment #8429616 - Flags: feedback?(jduell.mcbugs)
Comment on attachment 8429616 [details] [diff] [review]
wip1

Michal, please check on this as well, Jason is not a deep cache1 expert - no offense :)
Attachment #8429616 - Flags: feedback?(michal.novotny)
Attached patch wip1 -w [for simpler review] (obsolete) — Splinter Review
Here you have a whitespace ignored patch for simpler overlook.
Attachment #8430376 - Flags: review+
Michal, as discussed with Jason yesterday and outlined at https://etherpad.mozilla.org/Cache2Tasks I am working on a different patch.
Attachment #8429616 - Flags: feedback?(michal.novotny)
Attachment #8429616 - Flags: feedback?(jduell.mcbugs)
Attached patch v2Splinter Review
- 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
Attachment #8429616 - Attachment is obsolete: true
Attachment #8430376 - Attachment is obsolete: true
Attachment #8434359 - Flags: review?(michal.novotny)
Attachment #8434359 - Flags: review?(jduell.mcbugs)
Attached patch v2 -wSplinter Review
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?
Attachment #8434359 - Flags: review?(jduell.mcbugs) → review+
(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.
Attachment #8434359 - Flags: review?(michal.novotny) → review+
https://hg.mozilla.org/mozilla-central/rev/c0323d9a7ea3
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Depends on: 1021728
Depends on: mail-cache2
Keywords: addon-compat
Depends on: 1022133
Depends on: 1024296
You need to log in before you can comment on or make changes to this bug.