443023 - Implement document application cache association on first cache load

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Honza Bambas (:mayhemer)]

Attachment: Application cache fixes, v1

Fix for:
- document association
- applicationCache.state exception throw
- deletion of fallback/whitelist mapping on cache deletion (not sure of this part)
- cache selection w/o a manifest didn't appropriately recognize the resource was not loaded from the app cache

Some parts of this patch block tests.

Comment 1

[Dave Camp (:dcamp)]

Comment on attachment 327672 [details] [diff] [review]
Application cache fixes, v1


>diff --git a/content/base/src/nsDocument.h b/content/base/src/nsDocument.h
>--- a/content/base/src/nsDocument.h
>+++ b/content/base/src/nsDocument.h
>@@ -91,6 +91,7 @@
> #include "nsIChannel.h"
> #include "nsCycleCollectionParticipant.h"
> #include "nsIApplicationCache.h"
>+#include "nsIObserver.h"
> 
> // Put these here so all document impls get them automatically
> #include "nsHTMLStyleSheet.h"
>@@ -294,7 +295,8 @@
>                    public nsIScriptObjectPrincipal,
>                    public nsIRadioGroupContainer,
>                    public nsStubMutationObserver,
>-                   public nsIApplicationCacheContainer
>+                   public nsIApplicationCacheContainer,
>+                   public nsIObserver
> {

I'm not really sure here, but maybe it'd be nicer to just have a small wrapper class that watches updates for the document, keeping all the document-associating logic closer together (rather than having some of it in nsDocument.cpp).

>diff --git a/netwerk/cache/src/nsDiskCacheDeviceSQL.cpp b/netwerk/cache/src/nsDiskCacheDeviceSQL.cpp

It looks like the changes to nsDiskCacheDeviceSQL.cpp really belong in the opportunistic mapping patch, can we move it there?

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

>     nsCOMPtr<nsIDocumentLoader> loader(do_QueryInterface(container, &rv));
>     if (NS_FAILED(rv)) return rv;
> 
>+    // Check the document was fetched from application cache
>     nsCOMPtr<nsIChannel> channel;
>     rv = loader->GetDocumentChannel(getter_AddRefs(channel));
>     if (NS_FAILED(rv)) return rv;
> 
>-    nsCOMPtr<nsIApplicationCacheContainer> applicationCacheChannel(do_QueryInterface(channel, &rv));
>+    nsCOMPtr<nsICachingChannel> cachingChannel(do_QueryInterface(channel, &rv));
>+    if (NS_FAILED(rv)) return rv;
>+
>+    nsCOMPtr<nsISupports> token;
>+    rv = cachingChannel->GetCacheToken(getter_AddRefs(token));
>+    if (NS_FAILED(rv)) return rv;
>+    
>+    nsCOMPtr<nsICacheEntryDescriptor> descriptor(do_QueryInterface(token, &rv));
>     if (NS_FAILED(rv)) return NS_OK;
> 
>-    // Grab the application cache the document was loaded from
>-    nsCOMPtr<nsIApplicationCache> applicationCache;
>-    rv = applicationCacheChannel->GetApplicationCache(getter_AddRefs(applicationCache));
>-    if (NS_FAILED(rv)) return NS_OK;
>+    nsCacheStoragePolicy policy;
>+    rv = descriptor->GetStoragePolicy(&policy);
>+    if (policy == nsICache::STORE_OFFLINE)
>+    {
>+        nsCOMPtr<nsIApplicationCacheContainer> applicationCacheChannel(do_QueryInterface(channel, &rv));
>+        if (NS_FAILED(rv)) return NS_OK;
> 
>-    if (applicationCache)
>-    {
>+        // Grab the application cache the document was loaded from
>+        nsCOMPtr<nsIApplicationCache> applicationCache;
>+        rv = applicationCacheChannel->GetApplicationCache(getter_AddRefs(applicationCache));
>+        NS_ENSURE_SUCCESS(rv, rv);
>+

Could you explain why this change? I'm not sure I understand.

Comment 2

[Honza Bambas (:mayhemer)]

Attachment: Application cache fixes, v2

I introduced the wrapper for cache update observing, quit good idea!

Changes to sql device are actually a preview of what should be fixed. I am not sure this is the right place to do such cleanup because it is IMHO inefficient.

The last hunk of the previous patch was recognizing correctly that the resource was really loaded from the offline cache. I have to rethink that more deeply.

Comment 3

[Honza Bambas (:mayhemer)]

Attachment: Application cache fixes, v3

fixes:
- document association on first cache load
- adding document with defined manifest URI as an implicit entry+its fetch to the correct cache even the manifest had not been modified
- nits with applicationCache.status attribute
- opportunistic caching flags/itemtype columns issue
- issue with client id generation and its global counter component
- added tab as fallback separator in manifest
- fixed opportunistic namespace filtering on cache manifest update
- removing unused opportunistic/fallback/whitelist mappings

Comment 4

[Honza Bambas (:mayhemer)]

Attachment: Application cache fixes, v4

fixes:
- document association on first cache load
- adding document with defined manifest URI as an implicit entry+its fetch to
the correct cache even the manifest had not been modified
- nits with applicationCache.status attribute
- issue with client id generation and its global counter component
- added tab as fallback separator in manifest
- removing unused opportunistic/fallback/whitelist mappings

I revert association of cache and document to the original version: nsDocument is observer to start and finish of an update. Dave noticed there might be leak of the wrapper object and it really might be. It is not simple to release the object when any step between its initiation and finish of an update fails. I personally don't like solution with hashtable (actually a hashtable of arrays of weak references). It is a lot of management and when the hashtable is in the cache update service not all properties of the document are accessible. When placed in the content sink it is possible but IMHO still too complicated to access all the properties needed. Note this algorithm is also used to force caching of document as an implicit entry. The hashtable management is already present in the observer service. Access to the properties is much more simple from the document itself.

Comment 5

[Dave Camp (:dcamp)]

Comment on attachment 329235 [details] [diff] [review]
Application cache fixes, v4


>-  *aAction = eNone;
>+  *aAction = CACHE_SELECTION_NONE;

All of the ECacheSelectionAction changes belong in the patch for 442812.


>+  if (!strcmp(aTopic, "offline-cache-update-added"))
>+  {
>+    if (!mChannel)
>+      return NS_OK;
>+
>+    nsCOMPtr<nsIURI> documentURI;
>+    rv = mChannel->GetOriginalURI(getter_AddRefs(documentURI));
>+    if (NS_FAILED(rv))
>+      return NS_OK;
>+
>+    rv = update->AddImplicitURI(documentURI, PR_TRUE /*Force update*/);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    observerService->RemoveObserver(this, "offline-cache-update-added");
>+    return NS_OK;

So if I'm understanding correctly, this doesn't always force an update because the observer is only added if the cache selection algorithm found an associated cache that didn't contain this implicit URI.  That should probably be explained in the comments here (if I'm right).


>--- a/netwerk/cache/src/nsDiskCacheDeviceSQL.cpp
>+++ b/netwerk/cache/src/nsDiskCacheDeviceSQL.cpp

Does all this eviction stuff belong in 444807?

>-[scriptable, uuid(877261bb-b952-4d27-847e-859bdd47c0ec)]
>+interface nsIApplicationCache;
>+
>+[scriptable, uuid(8cb54da0-1c8a-4cf0-b8bd-00567eb7c4a6)]
> interface nsIOfflineCacheUpdate : nsISupports {
>   /**
>    * Fetch the status of the running update.  This will return a value
>@@ -134,6 +136,11 @@ interface nsIOfflineCacheUpdate : nsISup
>    * attempt.
>    */
>   readonly attribute boolean isUpgrade;
>+
>+  /**
>+   * Application cache the channel was fething to.
>+   */

typo, s/fething/fetching/

Comment 6

[Honza Bambas (:mayhemer)]

Attachment: v5

All comments addressed. This patch is just fixing the document first load association.

Comment 7

[Dave Camp (:dcamp)]

Attachment: another approach

This patch takes a different approach, just saving up the documents that need to be updated in the update object.

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 344204 [details] [diff] [review]
another approach

Works for me with updated test.

Comment 9

[Honza Bambas (:mayhemer)]

I remember now why I have choose the observer approach and not the Dave's one. There is said in the spec we must wait for all implicit (master) entries to finish download if already in progress during update. Imagine a 10 or so MB top document referencing a manifest and having an iframe at the start referencing the same manifest. The iframe, when finished, will start an update of the manifest's cache with reference to its self as a master entry. If the top 10MB document load continues after the update finished and load of that document fails, we should discard the whole cache. What more, the cache doesn't even know the top document exists, because reference to it just lies at nsOfflineCacheUpdateService::mDocUpdates as PendingUpdate class instance.

This is not highly probable and imho not a major issue. Create a minor follow up?

Comment 10

[Dave Camp (:dcamp)]

I think that comment mostly applies to 461325, right?  This bug is just about cache association, and doesn't really change how we queue up master/implicit entries...

Comment 11

[Dave Camp (:dcamp)]

http://hg.mozilla.org/mozilla-central/rev/b5fcc6701197