405693 - don't update identical offline cache manifests

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect, P2)

Description

[Dave Camp (:dcamp)]

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.

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: First working draft for byte-for-byte check of manifest document

- 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 2

[Honza Bambas (:mayhemer)]

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

Comment 3

[Honza Bambas (:mayhemer)]

Attachment: Tests

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 4

[Honza Bambas (:mayhemer)]

Attachment: Byte-for-byte check of manifest document

Comment 5

[Honza Bambas (:mayhemer)]

Attachment: Tests, v2, merged

Comment 6

[Johnny Stenback (:jst)]

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.

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: Byte-for-byte check of manifest document, v2

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

Comment 8

[Honza Bambas (:mayhemer)]

Attachment: Tests, v3, merged

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

Comment 9

[Dave Camp (:dcamp)]

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

Comment 10

[Honza Bambas (:mayhemer)]

Attachment: Byte-for-byte check of manifest document, v3 + tests

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

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 12

[Honza Bambas (:mayhemer)]

Attachment: Byte-for-byte check of manifest document, v3 + tests, final

Fixed comments from Biesi.

Comment 13

[Reed Loden [:reed]]

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