Closed Bug 460353 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: Networking: Cache, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dcamp, Assigned: mayhemer)

References

()

Details

Attachments

(1 file, 4 obsolete files)

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..
-> me, fixing on top of the patch from bug 445544.
Assignee: nobody → honzab
Depends on: 445544
Depends on: 443017
Attached patch wip (obsolete) — Splinter Review
Work in progress, mostly honza's with some tweaks from me.
Depends on: 443023
Attached patch v1 (obsolete) — Splinter Review
First version including a test for this feature. Depends on the test suit. Dave, please take a look at the test.
Attachment #345369 - Attachment is obsolete: true
Attachment #345626 - Flags: review?(dcamp)
Attachment #345626 - Flags: review?(bzbarsky)
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.
I don't think I'll be able to get to this in time for freeze...
(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.
Attached patch v1.1 (obsolete) — Splinter Review
Addressed dave's comments (reverted few unnecessary changes left for the iframe inheritance).
Attachment #345626 - Attachment is obsolete: true
Attachment #345726 - Flags: review?(dcamp)
Attachment #345626 - Flags: review?(dcamp)
Attachment #345626 - Flags: review?(bzbarsky)
Attached patch v1.1Splinter Review
Same as previous but removed some fragile code from one of the tests.
Attachment #345726 - Attachment is obsolete: true
Attachment #345732 - Flags: review?(dcamp)
Attachment #345726 - Flags: review?(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?
Attachment #345732 - Flags: superreview?(jst)
Attachment #345732 - Flags: review?(jst)
Attachment #345732 - Flags: review?(dcamp)
Attachment #345732 - Flags: review+
Blocks: 461325
Comment on attachment 345732 [details] [diff] [review]
v1.1

Looks good to me. r+sr=jst
Attachment #345732 - Flags: superreview?(jst)
Attachment #345732 - Flags: superreview+
Attachment #345732 - Flags: review?(jst)
Attachment #345732 - Flags: review+
Attached patch test update (obsolete) — Splinter Review
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.
Attachment #346701 - Flags: review?(dcamp)
Landed as http://hg.mozilla.org/mozilla-central/rev/bfaeea708b98

Honza, could you open another bug for that test update please?
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Blocks: 463522
Attachment #346701 - Attachment is obsolete: true
Attachment #346701 - Flags: review?(dcamp)
Comment on attachment 346701 [details] [diff] [review]
test update

This patch is now attached to bug 463522.
You need to log in before you can comment on or make changes to this bug.