Bug 1367155 - Handle async-closed FileBlockCached::mFD when reading/writing - r?cpearce draft
authorGerald Squelart <gsquelart@mozilla.com>
Wed, 24 May 2017 13:21:28 +1200
changeset 584222 ce04eb3d16911ff814a8600b38d0dff1f9e32038
parent 584216 f81bcc23d37d7bec48f08b19a9327e93c54d37b5
child 630305 e1d20c0dfd869665ebd559adac456d2932f05580
push id60657
push usergsquelart@mozilla.com
push dateThu, 25 May 2017 02:14:02 +0000
reviewerscpearce
bugs1367155
milestone55.0a1
Bug 1367155 - Handle async-closed FileBlockCached::mFD when reading/writing - r?cpearce Also, close the FD if FileBlockCache::Closed() was called between Init() and SetCacheFile(). MozReview-Commit-ID: 5p8NslMow7D
dom/media/FileBlockCache.cpp
--- a/dom/media/FileBlockCache.cpp
+++ b/dom/media/FileBlockCache.cpp
@@ -15,43 +15,59 @@
 
 namespace mozilla {
 
 #undef LOG
 LazyLogModule gFileBlockCacheLog("FileBlockCache");
 #define LOG(x, ...) MOZ_LOG(gFileBlockCacheLog, LogLevel::Debug, \
   ("%p " x, this, ##__VA_ARGS__))
 
+static void
+CloseFD(PRFileDesc* aFD)
+{
+  PRStatus prrc;
+  prrc = PR_Close(aFD);
+  if (prrc != PR_SUCCESS) {
+    NS_WARNING("PR_Close() failed.");
+  }
+}
+
 void
 FileBlockCache::SetCacheFile(PRFileDesc* aFD)
 {
   LOG("SetFD(aFD=%p) mIsOpen=%d", aFD, mIsOpen);
 
   if (!aFD) {
     // Failed to get a temporary file. Shutdown.
     Close();
     return;
   }
   {
     MutexAutoLock lock(mFileMutex);
     mFD = aFD;
   }
   {
     MutexAutoLock lock(mDataMutex);
-    if (!mIsOpen) {
-      // We've been closed while waiting for the file descriptor. Bail out.
-      // Rely on the destructor to close the file descriptor.
+    if (mIsOpen) {
+      // Still open, complete the initialization.
+      mInitialized = true;
+      if (mIsWriteScheduled) {
+        // A write was scheduled while waiting for FD. We need to run/dispatch a
+        // task to service the request.
+        mThread->Dispatch(this, NS_DISPATCH_NORMAL);
+      }
       return;
     }
-    mInitialized = true;
-    if (mIsWriteScheduled) {
-      // A write was scheduled while waiting for FD. We need to run/dispatch a
-      // task to service the request.
-      mThread->Dispatch(this, NS_DISPATCH_NORMAL);
-    }
+  }
+  // We've been closed while waiting for the file descriptor.
+  // Close the file descriptor we've just received, if still there.
+  MutexAutoLock lock(mFileMutex);
+  if (mFD) {
+    CloseFD(mFD);
+    mFD = nullptr;
   }
 }
 
 nsresult
 FileBlockCache::Init()
 {
   LOG("Init()");
 
@@ -130,21 +146,17 @@ void FileBlockCache::Close()
     mFD = nullptr;
   }
 
   // Let the thread close the FD, and then trigger its own shutdown.
   // Note that mThread is now empty, so no other task will be posted there.
   // Also mThread and mFD are empty and therefore can be reused immediately.
   nsresult rv = thread->Dispatch(NS_NewRunnableFunction([thread, fd] {
     if (fd) {
-      PRStatus prrc;
-      prrc = PR_Close(fd);
-      if (prrc != PR_SUCCESS) {
-        NS_WARNING("PR_Close() failed.");
-      }
+      CloseFD(fd);
     }
     // We must shut down the thread in another runnable. This is called
     // while we're shutting down the media cache, and nsIThread::Shutdown()
     // can cause events to run before it completes, which could end up
     // opening more streams, while the media cache is shutting down and
     // releasing memory etc!
     nsCOMPtr<nsIRunnable> event = new ShutdownThreadEvent(thread);
     SystemGroup::Dispatch(
@@ -284,17 +296,16 @@ nsresult FileBlockCache::MoveBlockInFile
 }
 
 nsresult FileBlockCache::Run()
 {
   NS_ASSERTION(!NS_IsMainThread(), "Don't call on main thread");
   MutexAutoLock mon(mDataMutex);
   NS_ASSERTION(!mChangeIndexList.empty(), "Only dispatch when there's work to do");
   NS_ASSERTION(mIsWriteScheduled, "Should report write running or scheduled.");
-  MOZ_ASSERT(mFD);
 
   LOG("Run() mFD=%p mIsOpen=%d", mFD, mIsOpen);
 
   while (!mChangeIndexList.empty()) {
     if (!mIsOpen) {
       // We've been closed, abort, discarding unwritten changes.
       mIsWriteScheduled = false;
       return NS_ERROR_FAILURE;
@@ -313,30 +324,35 @@ nsresult FileBlockCache::Run()
     // memory, rather than the not-yet up to date data written to file.
     // This also ensures we will insert a new index into mChangeIndexList
     // when this happens.
 
     // Hold a reference to the change, in case another change
     // overwrites the mBlockChanges entry for this block while we drop
     // mDataMutex to take mFileMutex.
     int32_t blockIndex = mChangeIndexList.front();
-    mChangeIndexList.pop_front();
     RefPtr<BlockChange> change = mBlockChanges[blockIndex];
     MOZ_ASSERT(change,
                "Change index list should only contain entries for blocks "
                "with changes");
     {
       MutexAutoUnlock unlock(mDataMutex);
       MutexAutoLock lock(mFileMutex);
+      if (!mFD) {
+        // We may be here if mFD has been reset because we're closing, so we
+        // don't care anymore about writes.
+        return NS_OK;
+      }
       if (change->IsWrite()) {
         WriteBlockToFile(blockIndex, change->mData.get());
       } else if (change->IsMove()) {
         MoveBlockInFile(change->mSourceBlockIndex, blockIndex);
       }
     }
+    mChangeIndexList.pop_front();
     // If a new change has not been made to the block while we dropped
     // mDataMutex, clear reference to the old change. Otherwise, the old
     // reference has been cleared already.
     if (mBlockChanges[blockIndex] == change) {
       mBlockChanges[blockIndex] = nullptr;
     }
   }
 
@@ -393,16 +409,20 @@ nsresult FileBlockCache::Read(int64_t aO
       }
       // Block has been written to file, either as the source block of a move,
       // or as a stable (all changes made) block. Read the data directly
       // from file.
       nsresult res;
       {
         MutexAutoUnlock unlock(mDataMutex);
         MutexAutoLock lock(mFileMutex);
+        if (!mFD) {
+          // Not initialized yet, or closed.
+          return NS_ERROR_FAILURE;
+        }
         res = ReadFromFile(BlockIndexToOffset(blockIndex) + start,
                            dst,
                            amount,
                            bytesRead);
       }
       NS_ENSURE_SUCCESS(res,res);
     }
     dst += bytesRead;