Bug 1000338 - nsICacheEntry.lastModified not properly implemented, r=michal

netwerk/cache2/CacheEntry.cpp

@@ -759,6 +759,11 @@ void CacheEntry::InvokeAvailableCallback(Callback const & aCallback)
     return;
   }
 
+  if (NS_SUCCEEDED(mFileStatus)) {
+    // Let the last-fetched and fetch-count properties be updated.
+    mFile->OnFetched();
+  }
+
   if (mIsDoomed || aCallback.mNotWanted) {
     LOG(("  doomed or not wanted, notifying OCEA with NS_ERROR_CACHE_KEY_NOT_FOUND"));
     aCallback.mCallback->OnCacheEntryAvailable(

netwerk/cache2/CacheFile.cpp

@@ -910,27 +910,6 @@ CacheFile::GetExpirationTime(uint32_t *_retval)
   return mMetadata->GetExpirationTime(_retval);
 }
 
-nsresult
-CacheFile::SetLastModified(uint32_t aLastModified)
-{
-  CacheFileAutoLock lock(this);
-  MOZ_ASSERT(mMetadata);
-  NS_ENSURE_TRUE(mMetadata, NS_ERROR_UNEXPECTED);
-
-  PostWriteTimer();
-  return mMetadata->SetLastModified(aLastModified);
-}
-
-nsresult
-CacheFile::GetLastModified(uint32_t *_retval)
-{
-  CacheFileAutoLock lock(this);
-  MOZ_ASSERT(mMetadata);
-  NS_ENSURE_TRUE(mMetadata, NS_ERROR_UNEXPECTED);
-
-  return mMetadata->GetLastModified(_retval);
-}
-
 nsresult
 CacheFile::SetFrecency(uint32_t aFrecency)
 {
@@ -956,6 +935,16 @@ CacheFile::GetFrecency(uint32_t *_retval)
   return mMetadata->GetFrecency(_retval);
 }
 
+nsresult
+CacheFile::GetLastModified(uint32_t *_retval)
+{
+  CacheFileAutoLock lock(this);
+  MOZ_ASSERT(mMetadata);
+  NS_ENSURE_TRUE(mMetadata, NS_ERROR_UNEXPECTED);
+
+  return mMetadata->GetLastModified(_retval);
+}
+
 nsresult
 CacheFile::GetLastFetched(uint32_t *_retval)
 {
@@ -976,6 +965,18 @@ CacheFile::GetFetchCount(uint32_t *_retval)
   return mMetadata->GetFetchCount(_retval);
 }
 
+nsresult
+CacheFile::OnFetched()
+{
+  CacheFileAutoLock lock(this);
+  MOZ_ASSERT(mMetadata);
+  NS_ENSURE_TRUE(mMetadata, NS_ERROR_UNEXPECTED);
+
+  PostWriteTimer();
+
+  return mMetadata->OnFetched();
+}
+
 void
 CacheFile::Lock()
 {

netwerk/cache2/CacheFile.h

@@ -90,12 +90,14 @@ public:
   nsresult ElementsSize(uint32_t *_retval);
   nsresult SetExpirationTime(uint32_t aExpirationTime);
   nsresult GetExpirationTime(uint32_t *_retval);
-  nsresult SetLastModified(uint32_t aLastModified);
-  nsresult GetLastModified(uint32_t *_retval);
   nsresult SetFrecency(uint32_t aFrecency);
   nsresult GetFrecency(uint32_t *_retval);
+  nsresult GetLastModified(uint32_t *_retval);
   nsresult GetLastFetched(uint32_t *_retval);
   nsresult GetFetchCount(uint32_t *_retval);
+  // Called by upper layers to indicated the entry has been fetched,
+  // i.e. delivered to the consumer.
+  nsresult OnFetched();
 
   bool DataSize(int64_t* aSize);
   void Key(nsACString& aKey) { aKey = mKey; }

netwerk/cache2/CacheFileMetadata.cpp

@@ -27,6 +27,8 @@ namespace net {
 
 #define kCacheEntryVersion 1
 
+#define NOW_SECONDS() (uint32_t(PR_Now() / PR_USEC_PER_SEC))
+
 NS_IMPL_ISUPPORTS(CacheFileMetadata, CacheFileIOListener)
 
 CacheFileMetadata::CacheFileMetadata(CacheFileHandle *aHandle, const nsACString &aKey)
@@ -82,7 +84,6 @@ CacheFileMetadata::CacheFileMetadata(bool aMemoryOnly, const nsACString &aKey)
   memset(&mMetaHdr, 0, sizeof(CacheFileMetadataHeader));
   mMetaHdr.mVersion = kCacheEntryVersion;
   mMetaHdr.mExpirationTime = nsICacheEntry::NO_EXPIRATION_TIME;
-  mMetaHdr.mFetchCount = 1;
   mKey = aKey;
   mMetaHdr.mKeySize = mKey.Length();
 
@@ -504,7 +505,7 @@ CacheFileMetadata::SetExpirationTime(uint32_t aExpirationTime)
   LOG(("CacheFileMetadata::SetExpirationTime() [this=%p, expirationTime=%d]",
        this, aExpirationTime));
 
-  MarkDirty();
+  MarkDirty(false);
   mMetaHdr.mExpirationTime = aExpirationTime;
   return NS_OK;
 }
@@ -516,31 +517,13 @@ CacheFileMetadata::GetExpirationTime(uint32_t *_retval)
   return NS_OK;
 }
 
-nsresult
-CacheFileMetadata::SetLastModified(uint32_t aLastModified)
-{
-  LOG(("CacheFileMetadata::SetLastModified() [this=%p, lastModified=%d]",
-       this, aLastModified));
-
-  MarkDirty();
-  mMetaHdr.mLastModified = aLastModified;
-  return NS_OK;
-}
-
-nsresult
-CacheFileMetadata::GetLastModified(uint32_t *_retval)
-{
-  *_retval = mMetaHdr.mLastModified;
-  return NS_OK;
-}
-
 nsresult
 CacheFileMetadata::SetFrecency(uint32_t aFrecency)
 {
   LOG(("CacheFileMetadata::SetFrecency() [this=%p, frecency=%f]",
        this, (double)aFrecency));
 
-  MarkDirty();
+  MarkDirty(false);
   mMetaHdr.mFrecency = aFrecency;
   return NS_OK;
 }
@@ -552,6 +535,13 @@ CacheFileMetadata::GetFrecency(uint32_t *_retval)
   return NS_OK;
 }
 
+nsresult
+CacheFileMetadata::GetLastModified(uint32_t *_retval)
+{
+  *_retval = mMetaHdr.mLastModified;
+  return NS_OK;
+}
+
 nsresult
 CacheFileMetadata::GetLastFetched(uint32_t *_retval)
 {
@@ -566,6 +556,25 @@ CacheFileMetadata::GetFetchCount(uint32_t *_retval)
   return NS_OK;
 }
 
+nsresult
+CacheFileMetadata::OnFetched()
+{
+  MarkDirty(false);
+
+  mMetaHdr.mLastFetched = NOW_SECONDS();
+  ++mMetaHdr.mFetchCount;
+  return NS_OK;
+}
+
+void
+CacheFileMetadata::MarkDirty(bool aUpdateLastModified)
+{
+  mIsDirty = true;
+  if (aUpdateLastModified) {
+    mMetaHdr.mLastModified = NOW_SECONDS();
+  }
+}
+
 nsresult
 CacheFileMetadata::OnFileOpened(CacheFileHandle *aHandle, nsresult aResult)
 {
@@ -721,7 +730,7 @@ CacheFileMetadata::InitEmptyMetadata()
   }
   mOffset = 0;
   mMetaHdr.mVersion = kCacheEntryVersion;
-  mMetaHdr.mFetchCount = 1;
+  mMetaHdr.mFetchCount = 0;
   mMetaHdr.mExpirationTime = nsICacheEntry::NO_EXPIRATION_TIME;
   mMetaHdr.mKeySize = mKey.Length();
 
@@ -832,7 +841,6 @@ CacheFileMetadata::ParseMetadata(uint32_t aMetaOffset, uint32_t aBufOffset,
   }
 
 
-  mMetaHdr.mFetchCount++;
   MarkDirty();
 
   mElementsSize = metaposOffset - elementsOffset;

netwerk/cache2/CacheFileMetadata.h

@@ -138,16 +138,18 @@ public:
 
   nsresult SetExpirationTime(uint32_t aExpirationTime);
   nsresult GetExpirationTime(uint32_t *_retval);
-  nsresult SetLastModified(uint32_t aLastModified);
-  nsresult GetLastModified(uint32_t *_retval);
   nsresult SetFrecency(uint32_t aFrecency);
   nsresult GetFrecency(uint32_t *_retval);
+  nsresult GetLastModified(uint32_t *_retval);
   nsresult GetLastFetched(uint32_t *_retval);
   nsresult GetFetchCount(uint32_t *_retval);
+  // Called by upper layers to indicate the entry this metadata belongs
+  // with has been fetched, i.e. delivered to the consumer.
+  nsresult OnFetched();
 
   int64_t  Offset() { return mOffset; }
   uint32_t ElementsSize() { return mElementsSize; }
-  void     MarkDirty() { mIsDirty = true; }
+  void     MarkDirty(bool aUpdateLastModified = true);
   bool     IsDirty() { return mIsDirty; }
   uint32_t MemoryUsage() { return sizeof(CacheFileMetadata) + mHashArraySize + mBufSize; }
 

netwerk/test/unit/test_cache2-28-last-access-attrs.js

@@ -0,0 +1,39 @@
+function run_test()
+{
+  do_get_profile();
+  function NowSeconds() {
+    return parseInt((new Date()).getTime() / 1000);
+  }
+  function do_check_time(t, min, max) {
+    do_check_true(t >= min);
+    do_check_true(t <= max);
+  }
+
+  var timeStart = NowSeconds();
+
+  asyncOpenCacheEntry("http://t/", "disk", Ci.nsICacheStorage.OPEN_NORMALLY, null,
+    new OpenCallback(NEW, "m", "d", function(entry) {
+
+      var firstOpen = NowSeconds();
+      do_check_eq(entry.fetchCount, 1);
+      do_check_time(entry.lastFetched, timeStart, firstOpen);
+      do_check_time(entry.lastModified, timeStart, firstOpen);
+
+      do_timeout(2000, () => {
+        asyncOpenCacheEntry("http://t/", "disk", Ci.nsICacheStorage.OPEN_NORMALLY, null,
+          new OpenCallback(NORMAL, "m", "d", function(entry) {
+
+            var secondOpen = NowSeconds();
+            do_check_eq(entry.fetchCount, 2);
+            do_check_time(entry.lastFetched, firstOpen, secondOpen);
+            do_check_time(entry.lastModified, timeStart, firstOpen);
+
+            finish_cache2_test();
+          })
+        );
+      })
+    })
+  );
+
+  do_test_pending();
+}

netwerk/test/unit/xpcshell.ini

@@ -59,6 +59,7 @@ support-files =
 # GC, that this patch is dependent on, doesn't work well on Android.
 skip-if = os == "android"
 [test_cache2-27-force-valid-for.js]
+[test_cache2-28-last-access-attrs.js]
 [test_304_responses.js]
 [test_cacheForOfflineUse_no-store.js]
 [test_307_redirect.js]