Bug 1253883 - Do not release mRWBuf until pending read/write operation finishes, r=honzab
authorMichal Novotny <michal.novotny@gmail.com>
Wed, 11 May 2016 10:48:28 +0200
changeset 296975 398b93819760f0be5904bd9ab225f7cc8924d08e
parent 296950 ad8395f134d7964187bc7491f41fbea18a133bd1
child 296976 5fe08cc926f133689004e6445e618c61c2383d01
push id30251
push usercbook@mozilla.com
push dateThu, 12 May 2016 09:54:48 +0000
treeherdermozilla-central@c3f5e6079284 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershonzab
bugs1253883
milestone49.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1253883 - Do not release mRWBuf until pending read/write operation finishes, r=honzab
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 = 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.