460353 - app caches should be per iframe, not per toplevel browsing context

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Dave Camp (:dcamp)]

The offline spec was changed to associate application caches per-iframe rather than per toplevel browsing context.  Off of the top of my head, I think we'll need to update:

* nsDocShell needs to remove the "is this toplevel" check in LoadURI
* nsDocShell should just return its own document for getInterface(nsIApplicationCacheContainer)
* nsDOMOfflineResourceList needs to be updated to allow normal access to non-toplevel items
* nsContentSink needs to be updated to do the cache selection stuff for non-toplevel items.

The last one may end up causing extra manifest checks..

Comment 1

[Honza Bambas (:mayhemer)]

-> me, fixing on top of the patch from bug 445544.

Comment 2

[Dave Camp (:dcamp)]

Attachment: wip

Work in progress, mostly honza's with some tweaks from me.

Comment 3

[Honza Bambas (:mayhemer)]

Attachment: v1

First version including a test for this feature. Depends on the test suit. Dave, please take a look at the test.

Comment 4

[Dave Camp (:dcamp)]

Comment on attachment 345626 [details] [diff] [review]
v1

>diff --git a/content/base/src/nsContentSink.cpp b/content/base/src/nsContentSink.cpp
>--- a/content/base/src/nsContentSink.cpp
>+++ b/content/base/src/nsContentSink.cpp

>-    // If there is an existing application cache for this manifest,
>-    // associate it with the document.
>-    nsCAutoString manifestURISpec;
>-    rv = aManifestURI->GetAsciiSpec(manifestURISpec);
>-    NS_ENSURE_SUCCESS(rv, rv);
>-
>-    nsCOMPtr<nsIApplicationCacheService> appCacheService =
>-      do_GetService(NS_APPLICATIONCACHESERVICE_CONTRACTID);
>-    if (!appCacheService) {
>-      // No application cache service, nothing to do here.
>-      return NS_OK;
>-    }
>-
>-    rv = appCacheService->GetActiveCache(manifestURISpec,
>-                                         getter_AddRefs(applicationCache));
>-    NS_ENSURE_SUCCESS(rv, rv);
>-
>-    if (applicationCache) {
>-      rv = applicationCacheDocument->SetApplicationCache(applicationCache);
>-      NS_ENSURE_SUCCESS(rv, rv);
>+      // the manifest specified.
>+      *aAction = CACHE_SELECTION_RESELECT_WITHOUT_MANIFEST;

Note for Boris or any other reviewers:  the spec did change to remove this explicit association at cache selection time.  The document will be associated with the application cache at the end of the cache update process (see bug 445544).

>-
>-  if (applicationCache) {
>-    // We are now associated with an application cache.  This item
>-    // should be marked as an implicit entry.
>-    nsCAutoString cachekey;
>-    rv = GetChannelCacheKey(mDocument->GetChannel(), cachekey);
>-    if (NS_SUCCEEDED(rv)) {
>-      rv = applicationCache->MarkEntry(cachekey,
>-                                       nsIApplicationCache::ITEM_IMPLICIT);
>-      NS_ENSURE_SUCCESS(rv, rv);
>+      // Always do an update in this case
>+      *aAction = CACHE_SELECTION_UPDATE;

Similarly, marking the entry as implicit should now happen during the update process.

> nsresult
> nsContentSink::SelectDocAppCacheNoManifest(nsIApplicationCache *aLoadApplicationCache,
>-                                           PRBool aIsTopDocument,
>                                            nsIURI **aManifestURI,
>                                            CacheSelectionAction *aAction)
> {
>   *aManifestURI = nsnull;
>   *aAction = CACHE_SELECTION_NONE;
> 
>-  if (!aIsTopDocument || !aLoadApplicationCache) {
>-    return NS_OK;
>-  }
>-
>   nsresult rv;
> 
>-  // The document was loaded from an application cache, use that
>-  // application cache as the document's application cache.
>-  nsCOMPtr<nsIApplicationCacheContainer> applicationCacheDocument =
>-    do_QueryInterface(mDocument);
>-  NS_ASSERTION(applicationCacheDocument,
>-               "mDocument must implement nsIApplicationCacheContainer.");
>+  if (aLoadApplicationCache) {
>+    // The document was loaded from an application cache, use that
>+    // application cache as the document's application cache.
>+    nsCOMPtr<nsIApplicationCacheContainer> applicationCacheDocument =
>+      do_QueryInterface(mDocument);
>+    NS_ASSERTION(applicationCacheDocument,
>+                 "mDocument must implement nsIApplicationCacheContainer.");
> 
>-  rv = applicationCacheDocument->SetApplicationCache(aLoadApplicationCache);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+    nsCAutoString docURISpec, clientID;
>+    mDocumentURI->GetAsciiSpec(docURISpec);
>+    aLoadApplicationCache->GetClientID(clientID);
>+    SINK_TRACE(gContentSinkLogModuleInfo, SINK_TRACE_CALLS,
>+        ("Selection, no manifest: assigning app cache %s to document %s", clientID.get(), docURISpec.get()));

if we don't use docURISpec or clientID for anything but this logging, can it be wrapped in some sort of #if PR_LOGGING (or whatever the relevant define is for SINK_TRACE).

> void
> nsContentSink::ProcessOfflineManifest(nsIContent *aElement)
> {
>   // Only check the manifest for root document nodes.
>   if (aElement != mDocument->GetRootContent()) {
>     return;
>   }
>+
>+  nsCOMPtr<nsIApplicationCacheContainer> applicationCacheDocument =
>+      do_QueryInterface(mDocument);
>+  if (!applicationCacheDocument) {
>+    // If the document is not an application cache supporting channel
>+    // we can bypass cache selection algorithm here as an optimization
>+    return;
>+  }

We assume/assert that documents are cache containers elsewhere in the code.

>-  if (manifestSpec.IsEmpty() && !applicationCache) {
>-    // Not loaded from an application cache, and no manifest
>-    // attribute.  Nothing to do here.
>-    return;
>-  }

I think this clause is still relevant, and worth keeping in, right?

>   CacheSelectionAction action = CACHE_SELECTION_NONE;
>   nsCOMPtr<nsIURI> manifestURI;
> 
>   if (manifestSpec.IsEmpty()) {
>-    rv = SelectDocAppCacheNoManifest(applicationCache,
>-                                     isTop,
>-                                     getter_AddRefs(manifestURI),
>-                                     &action);
>-    if (NS_FAILED(rv)) {
>-      return;
>-    }
>+    action = CACHE_SELECTION_RESELECT_WITHOUT_MANIFEST;
>   }
>   else {
>     nsContentUtils::NewURIWithDocumentCharset(getter_AddRefs(manifestURI),
>                                               manifestSpec, mDocument,
>                                               mDocumentURI);
>     if (!manifestURI) {
>       return;
>     }
> 
>     // Documents must list a manifest from the same origin
>     rv = mDocument->NodePrincipal()->CheckMayLoad(manifestURI, PR_TRUE);
>     if (NS_FAILED(rv)) {
>-      return;
>+      action = CACHE_SELECTION_RESELECT_WITHOUT_MANIFEST;
>     }
>+    else {
>+      // Only continue if the document has permission to use offline APIs.
>+      if (!nsContentUtils::OfflineAppAllowed(mDocument->NodePrincipal())) {
>+        return;
>+      }
> 
>-    // Only continue if the document has permission to use offline APIs.
>-    if (!nsContentUtils::OfflineAppAllowed(mDocument->NodePrincipal())) {
>-      return;
>+      PRBool fetchedWithHTTPGetOrEquiv = PR_FALSE;
>+      nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(mDocument->GetChannel()));
>+      if (httpChannel) {
>+        nsCAutoString method;
>+        rv = httpChannel->GetRequestMethod(method);
>+        if (NS_SUCCEEDED(rv))
>+          fetchedWithHTTPGetOrEquiv = method.Equals("GET");
>+      }
>+
>+      rv = SelectDocAppCache(applicationCache, manifestURI,
>+                             fetchedWithHTTPGetOrEquiv, &action);
>+      if (NS_FAILED(rv)) {
>+        return;
>+      }
>     }
>+  }
> 
>-    PRBool fetchedWithHTTPGetOrEquiv = PR_FALSE;
>-    nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(mDocument->GetChannel()));
>-    if (httpChannel) {
>-      nsCAutoString method;
>-      rv = httpChannel->GetRequestMethod(method);
>-      if (NS_SUCCEEDED(rv))
>-        fetchedWithHTTPGetOrEquiv = method.Equals("GET");
>-    }
>-
>-    rv = SelectDocAppCache(applicationCache, manifestURI, isTop,
>-                           fetchedWithHTTPGetOrEquiv, &action);
>+  if (action == CACHE_SELECTION_RESELECT_WITHOUT_MANIFEST) {
>+    rv = SelectDocAppCacheNoManifest(applicationCache,
>+                                     getter_AddRefs(manifestURI),
>+                                     &action);
>     if (NS_FAILED(rv)) {
>       return;
>     }
>   }
> 
>   switch (action)
>   {
>   case CACHE_SELECTION_NONE:
>-    return;
>+    break;
>   case CACHE_SELECTION_UPDATE: {
>     nsCOMPtr<nsIOfflineCacheUpdateService> updateService =
>       do_GetService(NS_OFFLINECACHEUPDATESERVICE_CONTRACTID);
> 
>     if (updateService) {
>       nsCOMPtr<nsIDOMDocument> domdoc = do_QueryInterface(mDocument);
>       updateService->ScheduleOnDocumentStop(manifestURI, mDocumentURI, domdoc);
>     }
>     break;
>   }
>   case CACHE_SELECTION_RELOAD: {
>     // This situation occurs only for toplevel documents, see bottom
>     // of SelectDocAppCache method.
>-    NS_ASSERTION(isTop, "Should only reload toplevel documents!");
>     nsCOMPtr<nsIWebNavigation> webNav = do_QueryInterface(mDocShell);
> 
>     webNav->Stop(nsIWebNavigation::STOP_ALL);
>     webNav->Reload(nsIWebNavigation::LOAD_FLAGS_NONE);
>     break;
>   }
>+  default:
>+    break;

It might be worth asserting here - the process should really be resulting in one of the first three actions.

>diff --git a/dom/src/offline/nsDOMOfflineResourceList.cpp b/dom/src/offline/nsDOMOfflineResourceList.cpp

> NS_IMETHODIMP
> nsDOMOfflineResourceList::SwapCache()
> {
>   nsresult rv = Init();
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   if (!nsContentUtils::OfflineAppAllowed(mDocumentURI)) {
>     return NS_ERROR_DOM_SECURITY_ERR;
>   }

Hrm, unrelated, but we should be using the doc principal rather than the URI here (followup).

>diff --git a/uriloader/prefetch/nsOfflineCacheUpdate.cpp b/uriloader/prefetch/nsOfflineCacheUpdate.cpp
>--- a/uriloader/prefetch/nsOfflineCacheUpdate.cpp
>+++ b/uriloader/prefetch/nsOfflineCacheUpdate.cpp

>     // Use the existing application cache as the cache to check.
>     rv = appCacheChannel->SetApplicationCache(mPreviousApplicationCache);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    rv = appCacheChannel->SetChooseApplicationCache(PR_FALSE);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    rv = appCacheChannel->SetInheritApplicationCache(PR_FALSE);
>     NS_ENSURE_SUCCESS(rv, rv);

Choose and inherit application cache are only relevant if a cache hasn't been explicitly set.  So these aren't necessary.  The comments in nsIApplicationCacheChannel should probably be updated to reflect that.

>     // configure HTTP specific stuff
>     nsCOMPtr<nsIHttpChannel> httpChannel =
>         do_QueryInterface(mChannel);
>     if (httpChannel) {
>         httpChannel->SetReferrer(mReferrerURI);
>         httpChannel->SetRequestHeader(NS_LITERAL_CSTRING("X-Moz"),
>                                       NS_LITERAL_CSTRING("offline-resource"),
>                                       PR_FALSE);
>@@ -1446,20 +1452,22 @@ nsOfflineCacheUpdate::AssociateDocument(
>     nsCOMPtr<nsIApplicationCacheContainer> container =
>         do_QueryInterface(aDocument);
>     if (!container)
>         return NS_OK;
> 
>     nsCOMPtr<nsIApplicationCache> existingCache;
>     nsresult rv = container->GetApplicationCache(getter_AddRefs(existingCache));
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>     if (!existingCache) {
>+        LOG(("Update %p: associating app cache %s to document %p", this, mClientID.get(), aDocument));
>+
>         rv = container->SetApplicationCache(mApplicationCache);
>         NS_ENSURE_SUCCESS(rv, rv);
>     }
> 
>     return NS_OK;
> }

I haven't looked at the test changes in this patch yet, need to look at the other test patch first.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I don't think I'll be able to get to this in time for freeze...

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to comment #4)
> > void
> > nsContentSink::ProcessOfflineManifest(nsIContent *aElement)
> > {
> >   // Only check the manifest for root document nodes.
> >   if (aElement != mDocument->GetRootContent()) {
> >     return;
> >   }
> >+
> >+  nsCOMPtr<nsIApplicationCacheContainer> applicationCacheDocument =
> >+      do_QueryInterface(mDocument);
> >+  if (!applicationCacheDocument) {
> >+    // If the document is not an application cache supporting channel
> >+    // we can bypass cache selection algorithm here as an optimization
> >+    return;
> >+  }
> 
> We assume/assert that documents are cache containers elsewhere in the code.
> 

Hmm... because I had to remove the clause bellow, I needed some optimization to run this code just for relevant documents.

> >-  if (manifestSpec.IsEmpty() && !applicationCache) {
> >-    // Not loaded from an application cache, and no manifest
> >-    // attribute.  Nothing to do here.
> >-    return;
> >-  }
> 
> I think this clause is still relevant, and worth keeping in, right?
> 

True is, that removal of this clause was needed just because of the parent inheritance. So I will re-review this.

> >   case CACHE_SELECTION_RELOAD: {
> >     // This situation occurs only for toplevel documents, see bottom
> >     // of SelectDocAppCache method.
> >-    NS_ASSERTION(isTop, "Should only reload toplevel documents!");
> >     nsCOMPtr<nsIWebNavigation> webNav = do_QueryInterface(mDocShell);
> > 
> >     webNav->Stop(nsIWebNavigation::STOP_ALL);
> >     webNav->Reload(nsIWebNavigation::LOAD_FLAGS_NONE);
> >     break;
> >   }
> >+  default:
> >+    break;
> 
> It might be worth asserting here - the process should really be resulting in
> one of the first three actions.
> 

I had the same tough.

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: v1.1

Addressed dave's comments (reverted few unnecessary changes left for the iframe inheritance).

Comment 8

[Honza Bambas (:mayhemer)]

Attachment: v1.1

Same as previous but removed some fragile code from one of the tests.

Comment 9

[Dave Camp (:dcamp)]

Comment on attachment 345732 [details] [diff] [review]
v1.1

>diff --git a/dom/tests/mochitest/ajax/offline/test_bug460353.html b/dom/tests/mochitest/ajax/offline/test_bug460353.html
>+  its assocaited cache. There is no check the cache is the

"associated", and "check that the cache"...

Looks good other than that.  Johnny, do you have time to take a look at this
patch?

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 345732 [details] [diff] [review]
v1.1

Looks good to me. r+sr=jst

Comment 11

[Honza Bambas (:mayhemer)]

Attachment: test update

This is patch enhancement that enables to get a cache really associated with a document. Problem is that we had to enhance DOMClassInfo for document to enable it.

Comment 12

[Dave Camp (:dcamp)]

Landed as http://hg.mozilla.org/mozilla-central/rev/bfaeea708b98

Honza, could you open another bug for that test update please?

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 346701 [details] [diff] [review]
test update

This patch is now attached to bug 463522.