The whatwg offline spec requires that new versions of the application not be downloaded if the manifest is byte-for-byte identical to previous values. nsOfflineCacheUpdate should store an md5sum in the cache metadata for the manifest and check that when downloading a new version of the metadata.

- added new attribute nsICachingChannel.offlineCacheToken accessing offline app cache - changed preference of the cache usage, offline cache is preferred when LOAD_CHECK_OFFLINE_CACHE is set in loadFlags of the channel - offline cache is open with nsICache::ACCESS_READ only rights to prevent write to it (see comments in the patch) - added hash computation of the manifest document and its value stored on success load to "offline-manifest-hash" meta data of the

Comment on attachment 292662 [details] [diff] [review] First working draft for byte-for-byte check of manifest document - added hash computation of the manifest document into nsOfflineManifestItem and its value stored on success load to "offline-manifest-hash" meta data of the offline cache token (usage of the new attribute on nsICachingChannel) - when newly computed hash value is the same (byte-for-byte identity check) mNeedsUpdate is dropped

Must be merged after attachment 287153 [details] [diff] [review] is in. This test probably reveals bug in the cache expiration. Running just the new test in this patch (test_identicalManifest) and then test_offlineIFrame, the letter will fail as follows: ok - http://localhost:8888/tests/dom/tests/mochitest/ajax/offline/simpleManifest.cacheManifest should exist in the offline cache not ok - http://localhost:8888/tests/SimpleTest/SimpleTest.js should exist in the offline cache not ok - http://localhost:8888/MochiKit/packed.js should exist in the offline cache not ok - http://localhost:8888/tests/dom/tests/mochitest/ajax/offline/offlineTests.js should exist in the offline cache The new test run passes every time.

Comment on attachment 293934 [details] [diff] [review] Tests, v2, merged Dave would be a better reviewer for this stuff, it seems. If not, please pass this back and I'll look at it.

Merged with attachment 295171 [details] [diff] [review]

Merged with attachment 295172 [details] [diff] [review]

Comment on attachment 295837 [details] [diff] [review] Byte-for-byte check of manifest document, v2 >@@ -407,27 +411,53 @@ NS_METHOD > nsOfflineManifestItem::ReadManifest(nsIInputStream *aInputStream, ... > if (manifest->mParserState == PARSE_ERROR) { > // parse already failed, ignore this > return NS_OK; > } > >+ if (!manifest->mManifestHashInitialized) { >+ // Avoid creation of crypto hash when LOAD_ONLY_IF_MODIFIED flag is set This comment seems wrong... >+ // Avoid re-creation of crypto hash when it fails from some reason the first time >+ manifest->mManifestHashInitialized = PR_TRUE; >+ >+ manifest->mManifestHash = do_CreateInstance("@mozilla.org/security/hash;1", &rv); >+ if (NS_SUCCEEDED(rv)) >+ { Bracket should be on the if() line. >+ rv = manifest->mManifestHash->Init(nsICryptoHash::MD5); >+ if (NS_FAILED(rv)) { >+ manifest->mManifestHash = nsnull; >+ LOG(("Could not initialize manifest hash for byte-to-byte check, rv=%08x", rv)); >+ } >+ } >+ } >+ >+ if (manifest->mManifestHash) { >+ rv = manifest->mManifestHash->Update((const PRUint8 *)(aFromSegment + aOffset), aCount); Should use a C++-style cast here. >+nsresult >+nsOfflineManifestItem::SaveOldManifestContentHash(nsIRequest *aRequest) >+{ Maybe this method should be called "GetOld" rather than "SaveOld"? >+ >+nsresult >+nsOfflineManifestItem::CheckNewManifestContentHash(nsIRequest *aRequest) >+{ >+ nsresult rv; >+ >+ if (!mManifestHash) { >+ // Nothing to compare against... >+ return NS_OK; >+ } >+ >+ nsCString newManifestHashValue; >+ rv = mManifestHash->Finish(PR_TRUE, newManifestHashValue); >+ mManifestHash = nsnull; >+ >+ if (NS_FAILED(rv)) { >+ LOG(("Could not finish manifest hash, rv=%08x", rv)); >+ // This is not critical error >+ return NS_OK; >+ } >+ >+ if (!ParseSucceeded()) { >+ // Parsing failed, the hash is not valid >+ return NS_OK; >+ } >+ >+ if (mOldManifestHashValue == newManifestHashValue) { >+ LOG(("Update not needed, downloaded manifest content is byte-for-byte identical")); >+ mNeedsUpdate = PR_FALSE; >+ } >+ >+ // Store the manifest content hash value to the new >+ // offline cache token >+ nsCOMPtr<nsICachingChannel> cachingChannel = do_QueryInterface(aRequest, &rv); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ nsCOMPtr<nsISupports> cacheToken; >+ cachingChannel->GetOfflineCacheToken(getter_AddRefs(cacheToken)); >+ if (cacheToken) { >+ nsCOMPtr<nsICacheEntryDescriptor> cacheDescryptor(do_QueryInterface(cacheToken, &rv)); Typo: should be cacheDescriptor

All comments fixed, fixed header of the test HTML page and finally merged with current HEAD.

Comment on attachment 297573 [details] [diff] [review] Byte-for-byte check of manifest document, v3 + tests !(NS_SUCCEEDED(rv) || rv == NS_ERROR_CACHE_WAIT_FOR_VALIDATION)) I think that'd be more readable as (NS_FAILED(rv) && rv != NS_ERROR_CACHE_WAIT_FOR_VALIDATION)) mOldManifestHashValue = EmptyCString(); mOldManifestHashValue.Truncate() ? // read previous manifest content hash value remove one space before "manifest" Technically this doesn't do a byte-for-byte comparison. If the spec doesn't mention that hashing is OK too maybe it should be changed.

Fixed comments from Biesi.

Checking in netwerk/base/public/nsICachingChannel.idl; /cvsroot/mozilla/netwerk/base/public/nsICachingChannel.idl,v <-- nsICachingChannel.idl new revision: 1.15; previous revision: 1.14 done Checking in netwerk/protocol/http/src/nsHttpChannel.cpp; /cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp,v <-- nsHttpChannel.cpp new revision: 1.325; previous revision: 1.324 done Checking in uriloader/prefetch/nsOfflineCacheUpdate.cpp; /cvsroot/mozilla/uriloader/prefetch/nsOfflineCacheUpdate.cpp,v <-- nsOfflineCacheUpdate.cpp new revision: 1.7; previous revision: 1.6 done Checking in uriloader/prefetch/nsOfflineCacheUpdate.h; /cvsroot/mozilla/uriloader/prefetch/nsOfflineCacheUpdate.h,v <-- nsOfflineCacheUpdate.h new revision: 1.5; previous revision: 1.4 done Checking in dom/tests/mochitest/ajax/offline/Makefile.in; /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/Makefile.in,v <-- Makefile.in new revision: 1.7; previous revision: 1.6 done RCS file: /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/test_identicalManifest.html,v done Checking in dom/tests/mochitest/ajax/offline/test_identicalManifest.html; /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/test_identicalManifest.html,v <-- test_identicalManifest.html initial revision: 1.1 done