442806 - Use separate, versioned caches for offline apps

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect, P1)

Description

[Dave Camp (:dcamp)]

Attachment: v1

The HTML5 spec calls for separate versioned caches rather than one flat offline cache.  It ties all the loads for a given toplevel browsing context to a given version of the application cache.

The attached patch implements the following to that end:
* nsIOfflineCacheSession has been removed and replaced with nsIApplicationCache.
* Channels pick up the nsIApplicationCache they're associated with from the load group (which in turn gets it from the docshell).
* Because the caches are separate, we were able to remove "ownerships" from the interface, which simplifies things a lot.
* nsDOMOfflineResourceList.cpp is updated to work with these application cache objects.

A few things are left for followups, which I'll file separately (this patch is getting big enough as it is):
* The UI doesn't pick up cache sizes correctly anymore
* The spec has a "cache selection algorithm" which verifies during parsing that the document was loaded from the right cache.  This patch just assumes we loaded from the right cache.
* Opportunistic caching/whitelisting aren't implemented in this patch.

Comment 1

[Dave Camp (:dcamp)]

One more - imgLoader.cpp doesn't pass on the load group appropriately, and its caching of requests causes some problems.  Will file a followup on that issue too.

Comment 2

[Dave Camp (:dcamp)]

Followups have been filed.  The ones that I think should block landing on mozilla-central are marked as blocking this bug, the one that I think can land separately/after (opportunistic caching/fallbacks) is dependent on this one.

Comment 3

[Dave Camp (:dcamp)]

Attachment: v1, with -p

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 327848 [details] [diff] [review]
v1, with -p

- In nsDocShell::GetInterface():

+        nsCOMPtr<nsIContentViewer> contentViewer;
+        root->GetContentViewer(getter_AddRefs(contentViewer));
+        if (!contentViewer)
+            return NS_ERROR_NO_INTERFACE;
+
+        nsCOMPtr<nsIDOMDocument> domDoc;
+        contentViewer->GetDOMDocument(getter_AddRefs(domDoc));

You could use do_GetInterface(domDoc) here to avoid the call to GetContentViewer() etc, save you some code...

r=jst, but I did not look at the network code in great detail, so others should pay attention there.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Dave, which files in this patch should I particularly pay attention to?

Comment 7

[Dave Camp (:dcamp)]

I believe jst looked at everything but the files in netwerk/

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 327848 [details] [diff] [review]
v1, with -p

>+++ b/browser/base/content/browser.js	Wed Jul 02 12:49:49 2008 -0700
>+    // XXX: include offline cache usage.

Please make this a FIXME comment with the bug# for it.

>+++ b/browser/components/preferences/advanced.js	Wed Jul 02 12:49:49 2008 -0700
>+    // XXX: include offline cache usage.

Same.

>+++ b/content/base/src/nsDocument.cpp	Wed Jul 02 12:49:49 2008 -0700
>+nsDocument::GetApplicationCache(nsIApplicationCache **aApplicationCache)
>+  NS_IF_ADDREF(*aApplicationCache = mApplicationCache.get());

No need for the .get(), I would think.

>+++ b/content/base/src/nsDocument.h	Wed Jul 02 12:49:49 2008 -0700
>+#include "nsIApplicationCache.h"

If you include that here, why include it in nsDocument.cpp?

>+  // nsIApplicationCacheContainer
>+  NS_DECL_NSIAPPLICATIONCACHECONTAINER

No need for the comment, since it's pretty clear what interface this is.

>+  // The application cache that this document is associated
>+  // with.

"if any". Or some other way of indicating this may be null.

Also, document that this can be changed at various points?

>+++ b/docshell/base/nsDocShell.cpp	Wed Jul 02 12:49:49 2008 -0700

>+    else if (aIID.Equals(NS_GET_IID(nsIApplicationCacheContainer)) &&
>+             NS_SUCCEEDED(EnsureContentViewer())) {
>+        // Return the toplevel document as an
>+        // nsIApplicationCacheContainer.

Hmm.  So if site A puts site B in a subframe.  And site B uses offline stuff.  Will this make site B hit site A's cache?  Is that desirable?  Is there a security review in the offing?  I realize this is what the spec says to do, by the way.  Just checking that it's secure and doesn't allow cross-site injection or anything like that.

>+        nsCOMPtr<nsIDocShellTreeItem> rootItem;
>+        GetSameTypeRootTreeItem(getter_AddRefs(rootItem));
>+        nsCOMPtr<nsIDocShell> root = do_QueryInterface(rootItem);
>+        if (!root)
>+            return NS_ERROR_NO_INTERFACE;

That will never happen, so I think it's safe to just assert that root is non-null.

>+        root->GetContentViewer(getter_AddRefs(contentViewer));
>+        if (!contentViewer)
>+            return NS_ERROR_NO_INTERFACE;

Again, I'm not sure this should be happening if the docshell tree is sane.  I can see null-checking this to be safe, but please assert that contentViewer is non-null.

>+        contentViewer->GetDOMDocument(getter_AddRefs(domDoc));
>+        if (!domDoc)
>+            return NS_ERROR_NO_INTERFACE;

Same for domDoc.

>@@ -7284,16 +7311,35 @@ nsDocShell::DoURILoad(nsIURI * aURI,
>+        if (root.get() == static_cast<nsIDocShellTreeItem *>(this)) {

How about SameCOMIdentity(root, this) instead?  Or just |root == this|, honestly.

>+                permissionManager->TestExactPermission(aURI, "offline-app",
>+                                                       &perm);

Hmm.  So we seem to do this test in a number of places, not all the same way.  For example, nsContentUtils::OfflineAppAllowed does something quite different from what you do here.  This test does match the one in GetQuota in nsDOMStorage.cpp, but I would think we'd want to use the same exact test in all three places.  The GetQuota knows it has an http:// URI, but your don't here.  In particular, you need to look at the innermost URI of aURI, and probably at the "offline-apps.allow_by_default" pref as needed.  And likely do the http/https thing that nsContentUtils does, too.

>+                    loadFlags |= nsICachingChannel::LOAD_CHECK_OFFLINE_CACHE;

So... I just looked through, and on redirect we just send this flag along, even if redirecting to a domain that doesn't have that permission.  That seems to be an existing problem, but we should still fix it.  Followup bug is probably OK.

>+++ b/dom/src/offline/nsDOMOfflineResourceList.cpp	Wed Jul 02 12:49:49 2008 -0700
>+static nsCAutoString gCachedManifestSpec;

So... generally this is bad: it'll show up as a leak any time the string value is longer than 80 chars or whatever we use now.  Followup bug to not us a static object here?

>@@ -402,23 +403,25 @@ nsDOMOfflineResourceList::GetStatus(PRUi
>     *aStatus = nsIDOMOfflineResourceList::IDLE;
>   } else {
>     *aStatus = nsIDOMOfflineResourceList::UNCACHED;

Is there a separate bug on CHECKING, DOWNLOADING, etc?

>@@ -449,17 +452,44 @@ nsDOMOfflineResourceList::SwapCache()
>+  rv = GetDocumentAppCache(getter_AddRefs(currentAppCache));

That doesn't look right.  In particular, here's what the spec says here (from <http://www.whatwg.org/specs/web-apps/current-work/multipage/offline.html#swapcache>):

  2.  Check that document is associated with an application cache.
      If it is not, then raise an INVALID_STATE_ERR exception and
      abort these steps.

        Note: This is not the same thing as the ApplicationCache
	object being itself associated with an application cache!
	In particular, the Document with which the ApplicationCache
	object is associated can only itself be associated with
	an application cache if it is in a top-level browsing context.

In other words, we probably need to check that this is a toplevel doc.  Or something.

> nsresult
>+nsDOMOfflineResourceList::GetDocumentAppCacheContainer(nsIApplicationCacheContainer **aAppCacheContainer)

This method always returns NS_OK, doesn't it?  Why not make its signature be:

 already_AddRefed<nsIApplicationCacheContainer> GetDocumentAppCacheContainer();

?  That would incindetally fix the issue it has now where it never inits the out param in some cases.

>+nsresult
>+nsDOMOfflineResourceList::GetDocumentAppCache(nsIApplicationCache **aApplicationCache)

This should return already_AddRefed<nsIApplicationCache>.  And again, it currently doesn't init the out param.

> nsDOMOfflineResourceList::CacheKeys()
>+  nsCAutoString manifestSpec;
>+  mManifestURI->GetAsciiSpec(manifestSpec);

Why can't this use mManifestSpec?  If there is a reason, it certainly deserves a comment here!

>+    gCachedManifestSpec = mManifestSpec;

Especially given that.

>+++ b/dom/src/offline/nsDOMOfflineResourceList.h	Wed Jul 02 12:49:49 2008 -0700
>+  nsCString mManifestSpec;

Document that this is guaranteed to me the AsciiSpec of mManifestURI?

>+++ b/dom/tests/mochitest/ajax/offline/offlineTests.js	Wed Jul 02 12:49:49 2008 -0700
> clear: function()
>+  var appCacheService = Cc["@mozilla.org/network/application-cache-service;1"]
>+                      .getService(Ci.nsIApplicationCacheService);

Fix the indent?

>+    if (expectEntry) {
>+      this.ok(false, url + " should not exist in the offline cache");
>+    } else {
>+      this.ok(true, url + " should not exist in the offline cache");
>+    }

Isn't that whole block the same as:

  this.ok(!expectEntry, url + " should not exist in the offline cache");

>+++ b/netwerk/base/public/nsIApplicationCache.idl	Wed Jul 02 12:49:49 2008 -070

I'd really prefer one idl file per interface, I think.  Certainly the service should have its own idl.

>+ * Application caches store resources for offline use.  A given
>+ * application cache belongs to a group of application caches
>+ * associated with a given cache manifest URI.

It might be worth talking about any invariants a group has (e.g. all caches in a group have the same id, different client IDs (do they?), and only one of them is active at a given point in time).

>+     * Entries in an application cache can be marked as one of the
>+     * following types:

Or rather one or more, right?  Are all combinations of types OK?  If some are and some aren't, it definitely needs documenting.

>+     * The client ID for this application cache.  This is the ID that
>+     * clients should use when interacting with the cache service to
>+     * access the items in this cache.

That's the nsICacheService, not the nsIApplicationCacheService, right?  Might be clearer to just say it.  And maybe explicitly say that this is what you pass to getCacheSession, etc?

>+     * Makes this cache the active application cache for this group.
>+     * Future loads associated with this group will come from this
>+     * cache.

This will handle toggling the state of the other caches in the group as needed, right?  Good to spell that out.

>+     * Discard this application cache.  Removes all cached resources
>+     * for this cache.  If this is the active application cache for the
>+     * group, the group will be removed.

So it will also remove all the inactive caches in that case?  OK.

Should there be a way to check whether a cache is active, by the way?

>+    void markEntry(in ACString key, in unsigned long type);

Call this typeBits to make it clear that you can add more than one?  Same in the implementation, and throughout where type is used.

>+    void unmarkEntry(in ACString key, in unsigned long type);

Same here.

>+     * Gather entries of a given type.

Can this be a set of multiple type bits?  Or only one type bit?  Document, please.  And if the latter, assert accordingly.

>+++ b/netwerk/base/src/nsLoadGroup.cpp	Wed Jul 02 12:49:49 2008 -0700

These changes should go away, since we talked about using nsHttpChannel::GetCallback for this instead, right?

>+++ b/netwerk/build/nsNetModule.cpp	Wed Jul 02 12:49:49 2008 -0700
>@@ -1040,16 +1045,22 @@ static const nsModuleComponentInfo gNetM
>+    {  NS_APPLICATIONCACHESERVICE_CLASSNAME,
>+       NS_APPLICATIONCACHESERVICE_CID,
>+       NS_APPLICATIONCACHESERVICE_CONTRACTID,
>+       nsOfflineCacheDeviceConstructor
>+    },

That should be #ifdef NECKO_OFFLINE_CACHE.  Otherwise necko will fail to compile without storage.

>+++ b/netwerk/cache/public/nsICacheService.idl	Wed Jul 02 12:49:49 2008 -0700
>@@ -81,21 +81,17 @@ interface nsICacheService : nsISupports
>+     * This method is deprecated and will throw NS_ERROR_NOT_IMPLEMENTED.

Please file a bug to remove it, mention the bug# here, and make sure that bug is being tracked by the "interface changes to actually make when we can" bug.  If we don't have such a tracking bug, please file one?  We really don't want to leave cruft like this around one minute longer than we absolutely have to.

>+++ b/netwerk/cache/src/nsDiskCacheDeviceSQL.cpp	Wed Jul 02 12:49:49 2008 -0700
>+nsOfflineCacheDevice::GetInstance()

>+  nsCacheService *cacheService = reinterpret_cast<nsCacheService*>(serv.get());

I'd rather you static_cast to nsICacheService* and then static_cast again to nsCacheService.  Seems safer.  For example, that works if nsICacheService is not hte first interface in the nsCacheService vtable, unlike the code here.

>@@ -730,112 +878,178 @@ nsOfflineCacheDevice::Init()
>+                         "  ItemType        INTEGER DEFAULT 0\n"

So when I use a profile with 1.9.0, then 1.9.1, then 1.9.0 again I'll be adding a bunch of 0 itemTypes to the table.  If I then go back to 1.9.1, will we deal with that ok?

>+  rv = mDB->ExecuteSimpleSQL(
>+    NS_LITERAL_CSTRING("CREATE INDEX IF NOT EXISTS moz_cache_keys_index"
>+                       " ON moz_cache(Key);"));

So...  Maybe document why the index setup is the way it is?  It's pretty non-obvious (e.g. I have no idea why we have this index but not any indices on moz_cache_groups).

>+    // XXX:
>+    StatementSql ( mStatement_DomainSize,        "SELECT 0;"),

Followup bug, mention number here.

>+    StatementSql ( mStatement_DeactivateGroup,    "DELETE FROM moz_cache_groups WHERE GroupID = ?;" ),

Hmm.  I assume we remove the moz_cache rows that correspond to the various caches in this group somewhere, right?  Where does that removal happen?

>+nsOfflineCacheDevice::InitActiveCaches()
>+{
>+  mCaches.Init();
>+  mActiveCachesByGroup.Init();
>+  mActiveCaches.Init(5);

All three of those can fail, no?  Check for failure?

>+    mActiveCachesByGroup.Put(group, new nsCString(clientID));

If we're going to do that, make clientID an nsCString to start with?  That way we'll just share the buffer instead of doing another buffer copy here.

>@@ -1020,18 +1234,16 @@ nsOfflineCacheDevice::BindEntry(nsCacheE
>   rv = statement->ExecuteStep(&hasRows);
>-  NS_ENSURE_SUCCESS(rv, rv);

Why, exactly?

>+nsOfflineCacheDevice::GatherEntries(const nsACString &clientID,
>+  LOG(("nsOfflineCacheDevice::GatherEntries [cid=%s]\n",
>+       PromiseFlatCString(clientID).get()));

Log the type too?

>+nsOfflineCacheDevice::CreateApplicationCache(const nsACString &group,
>+                                  gNextTemporaryClientID));

That's missing a ++, right?

>+  mCaches.Put(clientID, weak);

Is there any place where we go through and remove the entries in mCaches whose value has a null referent?  I'm not seeing one, so over a browsing session this thing will grow and grow.

We should have such a place, I think.  Followup bug OK if it's too much pain as part of this patch.

>+  cache.swap(*out);

You need to set *out to null at the beginning of the method if you want to do that.  Alternately, you could set *out to cache.forget(), right?

>+nsOfflineCacheDevice::GetApplicationCache(const nsACString &clientID,
>+    if (group.IsEmpty())
>+      return NS_OK;

Don't you need to set the out param to null?

>+    nsRefPtr<nsApplicationCache> appCache =
>+      new nsApplicationCache(this, group, clientID);
>+    if (!appCache)
>+      return NS_ERROR_OUT_OF_MEMORY;
>+    cache = do_QueryInterface(static_cast<nsIApplicationCache*>(appCache));

How about just:

     cache = new nsApplicationCache(this, group, clientID);

?  No need to do that QI....

>+  cache.swap(*out);

Again, *out needs to have been pre-set for this to work.  Otherwise you'll Release garbage memory, if the caller is using a raw ptr.

>+nsOfflineCacheDevice::GetActiveCache(const nsACString &group,
>+                                     nsIApplicationCache **out)
...
>+  return NS_OK;

Need to set *out to null here.

>+nsOfflineCacheDevice::ChooseApplicationCache(const nsACString &key,
>+                                             nsIApplicationCache **out)
...
>+    if (mActiveCaches.Contains(clientID))
>+      return GetApplicationCache(clientID, out);

So why does the query select the ItemType too?

>   return NS_OK;

Need to null out *out.

Maybe just start nulling out your out params up front in all functions that have them?  That's the safe thing to do....

>+nsOfflineCacheDevice::ActivateCache(const nsACString &group,
>+    mActiveCaches.Put(clientID);
>+    mActiveCachesByGroup.Put(group, new nsCString(clientID));

So after this point, if your sqlite munging fails your hashes will not match the database state.  Can that be avoided somehow?  Or do we at least handle such a mismatch sanely (at least from a security/stability perspective)?

>+nsOfflineCacheDevice::DeactivateGroup(const nsACString &group)
>+    mActiveCaches.Remove(*active);
>+    mActiveCachesByGroup.Remove(group);

Similar issue here.

>+nsOfflineCacheDevice::GetGroupForCache(const nsACString &clientID,
>+  group.Truncate(group.FindChar('|'));

How do we know the original group name (aka manifest spec) didn't have a '|' in it?  I don't see us enforcing that anywhere (and yes, I know '|' is not technically a valid URI char).

>+  group.ReplaceChar('+', ':');

Same here, except '+' most certainly _is_ a valid URI char.

>+++ b/netwerk/cache/src/nsDiskCacheDeviceSQL.h	Wed Jul 02 12:49:49 2008 -0700
>+  nsresult                ActivateCache(const nsACString &group,
>+                                        const nsACString &clientID);
>+  PRBool                  IsActiveCache(const nsACString &group,
>+                                        const nsACString &clientID);
>+  nsresult                DeactivateGroup(const nsACString &group);
>+  nsresult                GetGroupForCache(const nsACString &clientID,
>+                                           nsACString &out);

This is not public (IDL) API, right?  Make those nsCSubstrings instead of nsACStrings, in that case.  Or even nsCStrings if we ever need to .get() on them.

>+  nsresult MarkEntry(const nsACString &clientID,
>+  nsresult UnmarkEntry(const nsACString &clientID,
>+  nsresult GetTypes(const nsACString &clientID,
>+  nsresult GatherEntries(const nsACString &clientID,

Probably same here, for the clientID.  The other args do come from IDL.

>+++ b/netwerk/protocol/http/src/nsHttpChannel.cpp	Wed Jul 02 12:49:49 2008 -0700

>@@ -1381,36 +1382,71 @@ nsHttpChannel::OpenCacheEntry(PRBool off
>+        // Pick up an application cache from the load group if available
>+        nsCOMPtr<nsIApplicationCacheContainer> appCacheContainer =
>+            do_GetInterface(mLoadGroup);

So this should be:

  nsCOMPtr<nsIApplicationCacheContainer> appCacheContainer;
  GetCallback(appCacheContainer);
    

>+++ b/netwerk/protocol/http/src/nsHttpChannel.h	Wed Jul 02 12:49:49 2008 -0700
>     nsresult OpenCacheEntry(PRBool offline, PRBool *delayed);
>+
>     nsresult OpenOfflineCacheEntryForWriting();

Why add that newline?

>+++ b/uriloader/prefetch/nsOfflineCacheUpdate.cpp	Wed Jul 02 12:49:49 2008 -0700
>+nsOfflineCacheUpdate::GetCacheKey(nsIURI *aURI, nsACString &aKey)

Might be safer to clone the URI, QI to nsIURL, if that succeeds SetRef(EmptyCString()), and then GetAsciiSpec.  Hand-parsing of URIs scares me....

>@@ -788,67 +829,39 @@ nsOfflineCacheUpdate::Init(PRBool aParti
>+    // Partial updates don't need a new application cache
>     if (aPartialUpdate) {
>-        mCacheSession = mMainCacheSession;
>+        mApplicationCache = mPreviousApplicationCache;

Can it happen that aPartialUpdate is true but mPreviousApplicationCache is null?  If so, we'll crash when calling GetClientID below.  If it's not possible, I'd like to know why in comments here, and with a nice assert to that effect.

I'd like another look once you update this stuff.

And there's still the issue of images, right?  Is there a separate patch somewhere that handles them?

Comment 9

[Boris Zbarsky [:bzbarsky]]

I'm also a bit unclear about toplevel document loads here.  We'll try to do them from the application cache of the document that's currently loaded in the docshell, right?  That seems incorrect to me.

Comment 10

[Dave Camp (:dcamp)]

(In reply to comment #9)
> I'm also a bit unclear about toplevel document loads here.  We'll try to do
> them from the application cache of the document that's currently loaded in the
> docshell, right?  That seems incorrect to me.

There's a potential workaround for that in bug 442806 - if you want to take a quick look at that, I can roll it in to the next version of this patch.

Comment 11

[Dave Camp (:dcamp)]

(In reply to comment #10)

> There's a potential workaround for that in bug 442806 - if you want to take a
> quick look at that, I can roll it in to the next version of this patch.

err, wrong bug - I meant 445544. 

Comment 12

[Boris Zbarsky [:bzbarsky]]

I should really cc myself on bugs I review, so I see the responses to my questions...

I'll take a look at that bug when I get back.

Also, I assume there will be tests for this stuff at some point before 1.9.1 final?

Comment 13

[Honza Bambas (:mayhemer)]

(In reply to comment #8)
> (From update of attachment 327848 [details] [diff] [review])
> >+nsOfflineCacheDevice::CreateApplicationCache(const nsACString &group,
> >+                                  gNextTemporaryClientID));
> 
> That's missing a ++, right?

Fixed (+ some other fixes around this) in 443023.

Comment 14

[Dave Camp (:dcamp)]

(In reply to comment #8)

> >+++ b/content/base/src/nsDocument.cpp	Wed Jul 02 12:49:49 2008 -0700
> >+++ b/content/base/src/nsDocument.h	Wed Jul 02 12:49:49 2008 -0700

> >+  // nsIApplicationCacheContainer
> >+  NS_DECL_NSIAPPLICATIONCACHECONTAINER
> 
> No need for the comment, since it's pretty clear what interface this is.

As mentioned on irc, I'm leaving that in because it's the prevailing style of the file.


> >+++ b/docshell/base/nsDocShell.cpp	Wed Jul 02 12:49:49 2008 -0700
> 
> >+    else if (aIID.Equals(NS_GET_IID(nsIApplicationCacheContainer)) &&
> >+             NS_SUCCEEDED(EnsureContentViewer())) {
> >+        // Return the toplevel document as an
> >+        // nsIApplicationCacheContainer.
> 
> Hmm.  So if site A puts site B in a subframe.  And site B uses offline stuff. 
> Will this make site B hit site A's cache?

While loaded as a frame of site A, site B will be loaded from site A's cache.  Site B's manifest/cache will be essentially ignored (except, as an optimization, to mark its entry in site A's cache as FOREIGN, preventing a toplevel load of site b from site a's cache).

If site B tries to access its window.applicationCache object, it will get a security error, because it is not same-origin with the owner of the application cache it was loaded from.

>  Is that desirable?  Is there a
> security review in the offing?  I realize this is what the spec says to do, by
> the way.  Just checking that it's secure and doesn't allow cross-site injection
> or anything like that.

I added this to the notes for the impending security review.

> >+        nsCOMPtr<nsIDocShellTreeItem> rootItem;
> >+        GetSameTypeRootTreeItem(getter_AddRefs(rootItem));
> >+        nsCOMPtr<nsIDocShell> root = do_QueryInterface(rootItem);
> >+        if (!root)
> >+            return NS_ERROR_NO_INTERFACE;
> 
> That will never happen, so I think it's safe to just assert that root is
> non-null.
> 
> >+        root->GetContentViewer(getter_AddRefs(contentViewer));
> >+        if (!contentViewer)
> >+            return NS_ERROR_NO_INTERFACE;
> 
> Again, I'm not sure this should be happening if the docshell tree is sane.  I
> can see null-checking this to be safe, but please assert that contentViewer is
> non-null.
> 
> >+        contentViewer->GetDOMDocument(getter_AddRefs(domDoc));
> >+        if (!domDoc)
> >+            return NS_ERROR_NO_INTERFACE;
> 
> Same for domDoc.

I changed this based on comment 4.

> >+                permissionManager->TestExactPermission(aURI, "offline-app",
> >+                                                       &perm);
> 
> Hmm.  So we seem to do this test in a number of places, not all the same way. 
> For example, nsContentUtils::OfflineAppAllowed does something quite different
> from what you do here.  This test does match the one in GetQuota in
> nsDOMStorage.cpp, but I would think we'd want to use the same exact test in all
> three places.  The GetQuota knows it has an http:// URI, but your don't here. 
> In particular, you need to look at the innermost URI of aURI, and probably at
> the "offline-apps.allow_by_default" pref as needed.  And likely do the
> http/https thing that nsContentUtils does, too.

I brought in a copy of nsContentUtils::OfflineAppAllowed() into nsDocShell.cpp (is there a decent common place to put this?).  I filed 450667 about updating nsDOMStorage.cpp appropriately.

> 
> >+                    loadFlags |= nsICachingChannel::LOAD_CHECK_OFFLINE_CACHE;
> 
> So... I just looked through, and on redirect we just send this flag along, even
> if redirecting to a domain that doesn't have that permission.  That seems to be
> an existing problem, but we should still fix it.  Followup bug is probably OK.

Filed as 450175.

> >+++ b/dom/src/offline/nsDOMOfflineResourceList.cpp	Wed Jul 02 12:49:49 2008 -0700
> >+static nsCAutoString gCachedManifestSpec;
> 
> So... generally this is bad: it'll show up as a leak any time the string value
> is longer than 80 chars or whatever we use now.  Followup bug to not us a
> static object here?

Filed as 450174.

> >@@ -402,23 +403,25 @@ nsDOMOfflineResourceList::GetStatus(PRUi
> >     *aStatus = nsIDOMOfflineResourceList::IDLE;
> >   } else {
> >     *aStatus = nsIDOMOfflineResourceList::UNCACHED;
> 
> Is there a separate bug on CHECKING, DOWNLOADING, etc?

Those are handled by an earlier clause if there's an update in progress.

That said, I incorporated a bugfix from honza into this method, so it warrants a second look.

> >@@ -449,17 +452,44 @@ nsDOMOfflineResourceList::SwapCache()
> >+  rv = GetDocumentAppCache(getter_AddRefs(currentAppCache));
> 
> That doesn't look right.  In particular, here's what the spec says here (from
> <http://www.whatwg.org/specs/web-apps/current-work/multipage/offline.html#swapcache>):
..
> In other words, we probably need to check that this is a toplevel doc.  Or
> something.

We now throw INVALID_STATE_ERR if we're not the toplevel doc.

> >+++ b/dom/src/offline/nsDOMOfflineResourceList.h	Wed Jul 02 12:49:49 2008 -0700

> >+++ b/dom/tests/mochitest/ajax/offline/offlineTests.js	Wed Jul 02 12:49:49 2008 -0700

> >+    if (expectEntry) {
> >+      this.ok(false, url + " should not exist in the offline cache");
> >+    } else {
> >+      this.ok(true, url + " should not exist in the offline cache");
> >+    }
> 
> Isn't that whole block the same as:
> 
>   this.ok(!expectEntry, url + " should not exist in the offline cache");

It is, but the string for the first clause was meant to be different.  Fixed that.
> 
> >+++ b/netwerk/cache/public/nsICacheService.idl	Wed Jul 02 12:49:49 2008 -0700
> >@@ -81,21 +81,17 @@ interface nsICacheService : nsISupports
> >+     * This method is deprecated and will throw NS_ERROR_NOT_IMPLEMENTED.
> 
> Please file a bug to remove it, mention the bug# here, and make sure that bug
> is being tracked by the "interface changes to actually make when we can" bug. 
> If we don't have such a tracking bug, please file one?  We really don't want to
> leave cruft like this around one minute longer than we absolutely have to.

Filed bug 450180, targetted to mozilla2.0 and marked with a status whiteboard field of obsolete-api (after discussion at the gecko meeting).

> >+++ b/netwerk/cache/src/nsDiskCacheDeviceSQL.cpp	Wed Jul 02 12:49:49 2008 -0700

> >@@ -730,112 +878,178 @@ nsOfflineCacheDevice::Init()
> >+                         "  ItemType        INTEGER DEFAULT 0\n"
> 
> So when I use a profile with 1.9.0, then 1.9.1, then 1.9.0 again I'll be adding
> a bunch of 0 itemTypes to the table.  If I then go back to 1.9.1, will we deal
> with that ok?

The way this code is in this patch, it won't cause any problems.  Any items added to the cache in 1.9.0 won't belong to an active cache group (because 1.9.0 didn't have them).  So they'll be cleaned up on shutdown.

And when going back from 1.9.1 to 1.9.0, items won't be accessed (because they aren't in the HTTP-offline clientID).  They won't have any ownerships (because 1.9.1 doesn't use those anymore) and will be cleaned up on shutdown.

So basically, each transition between 1.9.0 and 1.9.1 will end up clearing out the db.  Some other options we have are:

a) Explicitly clear out the database on an upgrade.
b) Store the new database in a different directory, leaving a stale cache for people upgrading once and never downgrading.

Thoughts?

> >+  rv = mDB->ExecuteSimpleSQL(
> >+    NS_LITERAL_CSTRING("CREATE INDEX IF NOT EXISTS moz_cache_keys_index"
> >+                       " ON moz_cache(Key);"));
> 
> So...  Maybe document why the index setup is the way it is?  It's pretty
> non-obvious (e.g. I have no idea why we have this index but not any indices on
> moz_cache_groups).

Added some comments (and actually combined two of the indexes into one).  moz_cache_groups had an index - PRIMARY KEY gets an implicit index.

> >+    StatementSql ( mStatement_DeactivateGroup,    "DELETE FROM moz_cache_groups WHERE GroupID = ?;" ),
> 
> Hmm.  I assume we remove the moz_cache rows that correspond to the various
> caches in this group somewhere, right?  Where does that removal happen?

nsApplicationCache's destructor (which calls DeactiveGroup()) will also evict all entries in the application cache.  As an added precaution, at shutdown we clean up all application caches that aren't currently active.

> Is there any place where we go through and remove the entries in mCaches whose
> value has a null referent?  I'm not seeing one, so over a browsing session this
> thing will grow and grow.

nsApplicationCache's destructor cleans itself out of mCaches.

> >+nsOfflineCacheDevice::ChooseApplicationCache(const nsACString &key,
> >+                                             nsIApplicationCache **out)
> ...
> >+    if (mActiveCaches.Contains(clientID))
> >+      return GetApplicationCache(clientID, out);
> 
> So why does the query select the ItemType too?

Pre-emptively included for one of the later patches, but I removed it and add it in the future patch.

> >+nsOfflineCacheDevice::ActivateCache(const nsACString &group,
> >+    mActiveCaches.Put(clientID);
> >+    mActiveCachesByGroup.Put(group, new nsCString(clientID));
> 
> So after this point, if your sqlite munging fails your hashes will not match
> the database state.  Can that be avoided somehow?  Or do we at least handle
> such a mismatch sanely (at least from a security/stability perspective)?

Rearranged so that we update the hashes after the sqlite changes, under the assumption that the sqlite changes are more likely to fail.

If sqlite changes are committed without updates to the hashes finishing, we'll just treat the older cache as active until either a new cache is activated or shutdown (after which we'll reread the database state).

> And there's still the issue of images, right?  Is there a separate patch
> somewhere that handles them?

Bug 442809.

Left out comments that were fixed uninterestingly. 

(In reply to comment #12)
> Also, I assume there will be tests for this stuff at some point before 1.9.1
> final?

There is a test suite in bug 443017.

Comment 15

[Dave Camp (:dcamp)]

Attachment: v2

Comment 16

[Boris Zbarsky [:bzbarsky]]

Is it at all possible to post an interdiff here?  That would make it a lot easier to review...

Comment 17

[Dave Camp (:dcamp)]

Attachment: v2 interdiff

Comment 18

[Boris Zbarsky [:bzbarsky]]

> I brought in a copy of nsContentUtils::OfflineAppAllowed() into nsDocShell.cpp
> (is there a decent common place to put this?).

I was going to say no, but then I realized that it could in fact live in nsNetUtil.  It doesn't use anything Necko doesn't already depend on.

We could give it an optional (or even required, if we want, since both callers have one) nsIPrefBranch argument to avoid the hassle of getting one.   We'd still want to keep the nsContentUtils signature, and just have it call the nsNetUtil method.

Doesn't help with codesize, but at least helps with mindprint...

> So basically, each transition between 1.9.0 and 1.9.1 will end up clearing out
> the db. 

I think that's fine.  No need to do anything more complicated.

>nsOfflineCacheDevice::CreateApplicationCache(const nsACString &group,
>+  if (!NS_Escape(nsCString(group), clientID, url_Path)) {

We should probably document that some chars are special as far as the cache is concerned and that we need to make sure that we escape them here, since we plan to use this string as the cache key.

>+++ b/netwerk/protocol/http/src/nsHttpChannel.cpp	Tue Aug 19 08:22:39 2008 -0700
>+        // XXX: need to fix this.
>+        NS_ENSURE_TRUE(PR_FALSE, NS_ERROR_FAILURE);

Let's not put this part in the tree for now.  Just go back to the early-return version this used to be.

Comment 19

[Boris Zbarsky [:bzbarsky]]

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

r+sr=bzbarsky with the nits from comment 28 addressed.  Thanks a ton for the interdiff: it made this a lot simpler!

Comment 20

[Dave Camp (:dcamp)]

Attachment: nit fixes

Fixes requested in comment 28.

Comment 21

[Dave Camp (:dcamp)]

Attachment: final patch

Comment 22

[Boris Zbarsky [:bzbarsky]]

>+#include "nsIPermissionManager.h"

No need for that in nsNetUtil.h, no?  Just in nsIOService.cpp....

Comment 23

[Dave Camp (:dcamp)]

indeed, will fix that and post a new patch before landing.

Comment 24

[Dave Camp (:dcamp)]

Attachment: updated final patch

Comment 25

[Dave Camp (:dcamp)]

Pushed in rev ea551e6f0f40

Comment 26

[Dave Camp (:dcamp)]

Bugs 442803, 442806, and 442812 were in the regression range for a places unit test failure and were all backed out in an attempt to fix it.

Comment 27

[Dave Camp (:dcamp)]

Attachment: handle application cache access after shutdown

Two of the places tests seem to sometimes open http channels after shutdown.  This tries to start up the application cache, which crashes because the cache service has been shut down.

The attached patch does two things:
a) Access to the application cache service after shutdown will no longer crash.
b) Just being offline no longer sets LOAD_CHECK_OFFLINE_CACHE - this is now handled by the docshell, the channel shouldn't do it automatically.

(I'll be filing a bug on the places tests)

Comment 28

[Dave Camp (:dcamp)]

Attachment: final patch, again

Landed as http://hg.mozilla.org/mozilla-central/index.cgi/rev/3e41c4b388c3

Comment 29

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Followup patch

This was causing mochitests to fail locally for me. I'm checking it in with r+sr=jst (the warnings this caused really should be errors in gcc and MSVC :-/).

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

I pushed the followup as http://hg.mozilla.org/mozilla-central/index.cgi/rev/6464982e3c7f

Comment 31

[M** A****]

Could this checkin have caused
https://bugzilla.mozilla.org/show_bug.cgi?id=452775

Comment 32

[Benjamin Smedberg]

This landed prior to 1.9.1 branching

Comment 33

[Nickolay_Ponomarev]

dev-doc-needed: the doc team wasn't notified about this, so some docs were not updated. E.g. I came here because https://developer.mozilla.org/en/nsIDOMOfflineResourceList#swapCache.28.29 still claims swapCache() is not supported.