Bug 442806 leaves out an implementation of the "cache selection algorithm" for confirming the application cache selected during load.

Comment on attachment 327660 [details] [diff] [review] First working version >diff --git a/content/base/src/nsContentSink.cpp b/content/base/src/nsContentSink.cpp >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 Nit: the brace style for nsContentSink.cpp puts opening braces on the same line. >@@ -849,12 +850,38 @@ void > void > nsContentSink::ProcessOfflineManifest(nsIContent *aElement) ... >+ nsCOMPtr<nsIDOMDocument> domdoc = do_QueryInterface(mDocument); >+ nsCOMPtr<nsIOfflineCacheUpdateService> updateService = >+ do_GetService(NS_OFFLINECACHEUPDATESERVICE_CONTRACTID); >+ > // Check for a manifest= attribute. > nsAutoString manifestSpec; > aElement->GetAttr(kNameSpaceID_None, nsGkAtoms::manifest, manifestSpec); > >- if (manifestSpec.IsEmpty() || >- manifestSpec.FindChar('#') != kNotFound) { >+ if (manifestSpec.IsEmpty()) >+ { >+ updateService->ScheduleOnDocumentStop(nsnull, mDocumentURI, domdoc, isTop); >+ return; >+ } So if the manifest attribute is empty and we weren't loaded from an application cache, we don't need to do anything with respect to application caches. Since that is the (probably overwhelmingly) common case, we might want to save ourselves a bit of time and exit out more quickly if that's the case, without instantiating the offline update service. >diff --git a/netwerk/base/public/nsNetUtil.h b/netwerk/base/public/nsNetUtil.h >--- a/netwerk/base/public/nsNetUtil.h >+++ b/netwerk/base/public/nsNetUtil.h >+inline nsresult >+NS_GetURIOrigin(nsIURI* uri, nsACString& _retval) ... >+inline nsresult >+NS_IsURIOriginEqual(nsIURI *uri1, nsIURI *uri2, PRBool *isEqual) I'd be a bit wary of putting functions like this in easily-accessible commonly-used places like nsNetUtil.h. While the application cache spec does explicitly require same-scheme-host-port, I think in general the policies are moving away from strict same-origin checking. Adding easily-accessible functions to do the opposite might cause problems. But I don't have a better suggestion off the top of my head; I'll defer to people that know this stuff better than I. > > /** >diff --git a/netwerk/cache/src/nsDiskCacheDeviceSQL.cpp b/netwerk/cache/src/nsDiskCacheDeviceSQL.cpp >--- a/netwerk/cache/src/nsDiskCacheDeviceSQL.cpp >+++ b/netwerk/cache/src/nsDiskCacheDeviceSQL.cpp >@@ -942,7 +942,7 @@ nsOfflineCacheDevice::Init() > StatementSql ( mStatement_UpdateEntrySize, "UPDATE moz_cache SET DataSize = ? WHERE ClientID = ? AND Key = ?;" ), > StatementSql ( mStatement_UpdateEntryFlags, "UPDATE moz_cache SET Flags = ? WHERE ClientID = ? AND Key = ?;" ), > StatementSql ( mStatement_DeleteEntry, "DELETE FROM moz_cache WHERE ClientID = ? AND Key = ?;" ), >- StatementSql ( mStatement_FindEntry, "SELECT MetaData, Generation, Flags, DataSize, FetchCount, LastFetched, LastModified, ExpirationTime FROM moz_cache WHERE ClientID = ? AND Key = ?;" ), >+ StatementSql ( mStatement_FindEntry, "SELECT MetaData, Generation, Flags, DataSize, FetchCount, LastFetched, LastModified, ExpirationTime, ItemType FROM moz_cache WHERE ClientID = ? AND Key = ?;" ), Is this change used anymore? >diff --git a/netwerk/protocol/http/src/nsHttpChannel.cpp b/netwerk/protocol/http/src/nsHttpChannel.cpp >--- a/netwerk/protocol/http/src/nsHttpChannel.cpp >+++ b/netwerk/protocol/http/src/nsHttpChannel.cpp >@@ -1450,6 +1450,11 @@ nsHttpChannel::OpenCacheEntry(PRBool off > rv = session->OpenCacheEntry(cacheKey, accessRequested, PR_FALSE, > getter_AddRefs(mCacheEntry)); > } >+ else if (mApplicationCache && !mCacheForOfflineUse) >+ // XXX The condition is hack: we must allow network load when updating but >+ // mCacheForOfflineUse is not exactly indication of that. >+ // Entry was found in offline application cache, do not allow network actions >+ mLoadFlags |= LOAD_ONLY_FROM_CACHE | LOAD_NO_NETWORK_IO; Hrm, I think this condition actually belongs in the opportunistic caching/whitelisting patch? >diff --git a/uriloader/prefetch/Makefile.in b/uriloader/prefetch/Makefile.in >--- a/uriloader/prefetch/Makefile.in >+++ b/uriloader/prefetch/Makefile.in >@@ -47,12 +47,15 @@ LIBXUL_LIBRARY = 1 > LIBXUL_LIBRARY = 1 > REQUIRES = xpcom \ > dom \ >+ content \ > string \ > necko \ > uriloader \ > nkcache \ >+ layout \ > chardet \ > pref \ >+ widget \ > $(NULL) As an aside - nsOfflineCacheUpdate.cpp is in uriloader/prefetch/ for dumb historical reasons. I think we'll want to move it to dom/src/offline/ at some point, in which case we need to remember to clean up the REQUIRES here. >diff --git a/uriloader/prefetch/nsIOfflineCacheUpdate.idl b/uriloader/prefetch/nsIOfflineCacheUpdate.idl >--- a/uriloader/prefetch/nsIOfflineCacheUpdate.idl >+++ b/uriloader/prefetch/nsIOfflineCacheUpdate.idl >@@ -115,7 +115,7 @@ interface nsIOfflineCacheUpdateObserver > * load its items one by one, sending itemCompleted() to any registered > * observers. > */ >-[scriptable, uuid(4b206247-82ee-46cf-a8b7-f7284e753bc2)] >+[scriptable, uuid(0d2e445e-96ee-4be6-a07d-eb48fa9d2232)] > interface nsIOfflineCacheUpdate : nsISupports { > /** > * Fetch the status of the running update. This will return a value >@@ -154,26 +154,47 @@ interface nsIOfflineCacheUpdate : nsISup > /** > * Initialize the update. > * >- * @param aPartialUpdate >- * TRUE if the update should just update the URIs given to it, >- * FALSE if all URLs for the owner domain should be added. > * @param aManifestURI >- * The manifest URI to be checked, or for partial updates the >- * manifest that should own resources that are added. >+ * The manifest URI to be checked. > * @param aDocumentURI > * The page that is requesting the update. > */ >- void init(in boolean aPartialUpdate, >- in nsIURI aManifestURI, >- in nsIURI aDocumentURI); >+ void init(in nsIURI aManifestURI, in nsIURI aDocumentURI); > > /** >- * Add a URI to the offline cache as part of the update. >+ * Initialize the update for partial processing. >+ * >+ * @param clientID >+ * ClientID of the cache to store resource to. >+ * @param aDocumentURI >+ * The page that is requesting the update. May be null >+ * when this information is unknown. >+ */ >+ void initPartial(in ACString clientID, in nsIURI aDocumentURI); I like this change. >+ void addOpportunisticURI(in nsIURI aURI); >+ >+ /** >+ * Add an implicit URI to the offline cache as part of the update. >+ * >+ * @param aURI >+ * The document URI to add. >+ */ >+ void addImplicitURI(in nsIURI aURI); Do we still need these methods? I think they're artifacts of an old version of the patch. >diff --git a/uriloader/prefetch/nsOfflineCacheUpdate.cpp b/uriloader/prefetch/nsOfflineCacheUpdate.cpp This file has a brace style mismatch too. >+ else // !applicationCache >+ { >+ // Document has already been loaded from >+ // an applicaton cache. >+ nsCAutoString groupID; >+ rv = applicationCache->GetGroupID(groupID); >+ if (NS_FAILED(rv)) return NS_OK; >+ >+ nsCOMPtr<nsIURI> groupIDURI; >+ rv = NS_NewURI(getter_AddRefs(groupIDURI), groupID, nsnull, aManifestURI); >+ if (NS_FAILED(rv)) return NS_OK; groupURI would probably be easier to read. >+ 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; >+ >+ nsCAutoString key; >+ rv = descriptor->GetKey(key); >+ if (NS_FAILED(rv)) return rv; >+ >+ rv = applicationCache->MarkEntry(key, nsIApplicationCache::ITEM_FOREIGN); >+ if (NS_FAILED(rv)) return rv; >+ >+ LOG(("Document marked as foreign !")); >+ *aAction = aIsTopDocument ? eReload : eNone; >+ >+ return NS_OK; >+ } >+ } >+ >+ nsCAutoString documentURIspec; >+ rv = channelURI->GetAsciiSpec(documentURIspec); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ if (applicationCache) >+ { >+ rv = applicationCache->MarkEntry(documentURIspec, nsIApplicationCache::ITEM_IMPLICIT); >+ if (NS_FAILED(rv)) return rv; >+ } Hrm, in one case you're using the channel's cache key, in the other you're using the document URI. You should be consistent here (though either one is ok, really). But if you stick with the asciispec, be sure to use nsOfflineCacheUpdate::GetCacheKey() to sanitize it. So the last thing is that it's kind of strange that ScheduleOnDocumentStop() now performs a somewhat complicated algorithm dealing with documents/channels/etc and then Maybe schedules an update. It also means the cache update service, which should only have one job (update the cache), is now also making document-association decisions too. So I wonder if it would be cleaner to put these functions in the content sink. The cache selection algorithm is really part of parsing process (as specced, and conceptually), so I think the code should belong there.

Comment on attachment 327660 [details] [diff] [review] First working version One more thing, you don't seem to use: >+nsresult >+nsOfflineCacheUpdateService::FindRunningUpdate( >+ nsIURI *aManifestURI, >+ nsOfflineCacheUpdate **_retval) anywhere, but it looks like it's a step toward: >+ // XXX TODO - if there is already a scheduled update or >+ // update in progress we have to add this document as >+ // an implicit entry. We should either finish this before landing, or file a followup.

(In reply to comment #2) > > >+inline nsresult > >+NS_IsURIOriginEqual(nsIURI *uri1, nsIURI *uri2, PRBool *isEqual) > > I'd be a bit wary of putting functions like this in easily-accessible > commonly-used places like nsNetUtil.h. While the application cache spec does > explicitly require same-scheme-host-port, I think in general the policies are > moving away from strict same-origin checking. Adding easily-accessible > functions to do the opposite might cause problems. But I don't have a better > suggestion off the top of my head; I'll defer to people that know this stuff > better than I. I talked with jst about this on irc - it would probably be best to just rename these to not include "origin" in the name. Something like NS_SchemeHostPortIsEqual().

Comment on attachment 327660 [details] [diff] [review] First working version >+inline nsresult >+NS_GetURIOrigin(nsIURI* uri, nsACString& _retval) >+{ >+ nsresult rv; >+ >+ nsCAutoString origin, buffer; >+ rv = uri->GetScheme(buffer); >+ if (NS_FAILED(rv)) >+ return rv; >+ origin.Assign(buffer); >+ origin.AppendLiteral("://"); >+ >+ rv = uri->GetAsciiHost(buffer); >+ if (NS_FAILED(rv)) >+ return rv; >+ origin.Append(buffer); >+ >+ PRInt32 port; >+ rv = uri->GetPort(&port); >+ if (NS_FAILED(rv)) >+ return rv; >+ >+ if (port > -1) >+ { >+ origin.AppendLiteral(":"); >+ origin.AppendInt(port); >+ } This assumes that an nsIURI will never return the default port. So if it's possible for an http URI to return "80", it won't work right. I don't know if nsIURI makes that guarantee, but the IDL doesn't say it does.

Bug 407538 is related to the issue from comment #5.

All comments addressed including comment 5.

Comment on attachment 327880 [details] [diff] [review] Second working version >+ // XXX TODO - if there is already a scheduled update or >+ // update in progress we have to add this document as >+ // an implicit entry. >+ } This is pretty minor, but a followup should be filed. >--- a/content/base/src/nsContentSink.h >+++ b/content/base/src/nsContentSink.h >@@ -150,6 +150,12 @@ protected: > nsContentSink(); > virtual ~nsContentSink(); > >+ enum ECacheSelectionAction { >+ eNone = 0, >+ eUpdate = 1, >+ eReload = 2 >+ }; Prevailing style in this area seems to be enum CacheSelectionAction " CACHE_SELECTION_NONE, // etc }; >+inline nsresult >+NS_GetURIOrigin(nsIURI* uri, nsACString& _retval) This should probably be updated to NS_GetURISchemeHostPort too. Looks ok to me, other than that.

(In reply to comment #8) > (From update of attachment 327880 [details] [diff] [review]) > > >+ // XXX TODO - if there is already a scheduled update or > >+ // update in progress we have to add this document as > >+ // an implicit entry. > >+ } > > This is pretty minor, but a followup should be filed. There has already been a patch for this in bug 443023. > > >--- a/content/base/src/nsContentSink.h > >+++ b/content/base/src/nsContentSink.h > >@@ -150,6 +150,12 @@ protected: > > nsContentSink(); > > virtual ~nsContentSink(); > > > >+ enum ECacheSelectionAction { > >+ eNone = 0, > >+ eUpdate = 1, > >+ eReload = 2 > >+ }; > > Prevailing style in this area seems to be > > enum CacheSelectionAction " > CACHE_SELECTION_NONE, > // etc > }; > > >+inline nsresult > >+NS_GetURIOrigin(nsIURI* uri, nsACString& _retval) > > This should probably be updated to NS_GetURISchemeHostPort too. > I will fix after regular review.

Before I read the rest of this, is there a difference between NS_SchemeHostPortIsEqual and nsIScriptSecurityManager::CheckSameOriginURI? Or more precisely, are the differences (esp. as regards handling of jar: URIs) on purpose? Unless we have strong reasons not to, I'd rather move the security manager code into nsNetUtil wholesale and call the new method from the security manager...

I didn't use CheckSameOriginURI just because network module then had to require include caps, xpconnect and js. It becomes uncompilable and IMO it is wrong itself. To move code of nsScriptSecurityManager::SecurityCompareURIs to nsNetUtils would be great solution. That method is using interfaces only from network module.

Yeah, if that would give us the right behavior I would much prefer to see that. What does the spec have to say on the matter?

In the spec there is nothing said about nested URIs (usage of our NS_GetInnermostURI). Except this, the algorithm of SecurityCompareURIs looks to me exactly the same as proposed by the spec.

My real question is whether this means that using NS_GetInnermostURI here would be a spec violation or not.

So, as I understand NS_GetInnermostURI is used mainly (and exclusively?) to extract source uri for JAR files in this form: jar:http://foo.com/bar/my.jar!my_resource.xy. Then I believe it is correct to use the NS_GetInnermostURI for origin compare.

Yeah, it's used for jar: and view-source: and for safe about: URIs (the latter have a moz-safe-about: inner). Of course except for jar: these don't have hosts or ports... That's the other question. If no host or port, do we want to compare not-same-origin, or what?

For definition of the origin, spec says: "If url does not use a server-based naming authority, or if parsing url failed, or if url is not an absolute URL, then return a new globally unique identifier." This IMO includes the case there is no host nor port. The "new globally unique identifier" means, IMO, that origin for such URL could not ever be equal to any other origin. Even comparing a pair of identical URLs' origins would result to FALSE. Then we need a new code to compare origins. SecurityCompareURIs is not suitable.

Why, exactly? Looks to me like it does exactly what you want...

You are right, I overlooked the failure check on GetHost call. It, as I see implementations, succeeds only in standard URL. And, it always has a host otherwise it would hold an invalid URL. But, and it probably bug in the security manager, we must compare GetAsciiHost and not GetHost because of possible several different representations of a single host in case of IDN being used and also because the WHATWG spec says to do it.

I'll probably wait up for an updated patch before reviewing.

Addressing all comment. I created new function in nsNetUtil.h NS_SecurityCompareURIs which is almost exact copy of the same method of the security manager. I only had to play with host string compare which is little bit less efficient then before to make it compilable.

Sorry, v3 was unrefreshed patch. This one contains the fix with GetHost -> GetAsciiHost and correct ignore-case host compare.

Hrm, the fixed naming for ECacheSelectionAction ended up in the patch for 443023, that should be fixed up.

Comment on attachment 330193 [details] [diff] [review] v3.1 >+++ b/caps/src/nsScriptSecurityManager.cpp > nsScriptSecurityManager::SecurityCompareURIs(nsIURI* aSourceURI, >+ NS_ENSURE_TRUE(sIOService, PR_FALSE); I don't think there's a point to having this here, honestly. Just ditch that line. >+++ b/content/base/src/nsContentSink.cpp >+nsContentSink::SelectDocumentApplicationCache( >+ nsIURI *aManifestURI, PRBool aIsTopDocument, ECacheSelectionAction *aAction) I'd rather we put the args one below the other, and put nsIURI right after the '('. >+ // Grab the document's load channel Is there something wrong with mDocument->GetChannel()? If so, it should be clearly documented here so no one simplifies this code to just doing that. >+ nsCOMPtr<nsIApplicationCacheContainer> applicationCacheDocument(do_QueryInterface(mDocument, &rv)); Watch those 80 columns, in a few places here. >+ rv = channel->GetURI(getter_AddRefs(channelURI)); Are we sure we want the channel URI and not the channel principal? What's the right behavior for about:blank, data:, javascript: channels here? >+ PRBool equals = NS_SecurityCompareURIs(channelURI, aManifestURI); Please pass the right boolean for the third arg. Just use the nsContentUtil helpers for getting prefs, with a default of PR_TRUE. That said, you already checked that the document's principal CheckMayLoad() aManifestURI. Why is this additional check needed? >+ if (!equals) Please place curlies around the body, especially given the long comment. If !applicationCache the spec says to only do the application cache thing if "it was loaded using HTTP GET or equivalent". I don't see that reflected in this code... What does that even mean? >+ // Otherwise, find active cache (if any) and associate it "the active cache". >+ nsCOMPtr<nsIApplicationCacheService> appCacheService(do_GetService( >+ NS_APPLICATIONCACHESERVICE_CONTRACTID, &rv)); This would be more readable with a '=' instead of the constructor form. I think they compile to the same thing anyway. >+ rv = appCacheService->GetActiveCache(manifestURISpec, >+ getter_AddRefs(applicationCache)); This fits in 80 columns even without messing with the indent. Please indent the two args identically. >+ // XXX TODO - if there is already a scheduled update or Please put the bug number on doing this in this comment. >+ else { // !applicationCache That makes it look like the else case is that !applicationCache. I'd just take this comment out, since the one on the next line covers things pretty well. >+ // Document has already been loaded from >+ // an applicaton cache. Put that on a single line? >+ rv = NS_NewURI(getter_AddRefs(groupURI), groupID, nsnull, aManifestURI); Do we really want to use aManifestURI as a base URI here? If so, please document why, because that seems like a really weird thing to be doing. >+ // This is top level document and the http manifest attribute URI "a top level document" >+ // is equal to manifest URI of the cache the document was loaded "to the manifest URI" It's not clear to me why some |rv|s get propagated out of this function and some turn into NS_OK. Seems odd to be inconsistent about it. >+nsContentSink::SelectDocumentApplicationCacheNoManifest( >+ nsIURI **aManifestURI, PRBool aIsTopDocument, ECacheSelectionAction *aAction) If you s/Document/Doc/ there you can fit the args after the functon name. >+ // Grab the document's load channel Again, why not just GetChannel()? > nsContentSink::ProcessOfflineManifest(nsIContent *aElement) So first note... As the code stands, this can be called at any point during pageload. That makes it a little difficult to do the "Restart the current navigation from the top" as the spec says to do. Perhaps the "any point" thing should be fixed? >+ nsPIDOMWindow* pwindow = mDocument->GetWindow(); >+ nsCOMPtr<nsIDOMWindow> window = do_QueryInterface(pwindow); Why? Any nsPIDOMWindow is automatically an nsIDOMWindow. >+ nsCOMPtr<nsIDOMWindow> outerwindow = do_QueryInterface(pwindow->GetOuterWindow()); GetWindow() is clearly documented as already returning an outer window. >+ isTop = outerwindow && outerwindow == parentwindow.get(); Which means this check should just be: isTop = (pwindow == parentwindow); and I'd rename |pwindow| to just |window|. > nsCOMPtr<nsPIDOMWindow> pwindow = > do_QueryInterface(mDocument->GetScriptGlobalObject()); Er.... After all the trouble above to put the result of GetWindow() in a separate scope? That's all you want here, no? >+ if (manifestSpec.FindChar('#') != kNotFound) { >+ return; Why? >+ // We only care about manifests in toplevel windows. >+ nsCOMPtr<nsIDOMWindow> window = >+ do_QueryInterface(pwindow->GetOuterWindow()); That's just mDocument->GetWindow(); >+ window->GetParent(getter_AddRefs(parent)); >+ if (parent.get() != window.get()) { >+ return; We already know isTop here. Why are we recomputing it? >+ if (!nsContentUtils::OfflineAppAllowed(mDocumentURI)) { Should that really be the mDocumentURI and not the document's principal's URI, at least? Or possibly even something taking into account document.domain? >+ case eReload: { >+ nsCOMPtr<nsIWebNavigation> webNav(do_QueryInterface(pwindow->GetDocShell())); I assume there's a reason why mDocShell is not acceptable here? (Of course might be worth asserting that the two are equal). >+ webNav->Stop(nsIWebNavigation::STOP_CONTENT | nsIWebNavigation::STOP_NETWORK); So just STOP_ALL, right? >+ webNav->Reload(0); LOAD_FLAGS_NONE, please. I assume you've tested that this does the right thing with session history and so forth? >+++ b/content/base/src/nsContentSink.h >+ enum ECacheSelectionAction { Please document. >+ nsresult GetChannelCacheKey(nsIChannel* aChannel, nsACString& aCacheKey); >+ nsresult SelectDocumentApplicationCache( >+ nsIURI *aManifestURI, PRBool aIsTopDocument, ECacheSelectionAction *aAction); >+ nsresult SelectDocumentApplicationCacheNoManifest( >+ nsIURI **aManifestURI, PRBool aIsTopDocument, ECacheSelectionAction *aAction); Document these as well (and reindent the latter two as I asked for the cpp file). >+++ b/netwerk/base/public/nsNetUtil.h >+NS_GetSchemeHostPortFromURI(nsIURI* uri, nsACString& _retval) This is unused, right? >+inline PRBool >+NS_SecurityCompareURIs(nsIURI* aSourceURI, >+ nsIURI* aTargetURI, >+ PRBool aStrictFileOriginPolicy = PR_FALSE) Fix the indent here. And the default should be the secure PR_TRUE. Or possibly the argument should just be mandatory. >+ // Special handling for mailnews schemes Is there a bug on making this a protocol handler flag? If not, care to file (and maybe even fix)? >+ if (NS_FAILED( targetBaseURI->GetAsciiHost(targetHost) ) || >+ NS_FAILED( sourceBaseURI->GetAsciiHost(sourceHost) )) dveditz should probably double-check this, but I think this is the right thing to do. >+ ToLowerCase(targetHost); >+ ToLowerCase(sourceHost); >+ if (!targetHost.Equals(sourceHost)) The old code just used Equals() with nsCaseInsensitiveCStringComparator(). Why change from doing that, exactly? >+++ b/netwerk/cache/src/nsDiskCacheDeviceSQL.cpp >@@ -1729,14 +1729,35 @@ nsOfflineCacheDevice::ChooseApplicationC >+ PRBool equals = NS_SecurityCompareURIs(keyURI, groupURI); So when could that test false? An example here would be good, especially in terms of making sure people don't try to "simplify" this. Also, no need for the temporary. >+++ b/uriloader/prefetch/nsIOfflineCacheUpdate.idl >@@ -208,7 +213,7 @@ interface nsIOfflineCacheUpdate : nsISup >-[scriptable, uuid(3abee04b-5bbb-4405-b659-35f780e38da0)] >+[scriptable, uuid(8f88363d-6783-4ea9-93c2-d0020ebd807a)] > interface nsIOfflineCacheUpdateService : nsISupports { Why the uuid change? >+++ b/uriloader/prefetch/nsOfflineCacheUpdate.cpp >@@ -158,7 +161,9 @@ nsOfflineCacheUpdateItem::OpenChannel() >+ if (mReferrerURI) Why do you need the null-check? >@@ -847,19 +852,56 @@ nsOfflineCacheUpdate::Init(PRBool aParti >+ rv = cacheService->CreateApplicationCache(manifestSpec, >+ getter_AddRefs(mApplicationCache)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ rv = cacheService->CreateApplicationCache(manifestSpec, >+ getter_AddRefs(mApplicationCache)); >+ NS_ENSURE_SUCCESS(rv, rv); Deja vu. Remove the second set of lines? >+nsOfflineCacheUpdate::InitPartial(const nsACString& clientID, >+ rv = cacheService->GetApplicationCache(mClientID, >+ getter_AddRefs(mApplicationCache)); Please fix the weird indent. >@@ -1594,6 +1636,9 @@ nsOfflineCacheUpdateService::ScheduleOnD >+ nsresult rv; Why?

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

(In reply to comment #24) > (From update of attachment 330193 [details] [diff] [review]) > >+ rv = channel->GetURI(getter_AddRefs(channelURI)); > > Are we sure we want the channel URI and not the channel principal? What's the > right behavior for about:blank, data:, javascript: channels here? > Changed to examine channel principal. If I understand possible issue (inheritance of offline-app permission OR access to the offline application cache of the upper object) then: for the permission inheritance I am not quit sure and this is IMHO not part of this bug (we have to fill a new one with high priority to examine this issue), access to upper offline cache is impossible because the origin is not the same and the embed element cannot work with applicationCache object (cache is not associated with it). > If !applicationCache the spec says to only do the application cache thing if > "it was loaded using HTTP GET or equivalent". I don't see that reflected in > this code... What does that even mean? > Fixed, I added check of the http method to the right place. > So first note... As the code stands, this can be called at any point during > pageload. That makes it a little difficult to do the "Restart the current > navigation from the top" as the spec says to do. Perhaps the "any point" thing > should be fixed? > This is fixed in dave camp's patch by checking the document being top-level - is it what you wanted? > >+ // Special handling for mailnews schemes > > Is there a bug on making this a protocol handler flag? If not, care to file > (and maybe even fix)? > Filed a bug 451081

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

Comment on attachment 334485 [details] [diff] [review] interdiff to v4 Oops, that was to an old version of v4...

Comment on attachment 334321 [details] [diff] [review] v4 >+++ b/content/base/src/nsContentSink.cpp >+nsContentSink::SelectDocAppCache(nsIApplicationCache *loadApplicationCache, aLoadApplicationCache? Please document the arguments in the header! >+ nsCOMPtr<nsIApplicationCacheContainer> applicationCacheDocument = >+ do_QueryInterface(mDocument); >+ if (!applicationCacheDocument) { This won't ever be null, will it? >+nsContentSink::SelectDocAppCacheNoManifest(nsIApplicationCache *loadApplicationCache, Again, aLoadApplicationCache, and please document. >+ // The document was loaded from an application cache, confirm it. Which "it"? >+ nsCOMPtr<nsIApplicationCacheContainer> applicationCacheDocument = >+ do_QueryInterface(mDocument); >+ if (!mDocument) { >+ return NS_OK; >+ } This code doesn't make much sense, to be honest. Did you mean to null-check applicationCacheDocument? Does it need null-checking? > nsContentSink::ProcessOfflineManifest(nsIContent *aElement) >+ // Not loaded from am application cache, and no manifest s/ am / an / > + if (!nsContentUtils::OfflineAppAllowed(mDocument->NodePrincipal())) { I think this is what we want, but please make sure to flag this during the security review. >+ case CACHE_SELECTION_RELOAD: { > This is fixed in dave camp's patch by checking > the document being top-level - I don't see how that addresses my comment, to be honest. Consider a 10-kilobyte HTML document that has an <html> element with a manifest attribute on it somewhere around 8 kilobytes into the file. That can trigger this reload. Should it? What if it has 20 <html> elements, all with different manifest attributes? >+++ b/content/base/src/nsContentSink.h >+ // There is no offline cache manifest attribute specified in the document >+ // or the document was loaded from a cache other then it points to by >+ // its manifest attribute and IS NOT a top level document or an error occured >+ // during cache selection algo. There is no offline cache manifest specified by the document, or the document was loaded from a cache other than the one it specifies via its manifest attribute and IS NOT a top-level document, or an error occurred during the cache selection algorithm. >+ // The document was loaded from a cache other then it points to by its >+ // manifest attribute and IS a top level document. In such a case this >+ // document is marked as foreign in the cache it was load from and >+ // the document must be completelly reloaded to prevents its load from >+ // that (wrong) cache. The document was loaded from a cache other than the one it specifies via its manifest attribute and IS a top-level document. In this case, the document is marked as foreign in the cache it was loaded from and must be reloaded from the correct cache (the one it specifies). >+ // cache, le tit be associated with the document and eventually s/le tit/let it/ >+++ b/netwerk/base/public/nsNetUtil.h >+NS_SecurityCompareURIs(nsIURI* aSourceURI, >+ nsIURI* aTargetURI, >+ PRBool aStrictFileOriginPolicy = PR_TRUE) That third arg no longer needs to be optional, right? r=bzbarsky with those issues addressed.

Fixes for review comments.

Pushed as rev 1e3d4775197a

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.

Problems were in bug 442806, relanded this patch as http://hg.mozilla.org/mozilla-central/index.cgi/rev/f42b560650bf