739868 - Implement mechanism to have different quota for "offline" and "pinned" apps

Status: RESOLVED WONTFIX

(Core :: Networking: Cache, enhancement)

Description

[Honza Bambas (:mayhemer)]

"Pinned" apps are more specified "offline" apps and have some privileges over them:

- cannot be evicted on 404 of the manifest load (bug 674728 handles that)
- should have larger if not even unlimited storage space quota (this bug should handle that)


I have two proposals for this bug, inclining more to the second one:

- have a new storage policy PINNED and a new instance of nsOfflineCacheDevice, after deeper look into the code it seems as an overkill (nsOfflineCacheDevice is a service)

- have just a new storage policy PINNED, that can be read from nsCacheEntry in nsOfflineCacheDevice::BindEntry() and saved using "Flags" column in moz_cache table to mark "pinned" entries ; this flag then will be used for calculations of CacheSize() in nsOfflineCacheDevice::OnDataSizeChange() to distinguish the limit


Michal, is it OK to create a new storage policy for this purpose?  What more, if any, needs to be done that is not obvious like adding the new const to all conditions checking now for STORE_OFFLINE?

Comment 1

[Thinker Li [:sinker]]

Attachment: Part 1: Different quota for offline and pinned

Comment 2

[Thinker Li [:sinker]]

Attachment: Part 2: Testcase for pinned apps

Comment 3

[Thinker Li [:sinker]]

I implement it with the 2nd proposal of honza.  I do a little trick on cache entry keys.  Encode the pinned flag as the last byte of keys.

Comment 4

[Honza Bambas (:mayhemer)]

Comment on attachment 613529 [details] [diff] [review]
Part 1: Different quota for offline and pinned

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

Please expose all flags on the nsDiskCacheDeviceSQL class as public constants and use them instead of numbers where possible.

I cannot say carrying the pinned info in client id is the best way here, but so far I haven't a better idea my self.


I want to wait for Michal's feedback here before I r+/- the patch, so giving just f+.

::: netwerk/cache/nsDiskCacheDeviceSQL.cpp
@@ +618,4 @@
>    : mDevice(device)
>    , mGroup(group)
>    , mClientID(clientID)
> +  , mPinned(aPinned)

You don't need this flag.  You pass it only to mDevice->ActivateCache but that method is not using it.

See all *** comments.

@@ +681,5 @@
>  {
>    NS_ENSURE_TRUE(mValid, NS_ERROR_NOT_AVAILABLE);
>    NS_ENSURE_TRUE(mDevice, NS_ERROR_NOT_AVAILABLE);
>  
> +  mDevice->ActivateCache(mGroup, mClientID, mPinned);

***

@@ +2100,5 @@
>  }
>  
>  NS_IMETHODIMP
>  nsOfflineCacheDevice::GetGroupsTimeOrdered(PRUint32 *count,
> +                                           char ***keys)

No white space only change please.

@@ +2170,3 @@
>                                    now / PR_USEC_PER_SEC,
> +                                  gNextTemporaryClientID++,
> +                                  (aPinned ? '1' : '0')));

The idea is not bad, but I'd rather have a pair of methods as GetPinnedFromClientID/SetPinnedOnClientID that do the job.  Best would be to add '&pin=N' and not just the last byte.

@@ +2173,5 @@
>  
>    nsCOMPtr<nsIApplicationCache> cache = new nsApplicationCache(this,
>                                                                 group,
> +                                                               clientID,
> +                                                               aPinned);

***

@@ +2212,5 @@
>      }
>  
> +    // '1' for last byte indicate a pinned app.
> +    bool pinned = clientID.Last() == '1';
> +    cache = new nsApplicationCache(this, group, clientID, pinned);

***

(BTW, if nsApplicationCache would need info about |pinned|, let it extract it from the clientID it self)

@@ +2378,5 @@
>  
>  nsresult
>  nsOfflineCacheDevice::ActivateCache(const nsCSubstring &group,
> +                                    const nsCSubstring &clientID,
> +                                    bool pinned)

*** Unused.

Comment 5

[Thinker Li [:sinker]]

Attachment: Part 1: Different quota for offline and pinned, v2

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 614268 [details] [diff] [review]
Part 1: Different quota for offline and pinned, v2

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

::: netwerk/cache/nsCacheService.cpp
@@ +519,5 @@
> +            mOfflineCacheCapacity = NS_MAX(0, cacheCapacity);
> +
> +            PRInt32 pinnedCapacity = 0;
> +            rv = branch->GetIntPref(OFFLINE_PINNED_CAPACITY_PREF,
> +                                    &pinnedCapacity);

You need a new branch !strcmp(OFFLINE_PINNED_CAPACITY_PREF).  Change to your new pref will not be reflected by this code.

@@ +922,5 @@
>  nsCacheProfilePrefObserver::OfflineCacheEnabled()
>  {
> +    if ((mOfflineCacheCapacity == 0) ||
> +        (mOfflinePinnedCapacity == 0) ||
> +        (!mOfflineCacheParentDirectory))

This is wrong.  When you set capacity for "offline cache" to some non-zero but for "pinned cache" to zero you disable even the offline cache it self.  There must be &&.

::: netwerk/cache/nsDiskCacheDeviceSQL.cpp
@@ +440,5 @@
>  CreateCacheEntry(nsOfflineCacheDevice *device,
>                   const nsCString *fullKey,
>                   const nsOfflineCacheRecord &rec)
>  {
> +  if (rec.flags & 0x1)

Where are the kXXXX for the flags?

@@ +1496,5 @@
>    const char *cid, *key;
>    if (!DecomposeCacheEntryKey(entry->Key(), &cid, &key, keyBuf))
>      return NS_ERROR_UNEXPECTED;
>  
> +  bool pinned = strstr(cid, "&pin=1") != NULL;

No, have a method for this operation taking const nsCSubstring &.  You can use nsDependentCString to convert char*.

@@ +1517,5 @@
>    rec.lastFetched = PRTimeFromSeconds(entry->LastFetched());
>    rec.lastModified = PRTimeFromSeconds(entry->LastModified());
>    rec.expirationTime = PRTimeFromSeconds(entry->ExpirationTime());
> +  if (pinned) {
> +    rec.flags |= 0x2;         // mark entry as a part of a pinned app.

kXXXX is where?

@@ +1672,5 @@
>    mDeltaCounter += deltaSize; // this may go negative
>  
>    if (mDeltaCounter >= DELTA_THRESHOLD)
>    {
> +    bool pinned = entry->Key()->Find("&pin=1") >= 0;

s/>= 0/!= kNotFound/

@@ +2159,3 @@
>                                    now / PR_USEC_PER_SEC,
> +                                  gNextTemporaryClientID++,
> +                                  (aPinned ? '1' : '0')));

hmm... I didn't realize the group name may contain "&pin=1" substring, it's a valid part of a URL.  It could be freely used for DoS attacks...

We need to find some different way to carry this information.

Michal, any updates on checking the idea of a new storage policy?

Comment 7

[Thinker Li [:sinker]]

(In reply to Honza Bambas (:mayhemer) from comment #6)
> @@ +2159,3 @@
> >                                    now / PR_USEC_PER_SEC,
> > +                                  gNextTemporaryClientID++,
> > +                                  (aPinned ? '1' : '0')));
> 
> hmm... I didn't realize the group name may contain "&pin=1" substring, it's
> a valid part of a URL.  It could be freely used for DoS attacks...

It is escaped by NS_Escape().  I don't think '&' is appear in output string of NS_Escape().

> 
> We need to find some different way to carry this information.
> 
> Michal, any updates on checking the idea of a new storage policy?

Comment 8

[Thinker Li [:sinker]]

Attachment: Part 1: Different quota for offline and pinned, v3

--
Attachment #614268 [details] [diff] - Attachment is obsolete: true

Comment 9

[Honza Bambas (:mayhemer)]

(In reply to Thinker Li [:sinker] from comment #7)
> (In reply to Honza Bambas (:mayhemer) from comment #6)
> > @@ +2159,3 @@
> > >                                    now / PR_USEC_PER_SEC,
> > > +                                  gNextTemporaryClientID++,
> > > +                                  (aPinned ? '1' : '0')));
> > 
> > hmm... I didn't realize the group name may contain "&pin=1" substring, it's
> > a valid part of a URL.  It could be freely used for DoS attacks...
> 
> It is escaped by NS_Escape().  I don't think '&' is appear in output string
> of NS_Escape().

Add it to your test please.

Comment 10

[Thinker Li [:sinker]]

Attachment: Part 2: Testcase for pinned apps, v2

--
Attachment #613530 [details] [diff] - Attachment is obsolete: true

Comment 11

[Honza Bambas (:mayhemer)]

Michal, can we get your opinion here?  I'd like to finish the review here.  Thanks.

Comment 12

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #0)
> Michal, is it OK to create a new storage policy for this purpose?  What
> more, if any, needs to be done that is not obvious like adding the new const
> to all conditions checking now for STORE_OFFLINE?

This is IMO similar to bug #722845, see comment 88. We should probably introduce some flags to the cache entries and/or cache sessions.

Comment 13

[Thinker Li [:sinker]]

Attachment: Part 1: Different quota for offline and pinned, v4

--
Attachment #614661 [details] [diff] - Attachment is obsolete: true

Comment 14

[Honza Bambas (:mayhemer)]

Thinker, please stop updating the patches here.  I'll comment soon on how we will proceed.  This approach seems not to be the proper way after some talk with Michal.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Honza, you're blocking this work.  Please help us move forward asap.  It's been a week since Michal commented.

Comment 16

[Honza Bambas (:mayhemer)]

I'll try to reply today.

Comment 17

[Honza Bambas (:mayhemer)]

This is not a blocker.  We have a way to pin apps (bug 674728).  This bug is trying to optimize the space usage and I wanted to have a separate quota for pinned apps to remove the crazy "remove one unpinned" code that may unexpectedly block the main thread.  This bug is an enhancement.

I needed some time to figure out whether a new storage policy was the way here.  Michal didn't see it as a proper way unless we actually put the data to a different storage.  However, using a different storage seems better to me as an identification here.  

Then there is an aspect what to do when we change the permission (remove it) later.  With this patch there is not a simple way to also switch the caching.  But I doubt it is a valid use case at all to remove the permission.

Then I don't think putting information about pinning to the client id is the right way either.  It just carries the info to the lower layer and is then persisted (actually redundantly) as a flag we then work with.  You must feel it's not right.


So, here is my though, but unchecked since you want an answer ASAP:
- separate the implementation of nsIApplicationCacheService to a different class (I've just proposed this as part of bug 746697 that is much more blocking that this one, so you probably don't need to do it, just wait for it to get fixed)
- create a new storage policy (now it makes sense)
- have two instances of nsOfflineCacheDevice with different quota and different directory to store to
- use storage policy (STORE_OFFLINE and new STORE_PINNED) to identify the device to store to
- search both devices when looking for a cache by key (ChooseApplicationCache)

Comment 18

[Honza Bambas (:mayhemer)]

FYI: patch in bug 746697 is almost done.

Michal, Chris, Thinker: do you want to discuss the suggestion from comment 17 or does it seems OK to you to start implementing it?

Comment 19

[Thinker Li [:sinker]]

Comment 17 seems fine for me.

Comment 20

[Michal Novotny [:michal]]

The approach in comment #17 looks good to me.

Comment 21

[Honza Bambas (:mayhemer)]

Alright, two positive voices now.

Thinker, willing to rewrite this ones again?

Comment 22

[Thinker Li [:sinker]]

I will do it.

Comment 23

[Honza Bambas (:mayhemer)]

Any progress here?

Comment 24

[Thinker Li [:sinker]]

I am working on some other issues.  I will back to this issue this week.  A patch will be worked out next week.

Comment 25

[Thinker Li [:sinker]]

Attachment: WIP

Comment 26

[Honza Bambas (:mayhemer)]

Comment on attachment 628682 [details] [diff] [review]
WIP

This seems to be on a good way.  Thanks.

Only bad news for you is that there soon will be patches landed that will make you marge this patch almost completely.  But no major change is expected.

Comment 27

[Thinker Li [:sinker]]

Attachment: Part 1: Quota and the directory for the pinned cache

Comment 28

[Thinker Li [:sinker]]

Attachment: Part 2: Add the pinned cache device

Comment 29

[Thinker Li [:sinker]]

Attachment: Part 3: Change relative interfaces for pinned

Comment 30

[Thinker Li [:sinker]]

Attachment: Part 4: Enable the pinned cache

Comment 31

[Thinker Li [:sinker]]

Attachment: Part 5: Update telemetry for pinned cache device

Comment 32

[Thinker Li [:sinker]]

Attachment: Part 6: Fix testcases for pinned cache

Comment 33

[Honza Bambas (:mayhemer)]

Comment on attachment 633810 [details] [diff] [review]
Part 1: Quota and the directory for the pinned cache

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

r-

::: netwerk/cache/nsCacheService.cpp
@@ +63,5 @@
>  #define OFFLINE_CACHE_DIR_PREF      "browser.cache.offline.parent_directory"
>  #define OFFLINE_CACHE_CAPACITY_PREF "browser.cache.offline.capacity"
>  #define OFFLINE_CACHE_CAPACITY      512000
> +#define PINNED_CACHE_CAPACITY_PREF \
> +    "browser.cache.pinned.capacity"

Please add a default value for this pref to all.js under modules/pref as well.

No need for \ new-line, you fit one line.

@@ +479,5 @@
> +            PRInt32 capacity = 0;
> +            rv = branch->GetIntPref(PINNED_CACHE_CAPACITY_PREF, &capacity);
> +            if (NS_FAILED(rv))  return rv;
> +            mPinnedCacheCapacity = NS_MAX(0, capacity);
> +            // nsCacheService::SetPinnedCacheCapacity(mPinnedCacheCapacity);

Remove this.  Just a proof of a not very happy patch parts split.

@@ +1601,5 @@
> +
> +inline nsresult
> +nsCacheService::InitDiskSQLCacheDir(nsOfflineCacheDevice *aDevice,
> +                                    nsIFile *aParentDir,
> +                                    const char *aSubDir) {

Remove these two new functions.  Just change signature of SetCacheParentDirectory to pass the storage policy to it, let that method decide on the name internally.

::: netwerk/cache/nsCacheService.h
@@ +157,5 @@
>      static void      SetOfflineCacheEnabled(bool    enabled);
>      // Sets the offline cache capacity (in kilobytes)
>      static void      SetOfflineCacheCapacity(PRInt32  capacity);
> +    // Sets the offline cache pinned capacity (in kilobytes)
> +    static void      SetOfflineCachePinnedCapacity(PRInt32  capacity);

Single space between PRInt32 and capacity.

::: netwerk/cache/nsDiskCacheDeviceSQL.cpp
@@ +2372,4 @@
>    if (NS_FAILED(rv))
>      return;
>  
>    mCacheDirectory = do_QueryInterface(dir);

and mBase directory?

::: netwerk/cache/nsDiskCacheDeviceSQL.h
@@ +168,5 @@
>    /**
>     * Preference accessors
>     */
>  
> +  void                    SetCacheDirectory(nsIFile * parentDir);

No.  Leave the name.  Pass also the storage policy.  Let the method decide on the name internally.

Comment 34

[Honza Bambas (:mayhemer)]

Comment on attachment 633811 [details] [diff] [review]
Part 2: Add the pinned cache device

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

r-

::: netwerk/cache/nsCacheService.cpp
@@ +1667,5 @@
> +        mObserver->OfflineCacheParentDirectory(),
> +        PINNED_CACHE_SUBDIR,
> +        mObserver->PinnedCacheCapacity(),
> +        &mPinnedDevice);
> +    NS_ENSURE_SUCCESS(rv, rv);

Factor this out to CreatePinned[Offline]Device().  I don't want to have an empty PinnedCache dir in every browser profile when not necessary.

Other option is to use some argument passed here (and all other methods you dup for pinned) that would tell what device to work with.  But that is up to you, we don't expect to have another offline device soon, so having separation for offline and pinned is probably OK here.

@@ +2140,5 @@
>  
> +            entry = mPinnedDevice->FindEntry(key, collision);
> +            if (!entry) {
> +                entry = mOfflineDevice->FindEntry(key, collision);
> +            }

Opposite way please.  First search mOfflineDevice.  But this is open to arguments.

@@ +2234,5 @@
> +            nsresult rv = mPinnedDevice->BindEntry(entry);
> +            entry->ClearBinding();
> +            if (NS_SUCCEEDED(rv))
> +                device = mPinnedDevice;
> +        }

No.  When preloading a pinned cache, binding fails with this code.

We have 3 cases here:
- entry->CustomCacheDevice()
- entry->IsAllowedOffline()
- entry->IsAllowedPinned()

The first is currently implemented only for offline entries (using offline device).  So, I would change the code the following way:

- ease the condition at [1] to:
  !device && entry->IsStreamData() && entry->CustomCacheDevice()
- in that branch, use just CustomCacheDevice() to bind 
- in !device && entry->IsStreamData() && entry->IsAllowedOffline() branch use mOfflineDevice to bind only
- add a new branch for condition:
  !device && entry->IsStreamData() && entry->IsAllowedPinned()
- in that branch use mPinnedDevice to bind


[1] http://hg.mozilla.org/mozilla-central/annotate/a84944a6727f/netwerk/cache/nsCacheService.cpp#l2113

@@ +2374,5 @@
>              // XXX delete mDiskDevice?
>          }
>      }
>  
> +    if (gService->mOfflineDevice && gService->mPinnedDevice) {

I'd rather see a duplicated code for mPinnedDevice

::: netwerk/cache/nsCacheService.h
@@ +158,5 @@
>      static void      SetOfflineCacheEnabled(bool    enabled);
>      // Sets the offline cache capacity (in kilobytes)
>      static void      SetOfflineCacheCapacity(PRInt32  capacity);
> +    // Sets the pinned cache capacity (in kilobytes)
> +    static void      SetPinnedCacheCapacity(PRInt32  capacity);

Only a white space change?

@@ +196,5 @@
>  
>      nsresult         CreateDiskDevice();
>      nsresult         CreateOfflineDevice();
>      nsresult         CreateCustomOfflineDevice(nsIFile *aProfileDir,
> +                                               const char *aSubDir,

No.  Pass only the storage policy.

::: netwerk/cache/nsDiskCacheDeviceSQL.h
@@ +169,5 @@
>    /**
>     * Preference accessors
>     */
>  
> +  void                    SetCacheDirectory(nsIFile * aDir);

Shouldn't be in part1?

::: netwerk/cache/nsICache.idl
@@ +119,5 @@
>      const nsCacheStoragePolicy STORE_ON_DISK         = 2;
>      const nsCacheStoragePolicy STORE_ON_DISK_AS_FILE = 3;
>      const nsCacheStoragePolicy STORE_OFFLINE         = 4;
> +    const nsCacheStoragePolicy STORE_PINNED          = 5;
> +    const nsCacheStoragePolicy STORE_CUSTOM          = 6;

I strongly disagree with adding STORE_CUSTOM.  I don't a see a single reason to have it.

By using STORE_OFFLINE you choose to store to or read from mOfflineCache device working in <host-process-profile>/OfflineCache.

By using STORE_PINNED you choose to store to or read from mPinnedCache device working in <host-process-profile>/PinnedCache.

To choose to write to <some-other-profile>/OfflineCache or /PinnedCache you just use a custom device directly.  See bug 753990.  This is used only for writing.  You cannot read a custom profile in the host application cache.  It has not been implemented, and this patch series is not about adding such functionality.

Comment 35

[Honza Bambas (:mayhemer)]

Comment on attachment 633812 [details] [diff] [review]
Part 3: Change relative interfaces for pinned

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

Not well separated patch either.

r-, pending also on decision on "nsICachingChannel API for offline application cache writing - getting messed a bit"

::: netwerk/base/public/nsIApplicationCacheService.idl
@@ +20,5 @@
>       * Create a new, empty application cache for the given cache
>       * group.
>       */
> +    nsIApplicationCache createApplicationCache(in long storagePolicy,
> +                                               in ACString group);

createCustomApplicationCache method apparently needs this change as well.

::: netwerk/base/public/nsICachingChannel.idl
@@ +79,2 @@
>       */
> +    attribute long storagePolicy;

Have you read my thread "nsICachingChannel API for offline application cache writing - getting messed a bit" ?

Maybe it ended in your spam folder (I had reports of my email being treated as spam).


BTW: this is absolutely bad name for this attribute.

::: netwerk/cache/nsDiskCacheDeviceSQL.h
@@ +269,5 @@
>    nsClassHashtable<nsCStringHashKey, nsCString> mActiveCachesByGroup;
>    nsTHashtable<nsCStringHashKey> mActiveCaches;
>  
>    nsCOMPtr<nsIThread> mInitThread;
> +  PRInt32 mStoragePolicy;

This is not just an interface change.  Please initialize this var in the constructor.  Horrible name for this member.  But I believe we will remove it.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +3950,5 @@
>              }
>  
>              // cacheClientID, cacheForOfflineUse
>              cachingChannel->SetOfflineCacheClientID(mOfflineCacheClientID);
> +            cachingChannel->SetStoragePolicy(nsICache::STORE_OFFLINE);

Really not a good change...  You need to pass the storage policy given to this channel, right?  If you want this patch feel complete, then implement SetStoragePolicy.

@@ -5354,4 @@
>  {
> -    ENSURE_CALLED_BEFORE_ASYNC_OPEN();
> -
> -    mCacheForOfflineUse = value;

Removing this is a complete breakage of the offline caching functionality.  Probably this patch just has not a good edge cut.

::: uriloader/prefetch/nsOfflineCacheUpdate.cpp
@@ +1259,5 @@
>                                            getter_AddRefs(mPreviousApplicationCache));
>          NS_ENSURE_SUCCESS(rv, rv);
>  
> +        rv = cacheService->CreateApplicationCache(nsICache::STORE_OFFLINE,
> +                                                  manifestSpec,

You should have the information to decide on the correct storage policy here, shouldn't you?

@@ +1313,5 @@
>          rv = GetCacheKey(mManifestURI, manifestSpec);
>          NS_ENSURE_SUCCESS(rv, rv);
>  
>          rv = cacheService->CreateApplicationCache
> +            (nsICache::STORE_OFFLINE, manifestSpec, getter_AddRefs(mApplicationCache));

As well here...

@@ +2036,1 @@
>      NS_ENSURE_SUCCESS(rv, rv);

Wow!  Isn't it worth to rather completely remove this whole method?  This way you are passing cound and groups uninitialized to EvictOneOfCacheGroups.

Comment 36

[Honza Bambas (:mayhemer)]

Comment on attachment 633813 [details] [diff] [review]
Part 4: Enable the pinned cache

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

r-

::: netwerk/cache/nsCacheEntry.h
@@ +179,5 @@
>  
> +    bool IsAllowedCustom()
> +    {
> +        return (StoragePolicy() == nsICache::STORE_CUSTOM);
> +    }

Remove this.

::: netwerk/cache/nsCacheRequest.h
@@ +143,5 @@
>          MutexAutoLock lock(mLock);
>          mCondVar.Notify();
>      }
>  
> +    nsIFile *ProfileDir() { return mProfileDir; }

You don't need this.  See other comments.

::: netwerk/cache/nsCacheService.cpp
@@ +1622,5 @@
>      if (!mCustomOfflineDevices.Get(profilePath, aDevice)) {
>          rv = CreateCustomOfflineDevice(aProfileDir,
>                                         OFFLINE_CACHE_SUBDIR,
>                                         aQuota,
> +                                       nsICache::STORE_CUSTOM,

For now leave custom device behave as for STORE_OFFLINE.  We need to enhance this API to let also custom devices (actually with just a custom profile dir) to be both OFFLINE or PINNED.  See other comments.

@@ -1631,5 @@
> -    nsresult rv = dir->Exists(&exists);
> -    if (NS_SUCCEEDED(rv) && !exists)
> -        rv = dir->Create(nsIFile::DIRECTORY_TYPE, 0700);
> -    return rv;
> -}

Actually moot now, but this should have been removed in part2 or 1, not sure anymore.

@@ +1677,5 @@
>  nsresult
>  nsCacheService::CreateCustomOfflineDevice(nsIFile *aProfileDir,
>                                            const char *aSubDir,
>                                            PRInt32 aQuota,
> +                                          PRInt32 aType,

s/aType/aStoragePolicy/ ?

@@ +1886,5 @@
>      if (NS_SUCCEEDED(rv) && request->mProfileDir) {
>          // Custom cache directory has been demanded.  Preset the cache device.
> +        nsCacheStoragePolicy policy = entry->StoragePolicy();
> +        if (policy != nsICache::STORE_CUSTOM) {
> +            // Failsafe check: this is implemented only for custom cache atm.

No, this is intended for both OFFLINE and PINNED.

@@ +2180,5 @@
> +            if(NS_FAILED(rv)) {
> +                return nsnull;
> +            }
> +            if (device) {
> +                entry = device->FindEntry(key, collision);

Remove this (STORE_CUSTOM).

@@ +2252,5 @@
> +                (void)CreateOfflineDevice();
> +            }
> +            device = mOfflineDevice;
> +        } else if (entry->IsAllowedCustom()) {
> +            device = entry->CustomCacheDevice();

In one of the previous parts (no idea which one) I suggested a different change to this code.

::: netwerk/cache/nsCacheService.h
@@ +231,5 @@
>      nsCacheDevice *  EnsureEntryHasDevice(nsCacheEntry * entry);
>  
> +    nsCacheEntry *   SearchCacheDevices(nsCString * key,
> +                                        nsCacheStoragePolicy policy,
> +                                        nsIFile *aProfileDir,

Absolutely not.  See other comments.  Custom dirs are not intended to be readable by the host process.  If we need that functionality, let's do that in a followup.  But for this bug it is not needed at all.

::: netwerk/cache/nsCacheSession.cpp
@@ +42,5 @@
>  
>  
>  NS_IMETHODIMP nsCacheSession::SetProfileDirectory(nsIFile *profileDir)
>  {
> +  if (StoragePolicy() != nsICache::STORE_CUSTOM && profileDir) {

Absolutely wrong!

How does the cache recognize what dir to store to?  You can have custom updates with both pinned and offline storage policy.  This is currently not implemented, but is intended to be.  Followup to this bug will be sufficient.  For now let custom updates be just offline.  

We will need to change nsIApplicationCacheService.createCustomApplicationCache to also take storage policy introduced in this bug.

::: netwerk/cache/nsDiskCacheDeviceSQL.cpp
@@ +636,5 @@
>  NS_IMETHODIMP
>  nsApplicationCache::GetCacheDirectory(nsIFile **out)
>  {
> +  if (mDevice->CacheDirectory())
> +      NS_ADDREF(*out = mDevice->CacheDirectory());

This breaks all the custom dir functionality.  Result of this method is expected to be the profile directory.

Actually I forgot to address Michal's review comment here to change name of this attribute to profileDirectory.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +189,5 @@
>          : mChannel(channel)
>          , mHasQueryString(HasQueryString(channel->mRequestHead.Method(),
>                                           channel->mURI))
>          , mLoadFlags(channel->mLoadFlags)
> +        , mStoragePolicy(channel->StoragePolicy())

Your StoragePolicy() on the http channel is for offline use.  I personally intend to remove it, btw.

Since we are opening offline cache entries for writing synchronously you don't need any changes to HttpCacheQuery.  At least at this moment.  

Remove all these changes.

@@ +2448,3 @@
>                                  cacheKey, nsICache::ACCESS_READ,
>                                  mLoadFlags & LOAD_BYPASS_LOCAL_CACHE_IF_BUSY,
> +                                usingSSL, appCachePolicy);

Pass appCachePolicy where the original code passes nsICache::STORE_OFFLINE, right?

This is opening of offline cache entry for reading, not for writing...

@@ +2563,5 @@
>                                  UsingPrivateBrowsing(), cacheKey,
>                                  accessRequested,
>                                  mLoadFlags & LOAD_BYPASS_LOCAL_CACHE_IF_BUSY,
> +                                usingSSL,
> +                                HttpCacheQuery::STORE_NONE);

Here you are not passing demanded storage policy at all! :)

@@ +2654,2 @@
>      rv = serv->CreateSession(mOfflineCacheClientID.get(),
> +                             mStoragePolicy,

This is correct!

@@ +2853,5 @@
>          if (NS_SUCCEEDED(rv)) {
>              rv = session->SetDoomEntriesIfExpired(false);
>          }
>          if (NS_SUCCEEDED(rv)) {
> +            rv = session->SetProfileDirectory(mProfileDirectory);

Custom profile is used only and exceptionally for writing, no need to bother with it in cache query.  Remove this code.

::: netwerk/protocol/http/nsHttpChannel.h
@@ +118,2 @@
>        { mUploadStream = uploadStream; }
> +    void SetUploadStreamHasHeaders(bool hasHeaders)

Remove these white space only changes.

@@ +154,5 @@
>          GetUsingPrivateBrowsing(&usingPB);
>          return usingPB;
>      }
>  
> +    PRInt32 StoragePolicy() { return mStoragePolicy; }

Name this StoragePolicyForOfflineCaching if you still need it in the next patch version.

@@ +341,5 @@
>      PRUint32                          mTransactionReplaced      : 1;
>      PRUint32                          mAuthRetryPending         : 1;
>      PRUint32                          mResuming                 : 1;
>      PRUint32                          mInitedCacheEntry         : 1;
> +    PRUint32                          mStoragePolicy            : 4;

Have a separate member for this.

@@ +372,5 @@
>      nsresult WaitForRedirectCallback();
>      void PushRedirectAsyncFunc(nsContinueRedirectionFunc func);
>      void PopRedirectAsyncFunc(nsContinueRedirectionFunc func);
>  
> +    bool IsCacheForOfflinePinned() {

Call this IsForOfflineCache()

::: uriloader/prefetch/nsOfflineCacheUpdate.cpp
@@ +355,5 @@
>      if (cachingChannel) {
> +        if (aUpdate->IsPinned()) {
> +            rv = cachingChannel->SetStoragePolicy(nsICache::STORE_PINNED);
> +        } else if (mCustomProfileDir) {
> +            rv = cachingChannel->SetStoragePolicy(nsICache::STORE_CUSTOM);

Why this change?

@@ +363,3 @@
>          NS_ENSURE_SUCCESS(rv, rv);
>  
> +        rv = cachingChannel->SetProfileDirectory(mCustomProfileDir);

I absolutely don't understand why you stop using mApplicationCache->GetCacheDirectory().

Probably because you broke it?

Comment 37

[Honza Bambas (:mayhemer)]

Comment on attachment 633814 [details] [diff] [review]
Part 5: Update telemetry for pinned cache device

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

r-

::: netwerk/cache/nsCacheService.cpp
@@ +1621,5 @@
>      NS_ENSURE_SUCCESS(rv, rv);
>  
>      if (!mCustomOfflineDevices.Get(profilePath, aDevice)) {
> +        static PRUint32 customSN = 0;
> +        nsCAutoString deviceID("custom-%x", (PRUint32)customSN++);

I'm strongly against, but I just repeat my self...

The device should have the same id in the host process as in the target process we store for.

::: netwerk/cache/nsDiskCacheDeviceSQL.cpp
@@ +1303,5 @@
>  
>  const char *
>  nsOfflineCacheDevice::GetDeviceID()
>  {
> +  return mDeviceID.get();

Generally it seems to be a valid idea to change id of the device.

But please just change the result according the storage policy to "pinned" or "offline".

This will need Michal's feedback, though.

Comment 38

[Honza Bambas (:mayhemer)]

Thinker, please next time rather provide a single patch for review, divided to two pieces tops if needed.  It is hard to track several patches that were actually not separate but mostly overwriting each other.

Also, I'm quit curious why you added the STORE_CUSTOM policy.  Before you add so much code that we haven't originally agreed on please consult with me first.

Comment 39

[Thinker Li [:sinker]]

(In reply to Honza Bambas (:mayhemer) from comment #38)
> Thinker, please next time rather provide a single patch for review, divided
> to two pieces tops if needed.  It is hard to track several patches that were
> actually not separate but mostly overwriting each other.

I just want to break into several logical parts.  Some parts may overwrite earlier patches, they are idea of leaving a skeleton to shut-up the complains from the compiler.  By the way, I will put everything together next time since it is more convenient for you.

> 
> Also, I'm quit curious why you added the STORE_CUSTOM policy.  Before you
> add so much code that we haven't originally agreed on please consult with me
> first.

I am very sorry for that, I suppose reviewing or WIP is the time for discussion.  Appealing, I am wrong.

I think most comments are about STORE_CUSTOM.
The reason of adding the STORE_CUSTOM policy is it is more clear than using profileDirectory to use storagePolicy as the condition to control the flow for offline and custom.  offline, custom, and pinned, all them require a profile directory.  To use storagePolicy as the condition for controlling control flow is appealingly not clear.
But, I don't insist it since you have a plan of seeking a better name for profileDirectory in bug 766402.

Comment 40

[Thinker Li [:sinker]]

Comment on attachment 633810 [details] [diff] [review]
Part 1: Quota and the directory for the pinned cache

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

::: netwerk/cache/nsDiskCacheDeviceSQL.cpp
@@ +2372,4 @@
>    if (NS_FAILED(rv))
>      return;
>  
>    mCacheDirectory = do_QueryInterface(dir);

mBaseDirectory is abandoned later.  Keep this here, keep this patch passes relative testcases.

::: netwerk/cache/nsDiskCacheDeviceSQL.h
@@ +168,5 @@
>    /**
>     * Preference accessors
>     */
>  
> +  void                    SetCacheDirectory(nsIFile * parentDir);

With this change, nsOfflineCacheDevice doesn't deal with what kind of storage policy it is any more.  Even sometime later, we need to differentiate nsOfflineCacheDevice for storage policies.  It would be better to create derivations of nsOfflineCacheDevice than adding conditional branch.  What do you think?

Comment 41

[Thinker Li [:sinker]]

Comment on attachment 633811 [details] [diff] [review]
Part 2: Add the pinned cache device

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

::: netwerk/cache/nsCacheService.cpp
@@ +2140,5 @@
>  
> +            entry = mPinnedDevice->FindEntry(key, collision);
> +            if (!entry) {
> +                entry = mOfflineDevice->FindEntry(key, collision);
> +            }

What I worried is what will be happened if an application is offline/or pinned at beginning and be promoted/or demoted to a pinned/or offline application.  I don't work on this potential issue for this bug.  My ideal solution is keeping only in exact one of offline and pinned cache device at any instant.  In this case, the order here make no different in logic.

::: netwerk/cache/nsICache.idl
@@ +119,5 @@
>      const nsCacheStoragePolicy STORE_ON_DISK         = 2;
>      const nsCacheStoragePolicy STORE_ON_DISK_AS_FILE = 3;
>      const nsCacheStoragePolicy STORE_OFFLINE         = 4;
> +    const nsCacheStoragePolicy STORE_PINNED          = 5;
> +    const nsCacheStoragePolicy STORE_CUSTOM          = 6;

Ok.. I think bug 753990 is the missing chain link.  So, custom device is only used for building a preloading cache.  It is never used for real user devices.  Right?!

Comment 42

[Honza Bambas (:mayhemer)]

(In reply to Thinker Li [:sinker] from comment #40)
> > +  void                    SetCacheDirectory(nsIFile * parentDir);
> 
> With this change, nsOfflineCacheDevice doesn't deal with what kind of
> storage policy it is any more.  Even sometime later, we need to
> differentiate nsOfflineCacheDevice for storage policies.  It would be better
> to create derivations of nsOfflineCacheDevice than adding conditional
> branch.  What do you think?

The directory path (OfflineCache or PinnedCache subdir) determination must be encapsulated in the device.  nsCacheService must not deal with it.  So passing the storage policy to the device is IMO the right way.  It is useful to let the device know what storage policy it deals with.


(In reply to Thinker Li [:sinker] from comment #41)
> Comment on attachment 633811 [details] [diff] [review]
> Part 2: Add the pinned cache device
> 
> Review of attachment 633811 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/cache/nsCacheService.cpp
> @@ +2140,5 @@
> >  
> > +            entry = mPinnedDevice->FindEntry(key, collision);
> > +            if (!entry) {
> > +                entry = mOfflineDevice->FindEntry(key, collision);
> > +            }
> 
> What I worried is what will be happened if an application is offline/or
> pinned at beginning and be promoted/or demoted to a pinned/or offline
> application.  I don't work on this potential issue for this bug.  My ideal
> solution is keeping only in exact one of offline and pinned cache device at
> any instant.  In this case, the order here make no different in logic.

Yeah.  And what if it is cached in both?  Personally I think this should not be changed at all (for OFFLINE storage policy) and a new branch for PINNED should be introduced.  That is my first thought.

> Ok.. I think bug 753990 is the missing chain link.  So, custom device is
> only used for building a preloading cache.  It is never used for real user
> devices.  Right?!

Exactly.  It is only and only intended to write to a cache of a different profile then the profile of the current (hosting) application, e.g. Firefox.

If we would want to also be able to read from these "foreign" offline caches, then it would have been implemented in bug 753990 or in a followup you would have known about (be CC'ed on), don't you think?

Comment 43

[Honza Bambas (:mayhemer)]

Actually the WIP was looking quit promising.  Next time write emails or put more WIPs as you like, I'll gladly answer or take a look.  It's better to have more questions then more work to do..

Comment 44

[Thinker Li [:sinker]]

Attachment: WIP

Comment 45

[Honza Bambas (:mayhemer)]

Comment on attachment 635244 [details] [diff] [review]
WIP

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

Some feedback, thanks for this WIP!

::: netwerk/base/public/nsICachingChannel.idl
@@ +80,2 @@
>       */
> +    attribute long storagePolicy;

You actually won't need to change this interface after bug 767096.

::: netwerk/cache/nsCacheRequest.h
@@ +143,5 @@
>          MutexAutoLock lock(mLock);
>          mCondVar.Notify();
>      }
>  
> +    nsIFile *ProfileDir() { return mProfileDir; }

Unused.

::: netwerk/cache/nsCacheService.cpp
@@ +1623,5 @@
> +        rv = CreateCustomOfflineDevice(aProfileDir,
> +                                       aQuota,
> +                                       nsICache::STORE_OFFLINE,
> +                                       NS_LITERAL_CSTRING("offline"),
> +                                       true, // custom

Please remove these new args, see other comments why.

::: netwerk/cache/nsDiskCacheDeviceSQL.cpp
@@ +471,5 @@
>  
>  NS_IMETHODIMP
>  nsOfflineCacheEntryInfo::GetDeviceID(char ** deviceID)
>  {
> +  *deviceID = NS_strdup(mDeviceID);

Really don't do this.  Base the result on mStoragePolicy.

@@ +1303,5 @@
>  
>  const char *
>  nsOfflineCacheDevice::GetDeviceID()
>  {
> +  return mDeviceID.get();

As well here.

@@ +1669,5 @@
>    nsRefPtr<nsOfflineCacheEntryInfo> info = new nsOfflineCacheEntryInfo;
>    if (!info)
>      return NS_ERROR_OUT_OF_MEMORY;
>    info->mRec = &rec;
> +  info->mDeviceID = mDeviceID.get();

Why do you need to set device id on the info?  (Maybe it's correct, I just don't see it).

@@ +2383,5 @@
> +  if (aCustom) {
> +    mBaseDirectory = parentDir;
> +  } else {
> +    mBaseDirectory = nsnull;
> +  }

Why is this needed at all?  Why shouldn't a regular device have a base directory?

::: netwerk/cache/nsDiskCacheDeviceSQL.h
@@ +58,5 @@
>  
>  };
>  
> +#define OFFLINE_CACHE_SUBDIR "OfflineCache"
> +#define PINNED_CACHE_SUBDIR "PinnedCache"

I think this will be sufficient to put to the cpp file.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +83,3 @@
>      default:
>          return "HTTP";
>      }

I thought this code had already been removed...  Never mind...

@@ +190,5 @@
>          : mChannel(channel)
>          , mHasQueryString(HasQueryString(channel->mRequestHead.Method(),
>                                           channel->mURI))
>          , mLoadFlags(channel->mLoadFlags)
> +        , mStoragePolicy(channel->StoragePolicy())

This is different storage policy!

@@ +199,5 @@
>          , mCacheKey(cacheKey)
>          , mAccessToRequest(accessToRequest)
>          , mNoWait(noWait)
>          , mUsingSSL(usingSSL)
> +        , mApplicationCachePolicy(applicationCachePolicy)

I believe after bug 767096 lands, you won't need any changes to this code.  You actually don't need them even now...

@@ +265,4 @@
>      const bool mFallbackChannel;
>      const InfallableCopyCString mClientID;
>      const nsCacheStoragePolicy mStoragePolicy;
> +    const nsCOMPtr<nsIFile> mProfileDirectory;

No need to pass this down...

@@ +313,5 @@
>      , mTransactionReplaced(false)
>      , mAuthRetryPending(false)
>      , mResuming(false)
>      , mInitedCacheEntry(false)
> +    , mStoragePolicy(nsICache::STORE_ANYWHERE)

Will change in bug 767096.

@@ +453,5 @@
>      }
>  
>      // if cacheForOfflineUse has been set, open up an offline cache
>      // entry to update
> +    if (IsCacheForOfflinePinned()) {

Has already changed in bug 767096, on many other places as well.

@@ +2563,5 @@
>                                  UsingPrivateBrowsing(), cacheKey,
>                                  accessRequested,
>                                  mLoadFlags & LOAD_BYPASS_LOCAL_CACHE_IF_BUSY,
> +                                usingSSL,
> +                                HttpCacheQuery::STORE_NONE);

And where are you passing |storagePolicy| arg?  It has a different value then your nsHttpChannel->StoragePolicy().

@@ +2836,5 @@
>      if (!NS_IsMainThread()) {
>          AssertOnCacheThread();
>  
> +        nsCacheStoragePolicy policy = IsLoadedFromApplicationCache() ?
> +            mApplicationCachePolicy : mStoragePolicy;

So you are storing something in two members and here just pick one of them based on some other members state?  Please don't do this.

@@ +2853,5 @@
>              rv = session->SetDoomEntriesIfExpired(false);
>          }
>          if (NS_SUCCEEDED(rv)) {
> +            rv = session->SetProfileDirectory(mProfileDirectory);
> +        }

No, this is absolutely misplaced.  This code is opening cache entry just for reading.  You don't need to set profile dir on the session since you never read from custom profiles.  You are going beyond scope of this bug.

This is done at nsHttpChannel::OpenOfflineCacheEntryForWriting

@@ +3491,5 @@
>          
>          if (!ShouldUpdateOfflineCacheEntry()) {
>              LOG(("Skipping read from cache based on LOAD_ONLY_IF_MODIFIED "
> +                 "load flag (STORE_OFFLINE and STORE_PINNED"
> +                 "cases)\n"));

Has changed in bug 767096.

::: netwerk/protocol/http/nsHttpChannel.h
@@ +341,5 @@
>      PRUint32                          mTransactionReplaced      : 1;
>      PRUint32                          mAuthRetryPending         : 1;
>      PRUint32                          mResuming                 : 1;
>      PRUint32                          mInitedCacheEntry         : 1;
> +    PRUint32                          mStoragePolicy            : 4;

Although this will be removed, just a side note for next time: this is very badly chosen name, it has to indicate it is related to offline cache only, so a name like mOfflineCacheWriteStoragePolicy is a more correct name for member like this.  Also this should be a separate member, not a bit field.

::: uriloader/prefetch/nsOfflineCacheUpdate.cpp
@@ +354,5 @@
> +        if (aUpdate->IsPinned()) {
> +            rv = cachingChannel->SetStoragePolicy(nsICache::STORE_PINNED);
> +        } else {
> +            rv = cachingChannel->SetStoragePolicy(nsICache::STORE_OFFLINE);
> +        }

Also changed (actually simplified) in bug 767096.

@@ +1276,1 @@
>                                                    getter_AddRefs(mApplicationCache));

This is it!  Only place you set the storage policy.

Maybe have a helper method for mPinned ? nsICache::STORE_PINNED : nsICache::STORE_OFFLINE.

::: uriloader/prefetch/nsOfflineCacheUpdate.h
@@ +212,5 @@
>      void SetOwner(nsOfflineCacheUpdateOwner *aOwner);
>  
>      virtual nsresult UpdateFinished(nsOfflineCacheUpdate *aUpdate);
>  
> +    bool IsPinned() { return mPinned; }

Yeah, instead have a helper returning the policy.

Comment 46

[Honza Bambas (:mayhemer)]

Comment on attachment 633816 [details] [diff] [review]
Part 6: Fix testcases for pinned cache

Just cleaning my review queue.

Comment 47

[Thinker Li [:sinker]]

Comment on attachment 635244 [details] [diff] [review]
WIP

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

::: netwerk/cache/nsDiskCacheDeviceSQL.cpp
@@ +471,5 @@
>  
>  NS_IMETHODIMP
>  nsOfflineCacheEntryInfo::GetDeviceID(char ** deviceID)
>  {
> +  *deviceID = NS_strdup(mDeviceID);

mStoragePolicy? But, it is an instance of nsOfflineCacheEntryInfo.

@@ +1669,5 @@
>    nsRefPtr<nsOfflineCacheEntryInfo> info = new nsOfflineCacheEntryInfo;
>    if (!info)
>      return NS_ERROR_OUT_OF_MEMORY;
>    info->mRec = &rec;
> +  info->mDeviceID = mDeviceID.get();

Check nsOfflineCacheEntryInfo::GetDeviceID()

@@ +2383,5 @@
> +  if (aCustom) {
> +    mBaseDirectory = parentDir;
> +  } else {
> +    mBaseDirectory = nsnull;
> +  }

nsIApplicationCache::cacheDirectory returns the value of mBaseDirectory of its backend device.  As description in the nsIApplicationCache.idl, this value; cacheDirectory, is only available for custom cache.  The value of cacheDirectory is used for setting profile directory of instances of nsHttpChannel as the indication of custom cache devices.  That is the line there.

Comment 48

[Thinker Li [:sinker]]

Attachment: WIP v2

Comment 49

[Honza Bambas (:mayhemer)]

Comment on attachment 635675 [details] [diff] [review]
WIP v2

Just putting on my radar.

Comment 50

[Honza Bambas (:mayhemer)]

Comment on attachment 635675 [details] [diff] [review]
WIP v2

Thinker, I think you should update the patch to apply over patch from bug 767096, it will land soon (today).

Comment 51

[Andreas Gal :gal]

How are we doing here?

Comment 52

[Thinker Li [:sinker]]

(In reply to Andreas Gal :gal from comment #51)
> How are we doing here?

It needs a rebase.  I will do it after bug #771420.

Comment 53

[Honza Bambas (:mayhemer)]

(In reply to Thinker Li [:sinker] from comment #52)
> (In reply to Andreas Gal :gal from comment #51)
> > How are we doing here?
> 
> It needs a rebase.  I will do it after bug #771420.

Why after and not before?  I thought this bug was a "high priority".

Comment 54

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't think we should hold the release for this, so removing as a blocker.