Bug 1335054 - Fix IndexedDB blob writing ownership race. r=janv a=gchang a=jcristau
authorAndrew Sutherland <asutherland@asutherland.org>
Tue, 07 Feb 2017 01:18:33 -0500
changeset 378315 0ab74d80ea74104c4cd114be32357d906b44d8e3
parent 378314 47234025410c725b1ec214b7d5491876065ba3de
child 378316 c73bb9843d79d3107031b40efe2731d3ae465f66
push id1419
push userjlund@mozilla.com
push dateMon, 10 Apr 2017 20:44:07 +0000
treeherdermozilla-release@5e6801b73ef6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjanv, gchang, jcristau
bugs1335054
milestone53.0a2
Bug 1335054 - Fix IndexedDB blob writing ownership race. r=janv a=gchang a=jcristau
dom/indexedDB/ActorsParent.cpp
--- a/dom/indexedDB/ActorsParent.cpp
+++ b/dom/indexedDB/ActorsParent.cpp
@@ -6666,41 +6666,38 @@ private:
  * ObjectStoreAddOrPutRequestOp::StoredFileInfo for a queued or active add/put
  * op is holding a reference to us.
  */
 class DatabaseFile final
   : public PBackgroundIDBDatabaseFileParent
 {
   friend class Database;
 
+  // mBlobImpl's ownership lifecycle:
+  // - Initialized on the background thread at creation time.  Then
+  //   responsibility is handed off to the connection thread.
+  // - Checked and used by the connection thread to generate a stream to write
+  //   the blob to disk by an add/put operation.
+  // - Cleared on the connection thread once the file has successfully been
+  //   written to disk.
   RefPtr<BlobImpl> mBlobImpl;
   RefPtr<FileInfo> mFileInfo;
 
 public:
   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(mozilla::dom::indexedDB::DatabaseFile);
 
   FileInfo*
   GetFileInfo() const
   {
     AssertIsOnBackgroundThread();
 
     return mFileInfo;
   }
 
   /**
-   * Is there a blob available to provide an input stream?  This may be called
-   * on any thread under current lifecycle constraints.
-   */
-  bool
-  HasBlobImpl() const
-  {
-    return (bool)mBlobImpl;
-  }
-
-  /**
    * If mBlobImpl is non-null (implying the contents of this file have not yet
    * been written to disk), then return an input stream that is guaranteed to
    * block on reads and never return NS_BASE_STREAM_WOULD_BLOCK.  If mBlobImpl
    * is null (because the contents have been written to disk), returns null.
    *
    * Because this method does I/O, it should only be called on a database I/O
    * thread, not on PBackground.  Note that we actually open the stream on this
    * thread, where previously it was opened on PBackground.  This is safe and
@@ -6729,20 +6726,27 @@ public:
    *   writing), they will be sent up from the child via PSendStream in chunks
    *   to a non-blocking pipe that will return NS_BASE_STREAM_WOULD_BLOCK.
    * - Other Blob types could potentially be non-blocking.  (We're not making
    *   any assumptions.)
    */
   already_AddRefed<nsIInputStream>
   GetBlockingInputStream(ErrorResult &rv) const;
 
-  void
-  ClearInputStream()
-  {
-    AssertIsOnBackgroundThread();
+  /**
+   * To be called upon successful copying of the stream GetBlockingInputStream()
+   * returned so that we won't try and redundantly write the file to disk in the
+   * future.  This is a separate step from GetBlockingInputStream() because
+   * the write could fail due to quota errors that happen now but that might
+   * not happen in a future attempt.
+   */
+  void
+  WriteSucceededClearBlobImpl()
+  {
+    MOZ_ASSERT(!IsOnBackgroundThread());
 
     mBlobImpl = nullptr;
   }
 
 private:
   // Called when sending to the child.
   explicit DatabaseFile(FileInfo* aFileInfo)
     : mFileInfo(aFileInfo)
@@ -6768,18 +6772,18 @@ private:
   {
     AssertIsOnBackgroundThread();
   }
 };
 
 already_AddRefed<nsIInputStream>
 DatabaseFile::GetBlockingInputStream(ErrorResult &rv) const
 {
-  // We should only be called from a database I/O thread, not the background
-  // thread.  The background thread should call HasBlobImpl().
+  // We should only be called from our DB connection thread, not the background
+  // thread.
   MOZ_ASSERT(!IsOnBackgroundThread());
 
   if (!mBlobImpl) {
     return nullptr;
   }
 
   nsCOMPtr<nsIInputStream> inputStream;
   mBlobImpl->GetInternalStream(getter_AddRefs(inputStream), rv);
@@ -8315,18 +8319,16 @@ class ObjectStoreAddOrPutRequestOp final
   Maybe<UniqueIndexTable> mUniqueIndexTable;
 
   // This must be non-const so that we can update the mNextAutoIncrementId field
   // if we are modifying an autoIncrement objectStore.
   RefPtr<FullObjectStoreMetadata> mMetadata;
 
   FallibleTArray<StoredFileInfo> mStoredFileInfos;
 
-  RefPtr<FileManager> mFileManager;
-
   Key mResponse;
   const nsCString mGroup;
   const nsCString mOrigin;
   const PersistenceType mPersistenceType;
   const bool mOverwrite;
   bool mObjectStoreMayHaveIndexes;
   bool mDataOverThreshold;
 
@@ -8356,21 +8358,19 @@ private:
 struct ObjectStoreAddOrPutRequestOp::StoredFileInfo final
 {
   RefPtr<DatabaseFile> mFileActor;
   RefPtr<FileInfo> mFileInfo;
   // A non-Blob-backed inputstream to write to disk.  If null, mFileActor may
   // still have a stream for us to write.
   nsCOMPtr<nsIInputStream> mInputStream;
   StructuredCloneFile::FileType mType;
-  bool mCopiedSuccessfully;
 
   StoredFileInfo()
     : mType(StructuredCloneFile::eBlob)
-    , mCopiedSuccessfully(false)
   {
     AssertIsOnBackgroundThread();
 
     MOZ_COUNT_CTOR(ObjectStoreAddOrPutRequestOp::StoredFileInfo);
   }
 
   ~StoredFileInfo()
   {
@@ -26150,20 +26150,16 @@ ObjectStoreAddOrPutRequestOp::Init(Trans
 
   if (!fileAddInfos.IsEmpty()) {
     const uint32_t count = fileAddInfos.Length();
 
     if (NS_WARN_IF(!mStoredFileInfos.SetCapacity(count, fallible))) {
       return false;
     }
 
-    RefPtr<FileManager> fileManager =
-      aTransaction->GetDatabase()->GetFileManager();
-    MOZ_ASSERT(fileManager);
-
     for (uint32_t index = 0; index < count; index++) {
       const FileAddInfo& fileAddInfo = fileAddInfos[index];
 
       MOZ_ASSERT(fileAddInfo.type() == StructuredCloneFile::eBlob ||
                  fileAddInfo.type() == StructuredCloneFile::eMutableFile ||
                  fileAddInfo.type() == StructuredCloneFile::eWasmBytecode ||
                  fileAddInfo.type() == StructuredCloneFile::eWasmCompiled);
 
@@ -26180,20 +26176,16 @@ ObjectStoreAddOrPutRequestOp::Init(Trans
           storedFileInfo->mFileActor =
             static_cast<DatabaseFile*>(
               file.get_PBackgroundIDBDatabaseFileParent());
           MOZ_ASSERT(storedFileInfo->mFileActor);
 
           storedFileInfo->mFileInfo = storedFileInfo->mFileActor->GetFileInfo();
           MOZ_ASSERT(storedFileInfo->mFileInfo);
 
-          if (storedFileInfo->mFileActor->HasBlobImpl() && !mFileManager) {
-            mFileManager = fileManager;
-          }
-
           storedFileInfo->mType = StructuredCloneFile::eBlob;
           break;
         }
 
         case StructuredCloneFile::eMutableFile: {
           MOZ_ASSERT(file.type() ==
                        DatabaseOrMutableFile::TPBackgroundMutableFileParent);
 
@@ -26217,57 +26209,51 @@ ObjectStoreAddOrPutRequestOp::Init(Trans
           storedFileInfo->mFileActor =
             static_cast<DatabaseFile*>(
               file.get_PBackgroundIDBDatabaseFileParent());
           MOZ_ASSERT(storedFileInfo->mFileActor);
 
           storedFileInfo->mFileInfo = storedFileInfo->mFileActor->GetFileInfo();
           MOZ_ASSERT(storedFileInfo->mFileInfo);
 
-          if (storedFileInfo->mFileActor->HasBlobImpl() && !mFileManager) {
-            mFileManager = fileManager;
-          }
-
           storedFileInfo->mType = fileAddInfo.type();
           break;
         }
 
         default:
           MOZ_CRASH("Should never get here!");
       }
     }
   }
 
   if (mDataOverThreshold) {
     StoredFileInfo* storedFileInfo = mStoredFileInfos.AppendElement(fallible);
     MOZ_ASSERT(storedFileInfo);
 
-    if (!mFileManager) {
-      mFileManager = aTransaction->GetDatabase()->GetFileManager();
-      MOZ_ASSERT(mFileManager);
-    }
-
-    storedFileInfo->mFileInfo = mFileManager->GetNewFileInfo();
+    RefPtr<FileManager> fileManager =
+      aTransaction->GetDatabase()->GetFileManager();
+    MOZ_ASSERT(fileManager);
+
+    storedFileInfo->mFileInfo = fileManager->GetNewFileInfo();
 
     storedFileInfo->mInputStream =
       new SCInputStream(mParams.cloneInfo().data().data);
 
     storedFileInfo->mType = StructuredCloneFile::eStructuredClone;
   }
 
   return true;
 }
 
 nsresult
 ObjectStoreAddOrPutRequestOp::DoDatabaseWork(DatabaseConnection* aConnection)
 {
   MOZ_ASSERT(aConnection);
   aConnection->AssertIsOnConnectionThread();
   MOZ_ASSERT(aConnection->GetStorageConnection());
-  MOZ_ASSERT_IF(mFileManager, !mStoredFileInfos.IsEmpty());
 
   PROFILER_LABEL("IndexedDB",
                  "ObjectStoreAddOrPutRequestOp::DoDatabaseWork",
                  js::ProfileEntry::Category::STORAGE);
 
   if (NS_WARN_IF(IndexedDatabaseManager::InLowDiskSpaceMode())) {
     return NS_ERROR_DOM_INDEXEDDB_QUOTA_ERR;
   }
@@ -26434,44 +26420,34 @@ ObjectStoreAddOrPutRequestOp::DoDatabase
 
     rv = stmt->BindAdoptedBlobByName(NS_LITERAL_CSTRING("data"), dataBuffer,
                                      dataBufferLength);
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return rv;
     }
   }
 
-  Maybe<FileHelper> fileHelper;
-
-  if (mFileManager) {
-    fileHelper.emplace(mFileManager);
-
-    rv = fileHelper->Init();
-    if (NS_WARN_IF(NS_FAILED(rv))) {
-      IDB_REPORT_INTERNAL_ERR();
-      return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
-    }
-  }
-
   if (!mStoredFileInfos.IsEmpty()) {
+    // Moved outside the loop to allow it to be cached when demanded by the
+    // first write.  (We may have mStoredFileInfos without any required writes.)
+    Maybe<FileHelper> fileHelper;
     nsAutoString fileIds;
 
     for (uint32_t count = mStoredFileInfos.Length(), index = 0;
          index < count;
          index++) {
       StoredFileInfo& storedFileInfo = mStoredFileInfos[index];
       MOZ_ASSERT(storedFileInfo.mFileInfo);
 
       // If there is a StoredFileInfo, then one of the following is true:
       // - This was an overflow structured clone and storedFileInfo.mInputStream
       //   MUST be non-null.
       // - This is a reference to a Blob that may or may not have already been
       //   written to disk.  storedFileInfo.mFileActor MUST be non-null, but
-      //   its HasBlobImpl() may return false and so GetBlockingInputStream may
-      //   return null (so don't assert on them).
+      //   its GetBlockingInputStream may return null (so don't assert on them).
       // - It's a mutable file.  No writing will be performed.
       MOZ_ASSERT(storedFileInfo.mInputStream || storedFileInfo.mFileActor ||
                  storedFileInfo.mType == StructuredCloneFile::eMutableFile);
 
       nsCOMPtr<nsIInputStream> inputStream;
       // Check for an explicit stream, like a structured clone stream.
       storedFileInfo.mInputStream.swap(inputStream);
       // Check for a blob-backed stream otherwise.
@@ -26480,16 +26456,29 @@ ObjectStoreAddOrPutRequestOp::DoDatabase
         inputStream =
           storedFileInfo.mFileActor->GetBlockingInputStream(streamRv);
         if (NS_WARN_IF(streamRv.Failed())) {
           return streamRv.StealNSResult();
         }
       }
 
       if (inputStream) {
+        if (fileHelper.isNothing()) {
+          RefPtr<FileManager> fileManager =
+            Transaction()->GetDatabase()->GetFileManager();
+          MOZ_ASSERT(fileManager);
+
+          fileHelper.emplace(fileManager);
+          rv = fileHelper->Init();
+          if (NS_WARN_IF(NS_FAILED(rv))) {
+            IDB_REPORT_INTERNAL_ERR();
+            return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
+          }
+        }
+
         RefPtr<FileInfo>& fileInfo = storedFileInfo.mFileInfo;
 
         nsCOMPtr<nsIFile> file = fileHelper->GetFile(fileInfo);
         if (NS_WARN_IF(!file)) {
           IDB_REPORT_INTERNAL_ERR();
           return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
         }
 
@@ -26516,17 +26505,19 @@ ObjectStoreAddOrPutRequestOp::DoDatabase
           // Try to remove the file if the copy failed.
           nsresult rv2 = fileHelper->RemoveFile(file, journalFile);
           if (NS_WARN_IF(NS_FAILED(rv2))) {
             return rv;
           }
           return rv;
         }
 
-        storedFileInfo.mCopiedSuccessfully = true;
+        if (storedFileInfo.mFileActor) {
+          storedFileInfo.mFileActor->WriteSucceededClearBlobImpl();
+        }
       }
 
       if (index) {
         fileIds.Append(' ');
       }
       storedFileInfo.Serialize(fileIds);
     }
 
@@ -26600,33 +26591,17 @@ ObjectStoreAddOrPutRequestOp::GetRespons
   }
 }
 
 void
 ObjectStoreAddOrPutRequestOp::Cleanup()
 {
   AssertIsOnOwningThread();
 
-  if (!mStoredFileInfos.IsEmpty()) {
-    for (uint32_t count = mStoredFileInfos.Length(), index = 0;
-         index < count;
-         index++) {
-      StoredFileInfo& storedFileInfo = mStoredFileInfos[index];
-
-      MOZ_ASSERT_IF(storedFileInfo.mType == StructuredCloneFile::eMutableFile,
-                    !storedFileInfo.mCopiedSuccessfully);
-
-      RefPtr<DatabaseFile>& fileActor = storedFileInfo.mFileActor;
-      if (fileActor && storedFileInfo.mCopiedSuccessfully) {
-        fileActor->ClearInputStream();
-      }
-    }
-
-    mStoredFileInfos.Clear();
-  }
+  mStoredFileInfos.Clear();
 
   NormalTransactionOp::Cleanup();
 }
 
 NS_IMPL_ISUPPORTS(ObjectStoreAddOrPutRequestOp::SCInputStream, nsIInputStream)
 
 NS_IMETHODIMP
 ObjectStoreAddOrPutRequestOp::