Bug 1253883 - Do not release mRWBuf until pending read/write operation finishes, r=honzab, a=sylvestre
authorMichal Novotny <michal.novotny@gmail.com>
Wed, 11 May 2016 10:48:28 +0200
changeset 333136 d5c83c0c2aff50aba6a50468f4d776f17b3a8933
parent 333135 ed1c3aae460813241ce2c3480df30fae2268ab1f
child 333137 71b97e1011994fb5f1d60add58e3326e256d15a5
push id6048
push userkmoir@mozilla.com
push dateMon, 06 Jun 2016 19:02:08 +0000
treeherdermozilla-beta@46d72a56c57d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershonzab, sylvestre
bugs1253883
milestone48.0a2
Bug 1253883 - Do not release mRWBuf until pending read/write operation finishes, r=honzab, a=sylvestre
netwerk/cache2/CacheIndex.cpp
netwerk/cache2/CacheIndex.h
--- a/netwerk/cache2/CacheIndex.cpp
+++ b/netwerk/cache2/CacheIndex.cpp
@@ -245,28 +245,30 @@ CacheIndex::CacheIndex()
   , mDontMarkIndexClean(false)
   , mIndexTimeStamp(0)
   , mUpdateEventPending(false)
   , mSkipEntries(0)
   , mProcessEntries(0)
   , mRWBuf(nullptr)
   , mRWBufSize(0)
   , mRWBufPos(0)
+  , mRWPending(false)
   , mJournalReadSuccessfully(false)
   , mFrecencyArraySorted(false)
   , mAsyncGetDiskConsumptionBlocked(false)
 {
   sLock.AssertCurrentThreadOwns();
   LOG(("CacheIndex::CacheIndex [this=%p]", this));
   MOZ_COUNT_CTOR(CacheIndex);
   MOZ_ASSERT(!gInstance, "multiple CacheIndex instances!");
 }
 
 CacheIndex::~CacheIndex()
 {
+  sLock.AssertCurrentThreadOwns();
   LOG(("CacheIndex::~CacheIndex [this=%p]", this));
   MOZ_COUNT_DTOR(CacheIndex);
 
   ReleaseBuffer();
 }
 
 // static
 nsresult
@@ -343,25 +345,22 @@ CacheIndex::PreShutdown()
 
   if (index->mState == READY) {
     return NS_OK; // nothing to do
   }
 
   nsCOMPtr<nsIRunnable> event;
   event = NS_NewRunnableMethod(index, &CacheIndex::PreShutdownInternal);
 
-  RefPtr<CacheIOThread> ioThread = CacheFileIOManager::IOThread();
-  MOZ_ASSERT(ioThread);
-
-  // Executing PreShutdownInternal() on WRITE level ensures that read/write
-  // events holding pointer to mRWBuf will be executed before we release the
-  // buffer by calling FinishRead()/FinishWrite() in PreShutdownInternal(), but
-  // it will be executed before any queued event on INDEX level. That's OK since
-  // we don't want to wait until updating of the index finishes.
-  rv = ioThread->Dispatch(event, CacheIOThread::WRITE);
+  nsCOMPtr<nsIEventTarget> ioTarget = CacheFileIOManager::IOTarget();
+  MOZ_ASSERT(ioTarget);
+
+  // PreShutdownInternal() will be executed before any queued event on INDEX
+  // level. That's OK since we don't want to wait for any operation in progess.
+  rv = ioTarget->Dispatch(event, nsIEventTarget::DISPATCH_NORMAL);
   if (NS_FAILED(rv)) {
     NS_WARNING("CacheIndex::PreShutdown() - Can't dispatch event");
     LOG(("CacheIndex::PreShutdown() - Can't dispatch event" ));
     return rv;
   }
 
   return NS_OK;
 }
@@ -1562,17 +1561,17 @@ CacheIndex::ProcessPendingOperations()
   MOZ_ASSERT(mPendingUpdates.Count() == 0);
 
   EnsureCorrectStats();
 }
 
 bool
 CacheIndex::WriteIndexToDiskIfNeeded()
 {
-  if (mState != READY || mShuttingDown) {
+  if (mState != READY || mShuttingDown || mRWPending) {
     return false;
   }
 
   if (!mLastDumpTime.IsNull() &&
       (TimeStamp::NowLoRes() - mLastDumpTime).ToMilliseconds() <
       kMinDumpInterval) {
     return false;
   }
@@ -1592,16 +1591,17 @@ CacheIndex::WriteIndexToDisk()
   mIndexStats.Log();
 
   nsresult rv;
 
   sLock.AssertCurrentThreadOwns();
   MOZ_ASSERT(mState == READY);
   MOZ_ASSERT(!mRWBuf);
   MOZ_ASSERT(!mRWHash);
+  MOZ_ASSERT(!mRWPending);
 
   ChangeState(WRITING);
 
   mProcessEntries = mIndexStats.ActiveEntriesCount();
 
   mIndexFileOpener = new FileOpenHelper(this);
   rv = CacheFileIOManager::OpenFile(NS_LITERAL_CSTRING(TEMP_INDEX_NAME),
                                     CacheFileIOManager::SPECIAL_FILE |
@@ -1632,16 +1632,17 @@ void
 CacheIndex::WriteRecords()
 {
   LOG(("CacheIndex::WriteRecords()"));
 
   nsresult rv;
 
   sLock.AssertCurrentThreadOwns();
   MOZ_ASSERT(mState == WRITING);
+  MOZ_ASSERT(!mRWPending);
 
   int64_t fileOffset;
 
   if (mSkipEntries) {
     MOZ_ASSERT(mRWBufPos == 0);
     fileOffset = sizeof(CacheIndexHeader);
     fileOffset += sizeof(CacheIndexRecord) * mSkipEntries;
   } else {
@@ -1708,30 +1709,36 @@ CacheIndex::WriteRecords()
   }
 
   rv = CacheFileIOManager::Write(mIndexHandle, fileOffset, mRWBuf, mRWBufPos,
                                  mSkipEntries == mProcessEntries, false, this);
   if (NS_FAILED(rv)) {
     LOG(("CacheIndex::WriteRecords() - CacheFileIOManager::Write() failed "
          "synchronously [rv=0x%08x]", rv));
     FinishWrite(false);
+  } else {
+    mRWPending = true;
   }
 
   mRWBufPos = 0;
 }
 
 void
 CacheIndex::FinishWrite(bool aSucceeded)
 {
   LOG(("CacheIndex::FinishWrite() [succeeded=%d]", aSucceeded));
 
   MOZ_ASSERT((!aSucceeded && mState == SHUTDOWN) || mState == WRITING);
 
   sLock.AssertCurrentThreadOwns();
 
+  // If there is write operation pending we must be cancelling writing of the
+  // index when shutting down or removing the whole index.
+  MOZ_ASSERT(!mRWPending || (!aSucceeded && (mShuttingDown || mRemovingAll)));
+
   mIndexHandle = nullptr;
   mRWHash = nullptr;
   ReleaseBuffer();
 
   if (aSucceeded) {
     // Opening of the file must not be in progress if writing succeeded.
     MOZ_ASSERT(!mIndexFileOpener);
 
@@ -2054,16 +2061,17 @@ CacheIndex::StartReadingIndex()
   sLock.AssertCurrentThreadOwns();
 
   MOZ_ASSERT(mIndexHandle);
   MOZ_ASSERT(mState == READING);
   MOZ_ASSERT(!mIndexOnDiskIsValid);
   MOZ_ASSERT(!mDontMarkIndexClean);
   MOZ_ASSERT(!mJournalReadSuccessfully);
   MOZ_ASSERT(mIndexHandle->FileSize() >= 0);
+  MOZ_ASSERT(!mRWPending);
 
   int64_t entriesSize = mIndexHandle->FileSize() - sizeof(CacheIndexHeader) -
                         sizeof(CacheHash::Hash32_t);
 
   if (entriesSize < 0 || entriesSize % sizeof(CacheIndexRecord)) {
     LOG(("CacheIndex::StartReadingIndex() - Index is corrupted"));
     FinishRead(false);
     return;
@@ -2076,28 +2084,32 @@ CacheIndex::StartReadingIndex()
   mRWBufPos = std::min(mRWBufSize,
                        static_cast<uint32_t>(mIndexHandle->FileSize()));
 
   rv = CacheFileIOManager::Read(mIndexHandle, 0, mRWBuf, mRWBufPos, this);
   if (NS_FAILED(rv)) {
     LOG(("CacheIndex::StartReadingIndex() - CacheFileIOManager::Read() failed "
          "synchronously [rv=0x%08x]", rv));
     FinishRead(false);
+  } else {
+    mRWPending = true;
   }
 }
 
 void
 CacheIndex::ParseRecords()
 {
   LOG(("CacheIndex::ParseRecords()"));
 
   nsresult rv;
 
   sLock.AssertCurrentThreadOwns();
 
+  MOZ_ASSERT(!mRWPending);
+
   uint32_t entryCnt = (mIndexHandle->FileSize() - sizeof(CacheIndexHeader) -
                      sizeof(CacheHash::Hash32_t)) / sizeof(CacheIndexRecord);
   uint32_t pos = 0;
 
   if (!mSkipEntries) {
     CacheIndexHeader *hdr = reinterpret_cast<CacheIndexHeader *>(
                               moz_xmalloc(sizeof(CacheIndexHeader)));
     memcpy(hdr, mRWBuf, sizeof(CacheIndexHeader));
@@ -2204,32 +2216,35 @@ CacheIndex::ParseRecords()
 
   rv = CacheFileIOManager::Read(mIndexHandle, fileOffset, mRWBuf + pos, toRead,
                                 this);
   if (NS_FAILED(rv)) {
     LOG(("CacheIndex::ParseRecords() - CacheFileIOManager::Read() failed "
          "synchronously [rv=0x%08x]", rv));
     FinishRead(false);
     return;
+  } else {
+    mRWPending = true;
   }
 }
 
 void
 CacheIndex::StartReadingJournal()
 {
   LOG(("CacheIndex::StartReadingJournal()"));
 
   nsresult rv;
 
   sLock.AssertCurrentThreadOwns();
 
   MOZ_ASSERT(mJournalHandle);
   MOZ_ASSERT(mIndexOnDiskIsValid);
   MOZ_ASSERT(mTmpJournal.Count() == 0);
   MOZ_ASSERT(mJournalHandle->FileSize() >= 0);
+  MOZ_ASSERT(!mRWPending);
 
   int64_t entriesSize = mJournalHandle->FileSize() -
                         sizeof(CacheHash::Hash32_t);
 
   if (entriesSize < 0 || entriesSize % sizeof(CacheIndexRecord)) {
     LOG(("CacheIndex::StartReadingJournal() - Journal is corrupted"));
     FinishRead(false);
     return;
@@ -2241,28 +2256,32 @@ CacheIndex::StartReadingJournal()
   mRWBufPos = std::min(mRWBufSize,
                        static_cast<uint32_t>(mJournalHandle->FileSize()));
 
   rv = CacheFileIOManager::Read(mJournalHandle, 0, mRWBuf, mRWBufPos, this);
   if (NS_FAILED(rv)) {
     LOG(("CacheIndex::StartReadingJournal() - CacheFileIOManager::Read() failed"
          " synchronously [rv=0x%08x]", rv));
     FinishRead(false);
+  } else {
+    mRWPending = true;
   }
 }
 
 void
 CacheIndex::ParseJournal()
 {
   LOG(("CacheIndex::ParseJournal()"));
 
   nsresult rv;
 
   sLock.AssertCurrentThreadOwns();
 
+  MOZ_ASSERT(!mRWPending);
+
   uint32_t entryCnt = (mJournalHandle->FileSize() -
                        sizeof(CacheHash::Hash32_t)) / sizeof(CacheIndexRecord);
 
   uint32_t pos = 0;
 
   while (pos + sizeof(CacheIndexRecord) <= mRWBufPos &&
          mSkipEntries != entryCnt) {
     CacheIndexRecord *rec = reinterpret_cast<CacheIndexRecord *>(mRWBuf + pos);
@@ -2318,16 +2337,18 @@ CacheIndex::ParseJournal()
 
   rv = CacheFileIOManager::Read(mJournalHandle, fileOffset, mRWBuf + pos,
                                 toRead, this);
   if (NS_FAILED(rv)) {
     LOG(("CacheIndex::ParseJournal() - CacheFileIOManager::Read() failed "
          "synchronously [rv=0x%08x]", rv));
     FinishRead(false);
     return;
+  } else {
+    mRWPending = true;
   }
 }
 
 void
 CacheIndex::MergeJournal()
 {
   LOG(("CacheIndex::MergeJournal()"));
 
@@ -2402,16 +2423,20 @@ CacheIndex::FinishRead(bool aSucceeded)
   MOZ_ASSERT(
     // -> rebuild
     (!aSucceeded && !mIndexOnDiskIsValid && !mJournalReadSuccessfully) ||
     // -> update
     (!aSucceeded && mIndexOnDiskIsValid && !mJournalReadSuccessfully) ||
     // -> ready
     (aSucceeded && mIndexOnDiskIsValid && mJournalReadSuccessfully));
 
+  // If there is read operation pending we must be cancelling reading of the
+  // index when shutting down or removing the whole index.
+  MOZ_ASSERT(!mRWPending || (!aSucceeded && (mShuttingDown || mRemovingAll)));
+
   if (mState == SHUTDOWN) {
     RemoveFile(NS_LITERAL_CSTRING(TEMP_INDEX_NAME));
     RemoveFile(NS_LITERAL_CSTRING(JOURNAL_NAME));
   } else {
     if (mIndexHandle && !mIndexOnDiskIsValid) {
       CacheFileIOManager::DoomFile(mIndexHandle, nullptr);
     }
 
@@ -3163,20 +3188,24 @@ CacheIndex::AllocBuffer()
   }
 
   mRWBuf = static_cast<char *>(moz_xmalloc(mRWBufSize));
 }
 
 void
 CacheIndex::ReleaseBuffer()
 {
-  if (!mRWBuf) {
+  sLock.AssertCurrentThreadOwns();
+
+  if (!mRWBuf || mRWPending) {
     return;
   }
 
+  LOG(("CacheIndex::ReleaseBuffer() releasing buffer"));
+
   free(mRWBuf);
   mRWBuf = nullptr;
   mRWBufSize = 0;
   mRWBufPos = 0;
 }
 
 void
 CacheIndex::InsertRecordToFrecencyArray(CacheIndexRecord *aRecord)
@@ -3274,23 +3303,23 @@ CacheIndex::Run()
 
 nsresult
 CacheIndex::OnFileOpenedInternal(FileOpenHelper *aOpener,
                                  CacheFileHandle *aHandle, nsresult aResult)
 {
   LOG(("CacheIndex::OnFileOpenedInternal() [opener=%p, handle=%p, "
        "result=0x%08x]", aOpener, aHandle, aResult));
 
+  MOZ_ASSERT(CacheFileIOManager::IsOnIOThread());
+
   nsresult rv;
 
   sLock.AssertCurrentThreadOwns();
 
-  if (!IsIndexUsable()) {
-    return NS_ERROR_NOT_AVAILABLE;
-  }
+  MOZ_RELEASE_ASSERT(IsIndexUsable());
 
   if (mState == READY && mShuttingDown) {
     return NS_OK;
   }
 
   switch (mState) {
     case WRITING:
       MOZ_ASSERT(aOpener == mIndexFileOpener);
@@ -3384,35 +3413,33 @@ CacheIndex::OnFileOpened(CacheFileHandle
 
 nsresult
 CacheIndex::OnDataWritten(CacheFileHandle *aHandle, const char *aBuf,
                           nsresult aResult)
 {
   LOG(("CacheIndex::OnDataWritten() [handle=%p, result=0x%08x]", aHandle,
        aResult));
 
+  MOZ_ASSERT(CacheFileIOManager::IsOnIOThread());
+
   nsresult rv;
 
   StaticMutexAutoLock lock(sLock);
 
-  if (!IsIndexUsable()) {
-    return NS_ERROR_NOT_AVAILABLE;
-  }
+  MOZ_RELEASE_ASSERT(IsIndexUsable());
+  MOZ_RELEASE_ASSERT(mRWPending);
+  mRWPending = false;
 
   if (mState == READY && mShuttingDown) {
     return NS_OK;
   }
 
   switch (mState) {
     case WRITING:
-      if (mIndexHandle != aHandle) {
-        LOG(("CacheIndex::OnDataWritten() - ignoring notification since it "
-             "belongs to previously canceled operation [state=%d]", mState));
-        break;
-      }
+      MOZ_ASSERT(mIndexHandle == aHandle);
 
       if (NS_FAILED(aResult)) {
         FinishWrite(false);
       } else {
         if (mSkipEntries == mProcessEntries) {
           rv = CacheFileIOManager::RenameFile(mIndexHandle,
                                               NS_LITERAL_CSTRING(INDEX_NAME),
                                               this);
@@ -3425,32 +3452,35 @@ CacheIndex::OnDataWritten(CacheFileHandl
           WriteRecords();
         }
       }
       break;
     default:
       // Writing was canceled.
       LOG(("CacheIndex::OnDataWritten() - ignoring notification since the "
            "operation was previously canceled [state=%d]", mState));
+      ReleaseBuffer();
   }
 
   return NS_OK;
 }
 
 nsresult
 CacheIndex::OnDataRead(CacheFileHandle *aHandle, char *aBuf, nsresult aResult)
 {
   LOG(("CacheIndex::OnDataRead() [handle=%p, result=0x%08x]", aHandle,
        aResult));
 
+  MOZ_ASSERT(CacheFileIOManager::IsOnIOThread());
+
   StaticMutexAutoLock lock(sLock);
 
-  if (!IsIndexUsable()) {
-    return NS_ERROR_NOT_AVAILABLE;
-  }
+  MOZ_RELEASE_ASSERT(IsIndexUsable());
+  MOZ_RELEASE_ASSERT(mRWPending);
+  mRWPending = false;
 
   switch (mState) {
     case READING:
       MOZ_ASSERT(mIndexHandle == aHandle || mJournalHandle == aHandle);
 
       if (NS_FAILED(aResult)) {
         FinishRead(false);
       } else {
@@ -3460,16 +3490,17 @@ CacheIndex::OnDataRead(CacheFileHandle *
           ParseJournal();
         }
       }
       break;
     default:
       // Reading was canceled.
       LOG(("CacheIndex::OnDataRead() - ignoring notification since the "
            "operation was previously canceled [state=%d]", mState));
+      ReleaseBuffer();
   }
 
   return NS_OK;
 }
 
 nsresult
 CacheIndex::OnFileDoomed(CacheFileHandle *aHandle, nsresult aResult)
 {
@@ -3485,21 +3516,21 @@ CacheIndex::OnEOFSet(CacheFileHandle *aH
 }
 
 nsresult
 CacheIndex::OnFileRenamed(CacheFileHandle *aHandle, nsresult aResult)
 {
   LOG(("CacheIndex::OnFileRenamed() [handle=%p, result=0x%08x]", aHandle,
        aResult));
 
+  MOZ_ASSERT(CacheFileIOManager::IsOnIOThread());
+
   StaticMutexAutoLock lock(sLock);
 
-  if (!IsIndexUsable()) {
-    return NS_ERROR_NOT_AVAILABLE;
-  }
+  MOZ_RELEASE_ASSERT(IsIndexUsable());
 
   if (mState == READY && mShuttingDown) {
     return NS_OK;
   }
 
   switch (mState) {
     case WRITING:
       // This is a result of renaming the new index written to tmpfile to index
--- a/netwerk/cache2/CacheIndex.h
+++ b/netwerk/cache2/CacheIndex.h
@@ -986,16 +986,20 @@ private:
   // in hashtable that are initialized and are not marked as removed when writing
   // begins.
   uint32_t                  mProcessEntries;
   char                     *mRWBuf;
   uint32_t                  mRWBufSize;
   uint32_t                  mRWBufPos;
   RefPtr<CacheHash>         mRWHash;
 
+  // True if read or write operation is pending. It is used to ensure that
+  // mRWBuf is not freed until OnDataRead or OnDataWritten is called.
+  bool                      mRWPending;
+
   // Reading of journal succeeded if true.
   bool                      mJournalReadSuccessfully;
 
   // Handle used for writing and reading index file.
   RefPtr<CacheFileHandle> mIndexHandle;
   // Handle used for reading journal file.
   RefPtr<CacheFileHandle> mJournalHandle;
   // Used to check the existence of the file during reading process.