Bug 1000338 - nsICacheEntry.lastModified not properly implemented. r=michal, a=sledru
authorHonza Bambas <honzab.moz@firemni.cz>
Tue, 16 Sep 2014 15:51:50 +0200
changeset 216790 b88069789828
parent 216789 f5ba94d7170d
child 216791 2dce6525ddfe
push id3912
push userryanvm@gmail.com
push date2014-09-18 15:37 +0000
treeherdermozilla-beta@2dce6525ddfe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmichal, sledru
bugs1000338
milestone33.0
Bug 1000338 - nsICacheEntry.lastModified not properly implemented. r=michal, a=sledru
netwerk/cache2/CacheEntry.cpp
netwerk/cache2/CacheFile.cpp
netwerk/cache2/CacheFile.h
netwerk/cache2/CacheFileMetadata.cpp
netwerk/cache2/CacheFileMetadata.h
netwerk/test/unit/test_cache2-28-last-access-attrs.js
netwerk/test/unit/xpcshell.ini
--- a/netwerk/cache2/CacheEntry.cpp
+++ b/netwerk/cache2/CacheEntry.cpp
@@ -754,16 +754,21 @@ void CacheEntry::InvokeAvailableCallback
     nsRefPtr<AvailableCallbackRunnable> event =
       new AvailableCallbackRunnable(this, aCallback);
 
     rv = aCallback.mTargetThread->Dispatch(event, nsIEventTarget::DISPATCH_NORMAL);
     LOG(("  redispatched, (rv = 0x%08x)", rv));
     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(
       nullptr, false, nullptr, NS_ERROR_CACHE_KEY_NOT_FOUND);
     return;
   }
 
   if (state == READY) {
--- a/netwerk/cache2/CacheFile.cpp
+++ b/netwerk/cache2/CacheFile.cpp
@@ -919,37 +919,16 @@ CacheFile::GetExpirationTime(uint32_t *_
   CacheFileAutoLock lock(this);
   MOZ_ASSERT(mMetadata);
   NS_ENSURE_TRUE(mMetadata, NS_ERROR_UNEXPECTED);
 
   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)
 {
   CacheFileAutoLock lock(this);
   MOZ_ASSERT(mMetadata);
   NS_ENSURE_TRUE(mMetadata, NS_ERROR_UNEXPECTED);
 
   PostWriteTimer();
 
@@ -965,16 +944,26 @@ CacheFile::GetFrecency(uint32_t *_retval
   CacheFileAutoLock lock(this);
   MOZ_ASSERT(mMetadata);
   NS_ENSURE_TRUE(mMetadata, NS_ERROR_UNEXPECTED);
 
   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)
 {
   CacheFileAutoLock lock(this);
   MOZ_ASSERT(mMetadata);
   NS_ENSURE_TRUE(mMetadata, NS_ERROR_UNEXPECTED);
 
   return mMetadata->GetLastFetched(_retval);
 }
@@ -984,16 +973,28 @@ CacheFile::GetFetchCount(uint32_t *_retv
 {
   CacheFileAutoLock lock(this);
   MOZ_ASSERT(mMetadata);
   NS_ENSURE_TRUE(mMetadata, NS_ERROR_UNEXPECTED);
 
   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()
 {
   mLock.Lock();
 }
 
 void
 CacheFile::Unlock()
--- a/netwerk/cache2/CacheFile.h
+++ b/netwerk/cache2/CacheFile.h
@@ -85,22 +85,24 @@ public:
 
   // metadata forwarders
   nsresult GetElement(const char *aKey, char **_retval);
   nsresult SetElement(const char *aKey, const char *aValue);
   nsresult VisitMetaData(nsICacheEntryMetaDataVisitor *aVisitor);
   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; }
   bool IsDoomed();
   bool IsWriteInProgress();
 
   // Memory reporting
   size_t SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf) const;
--- a/netwerk/cache2/CacheFileMetadata.cpp
+++ b/netwerk/cache2/CacheFileMetadata.cpp
@@ -22,16 +22,18 @@
 namespace mozilla {
 namespace net {
 
 #define kMinMetadataRead 1024  // TODO find optimal value from telemetry
 #define kAlignSize       4096
 
 #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)
   : CacheMemoryConsumer(NORMAL)
   , mHandle(aHandle)
   , mHashArray(nullptr)
   , mHashArraySize(0)
   , mHashCount(0)
@@ -77,17 +79,16 @@ CacheFileMetadata::CacheFileMetadata(boo
 {
   LOG(("CacheFileMetadata::CacheFileMetadata() [this=%p, key=%s]",
        this, PromiseFlatCString(aKey).get()));
 
   MOZ_COUNT_CTOR(CacheFileMetadata);
   memset(&mMetaHdr, 0, sizeof(CacheFileMetadataHeader));
   mMetaHdr.mVersion = kCacheEntryVersion;
   mMetaHdr.mExpirationTime = nsICacheEntry::NO_EXPIRATION_TIME;
-  mMetaHdr.mFetchCount = 1;
   mKey = aKey;
   mMetaHdr.mKeySize = mKey.Length();
 
   DebugOnly<nsresult> rv;
   rv = ParseKey(aKey);
   MOZ_ASSERT(NS_SUCCEEDED(rv));
 }
 
@@ -499,79 +500,87 @@ CacheFileMetadata::SetHash(uint32_t aInd
 }
 
 nsresult
 CacheFileMetadata::SetExpirationTime(uint32_t aExpirationTime)
 {
   LOG(("CacheFileMetadata::SetExpirationTime() [this=%p, expirationTime=%d]",
        this, aExpirationTime));
 
-  MarkDirty();
+  MarkDirty(false);
   mMetaHdr.mExpirationTime = aExpirationTime;
   return NS_OK;
 }
 
 nsresult
 CacheFileMetadata::GetExpirationTime(uint32_t *_retval)
 {
   *_retval = mMetaHdr.mExpirationTime;
   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;
 }
 
 nsresult
 CacheFileMetadata::GetFrecency(uint32_t *_retval)
 {
   *_retval = mMetaHdr.mFrecency;
   return NS_OK;
 }
 
 nsresult
+CacheFileMetadata::GetLastModified(uint32_t *_retval)
+{
+  *_retval = mMetaHdr.mLastModified;
+  return NS_OK;
+}
+
+nsresult
 CacheFileMetadata::GetLastFetched(uint32_t *_retval)
 {
   *_retval = mMetaHdr.mLastFetched;
   return NS_OK;
 }
 
 nsresult
 CacheFileMetadata::GetFetchCount(uint32_t *_retval)
 {
   *_retval = mMetaHdr.mFetchCount;
   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)
 {
   MOZ_CRASH("CacheFileMetadata::OnFileOpened should not be called!");
   return NS_ERROR_UNEXPECTED;
 }
 
 nsresult
 CacheFileMetadata::OnDataWritten(CacheFileHandle *aHandle, const char *aBuf,
@@ -716,17 +725,17 @@ CacheFileMetadata::InitEmptyMetadata()
 {
   if (mBuf) {
     free(mBuf);
     mBuf = nullptr;
     mBufSize = 0;
   }
   mOffset = 0;
   mMetaHdr.mVersion = kCacheEntryVersion;
-  mMetaHdr.mFetchCount = 1;
+  mMetaHdr.mFetchCount = 0;
   mMetaHdr.mExpirationTime = nsICacheEntry::NO_EXPIRATION_TIME;
   mMetaHdr.mKeySize = mKey.Length();
 
   DoMemoryReport(MemoryUsage());
 }
 
 nsresult
 CacheFileMetadata::ParseMetadata(uint32_t aMetaOffset, uint32_t aBufOffset,
@@ -827,17 +836,16 @@ CacheFileMetadata::ParseMetadata(uint32_
   mHashCount = hashCount;
   if (mHashArraySize) {
     mHashArray = static_cast<CacheHash::Hash16_t *>(
                    moz_xmalloc(mHashArraySize));
     memcpy(mHashArray, mBuf + hashesOffset, mHashArraySize);
   }
 
 
-  mMetaHdr.mFetchCount++;
   MarkDirty();
 
   mElementsSize = metaposOffset - elementsOffset;
   memmove(mBuf, mBuf + elementsOffset, mElementsSize);
   mOffset = aMetaOffset;
 
   // TODO: shrink memory if buffer is too big
 
--- a/netwerk/cache2/CacheFileMetadata.h
+++ b/netwerk/cache2/CacheFileMetadata.h
@@ -133,26 +133,28 @@ public:
   nsresult     SetElement(const char *aKey, const char *aValue);
   nsresult     Visit(nsICacheEntryMetaDataVisitor *aVisitor);
 
   CacheHash::Hash16_t GetHash(uint32_t aIndex);
   nsresult            SetHash(uint32_t aIndex, CacheHash::Hash16_t aHash);
 
   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; }
 
   NS_IMETHOD OnFileOpened(CacheFileHandle *aHandle, nsresult aResult);
   NS_IMETHOD OnDataWritten(CacheFileHandle *aHandle, const char *aBuf,
                            nsresult aResult);
   NS_IMETHOD OnDataRead(CacheFileHandle *aHandle, char *aBuf, nsresult aResult);
   NS_IMETHOD OnFileDoomed(CacheFileHandle *aHandle, nsresult aResult);
new file mode 100644
--- /dev/null
+++ b/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();
+}
--- a/netwerk/test/unit/xpcshell.ini
+++ b/netwerk/test/unit/xpcshell.ini
@@ -64,16 +64,19 @@ skip-if = os == "android"
 [test_cache2-23-read-over-chunk.js]
 [test_cache2-24-exists.js]
 # Bug 675039, comment 6: "The difference is that the memory cache is disabled in Armv6 builds."
 skip-if = os == "android"
 [test_cache2-25-chunk-memory-limit.js]
 [test_cache2-26-no-outputstream-open.js]
 # GC, that this patch is depenedent on, doesn't work well on Android."
 skip-if = os == "android"
+[test_cache2-28-last-access-attrs.js]
+# This test will be fixed in bug 1067931
+skip-if = true
 [test_cache2-28-concurrent_read_resumable_entry_size_zero.js]
 [test_cache2-29-concurrent_read_non-resumable_entry_size_zero.js]
 [test_partial_response_entry_size_smart_shrink.js]
 [test_304_responses.js]
 # Bug 675039: test hangs on Android-armv6 
 skip-if = os == "android"
 [test_cacheForOfflineUse_no-store.js]
 [test_307_redirect.js]