Closed Bug 405693 Opened 13 years ago Closed 13 years ago

don't update identical offline cache manifests

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: dcamp, Assigned: mayhemer)

References

Details

Attachments

(1 file, 7 obsolete files)

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.
Flags: blocking1.9?
Blocks: 398161
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
- 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
Attached patch Tests (obsolete) — Splinter Review
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.
Attachment #292662 - Attachment is obsolete: true
Attachment #293933 - Flags: review?(cbiesinger)
Attached patch Tests, v2, merged (obsolete) — Splinter Review
Attachment #292667 - Attachment is obsolete: true
Attachment #293934 - Flags: review?(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.
Attachment #293934 - Flags: review?(jst) → review?(dcamp)
Merged with attachment 295171 [details] [diff] [review]
Attachment #293933 - Attachment is obsolete: true
Attachment #295837 - Flags: review?(cbiesinger)
Attachment #293933 - Flags: review?(cbiesinger)
Attached patch Tests, v3, merged (obsolete) — Splinter Review
Merged with attachment 295172 [details] [diff] [review]
Attachment #293934 - Attachment is obsolete: true
Attachment #295838 - Flags: review?(dcamp)
Attachment #293934 - Flags: review?(dcamp)
Assignee: nobody → honzab
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.
Attachment #295837 - Attachment is obsolete: true
Attachment #295838 - Attachment is obsolete: true
Attachment #297573 - Flags: review?(dcamp)
Attachment #295837 - Flags: review?(cbiesinger)
Attachment #295838 - Flags: review?(dcamp)
Attachment #297573 - Flags: superreview?(cbiesinger)
Attachment #297573 - Flags: review?(dcamp) → review+
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.
Attachment #297573 - Flags: superreview?(cbiesinger) → superreview+
Fixed comments from Biesi.
Attachment #297573 - Attachment is obsolete: true
Keywords: checkin-needed
Version: unspecified → Trunk
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
Status: NEW → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M11
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.