Changeset 201693 in webkit

Timestamp:

Jun 4, 2016 8:46:42 PM (8 years ago)

Author:

beidson@apple.com

Message:

Modern IDB: Crash seen in IDBConnectionProxy::putOrAdd on GuardMalloc bot
​https://bugs.webkit.org/show_bug.cgi?id=158124

Reviewed by Darin Adler.

No new tests (Covered by existing test configurations).

• Modules/indexeddb/IDBTransaction.cpp:

(WebCore::IDBTransaction::putOrAddOnServer):

• Modules/indexeddb/client/IDBConnectionProxy.cpp:

(WebCore::IDBClient::IDBConnectionProxy::putOrAdd):

• Modules/indexeddb/client/IDBConnectionProxy.h:

(WebCore::IDBClient::IDBConnectionProxy::callConnectionOnMainThread):

• bindings/js/SerializedScriptValue.cpp:

(WebCore::SerializedScriptValue::writeBlobsToDiskForIndexedDB):

• bindings/js/SerializedScriptValue.h:

• platform/network/BlobRegistry.h:
• platform/network/BlobRegistryImpl.cpp:

(WebCore::BlobRegistryImpl::writeBlobsToTemporaryFiles):

• platform/network/BlobRegistryImpl.h:

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• Modules/indexeddb/IDBTransaction.cpp (modified) (2 diffs)
• Modules/indexeddb/client/IDBConnectionProxy.cpp (modified) (1 diff)
• Modules/indexeddb/client/IDBConnectionProxy.h (modified) (2 diffs)
• bindings/js/SerializedScriptValue.cpp (modified) (1 diff)
• bindings/js/SerializedScriptValue.h (modified) (2 diffs)
• platform/network/BlobRegistry.h (modified) (2 diffs)
• platform/network/BlobRegistryImpl.cpp (modified) (2 diffs)
• platform/network/BlobRegistryImpl.h (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2016-06-04  Brady Eidson  <beidson@apple.com>
+
+        Modern IDB: Crash seen in IDBConnectionProxy::putOrAdd on GuardMalloc bot
+        https://bugs.webkit.org/show_bug.cgi?id=158124
+
+        Reviewed by Darin Adler.
+
+        No new tests (Covered by existing test configurations).
+
+        * Modules/indexeddb/IDBTransaction.cpp:
+        (WebCore::IDBTransaction::putOrAddOnServer):
+        
+        * Modules/indexeddb/client/IDBConnectionProxy.cpp:
+        (WebCore::IDBClient::IDBConnectionProxy::putOrAdd):
+        
+        * Modules/indexeddb/client/IDBConnectionProxy.h:
+        (WebCore::IDBClient::IDBConnectionProxy::callConnectionOnMainThread):
+        
+        * bindings/js/SerializedScriptValue.cpp:
+        (WebCore::SerializedScriptValue::writeBlobsToDiskForIndexedDB):
+        * bindings/js/SerializedScriptValue.h:
+        
+        * platform/network/BlobRegistry.h:
+        * platform/network/BlobRegistryImpl.cpp:
+        (WebCore::BlobRegistryImpl::writeBlobsToTemporaryFiles):
+        * platform/network/BlobRegistryImpl.h:
+
 2016-06-03  Ada Chan  <adachan@apple.com>
 

trunk/Source/WebCore/Modules/indexeddb/IDBTransaction.cpp

@@ -967 +967 @@
     }
 
-    RefPtr<IDBTransaction> protectedThis(this);
-    RefPtr<IDBClient::TransactionOperation> protectedOperation(&operation);
-    value->writeBlobsToDiskForIndexedDB([protectedThis = WTFMove(protectedThis), this, protectedOperation = WTFMove(protectedOperation), key = WTFMove(key), overwriteMode](const IDBValue& idbValue) mutable {
+    value->writeBlobsToDiskForIndexedDB([protectedThis = Ref<IDBTransaction>(*this), this, protectedOperation = Ref<IDBClient::TransactionOperation>(operation), keyData = IDBKeyData(key.get()).isolatedCopy(), overwriteMode](const IDBValue& idbValue) mutable {
         ASSERT(currentThread() == originThreadID());
         ASSERT(isMainThread());
         if (idbValue.data().data()) {
-            m_database->connectionProxy().putOrAdd(*protectedOperation, key.get(), idbValue, overwriteMode);
+            m_database->connectionProxy().putOrAdd(protectedOperation.get(), WTFMove(keyData), idbValue, overwriteMode);
             return;
         }
@@ -980 +978 @@
         // In that case, we cannot successfully store this record, so we callback with an error.
         auto result = IDBResultData::error(protectedOperation->identifier(), { IDBDatabaseException::UnknownError, ASCIILiteral("Error preparing Blob/File data to be stored in object store") });
-        callOnMainThread([protectedThis = WTFMove(protectedThis), protectedOperation = WTFMove(protectedOperation), result = WTFMove(result)]() {
+        callOnMainThread([protectedThis = WTFMove(protectedThis), protectedOperation = WTFMove(protectedOperation), result = WTFMove(result)]() mutable {
             protectedOperation->completed(result);
         });

trunk/Source/WebCore/Modules/indexeddb/client/IDBConnectionProxy.cpp

@@ -157 +157 @@
 }
 
-void IDBConnectionProxy::putOrAdd(TransactionOperation& operation, IDBKey* key, const IDBValue& value, const IndexedDB::ObjectStoreOverwriteMode mode)
-{
-    const IDBRequestData requestData(operation);
-    saveOperation(operation);
-
-    callConnectionOnMainThread(&IDBConnectionToServer::putOrAdd, requestData, IDBKeyData(key), value, mode);
+void IDBConnectionProxy::putOrAdd(TransactionOperation& operation, IDBKeyData&& keyData, const IDBValue& value, const IndexedDB::ObjectStoreOverwriteMode mode)
+{
+    const IDBRequestData requestData(operation);
+    saveOperation(operation);
+
+    callConnectionOnMainThread(&IDBConnectionToServer::putOrAdd, requestData, keyData, value, mode);
 }
 

trunk/Source/WebCore/Modules/indexeddb/client/IDBConnectionProxy.h

@@ -71 +71 @@
     void createIndex(TransactionOperation&, const IDBIndexInfo&);
     void deleteIndex(TransactionOperation&, uint64_t objectStoreIdentifier, const String& indexName);
-    void putOrAdd(TransactionOperation&, IDBKey*, const IDBValue&, const IndexedDB::ObjectStoreOverwriteMode);
+    void putOrAdd(TransactionOperation&, IDBKeyData&&, const IDBValue&, const IndexedDB::ObjectStoreOverwriteMode);
     void getRecord(TransactionOperation&, const IDBKeyRangeData&);
     void getCount(TransactionOperation&, const IDBKeyRangeData&);
@@ -125 +125 @@
     {
         if (isMainThread())
-            (m_connectionToServer.*method)(arguments...);
+            (m_connectionToServer.*method)(std::forward<Arguments>(arguments)...);
         else
             postMainThreadTask(m_connectionToServer, method, arguments...);

trunk/Source/WebCore/bindings/js/SerializedScriptValue.cpp

@@ -2797 +2797 @@
 }
 
-void SerializedScriptValue::writeBlobsToDiskForIndexedDB(std::function<void (const IDBValue&)> completionHandler)
+void SerializedScriptValue::writeBlobsToDiskForIndexedDB(NoncopyableFunction<void (const IDBValue&)>&& completionHandler)
 {
     ASSERT(isMainThread());

trunk/Source/WebCore/bindings/js/SerializedScriptValue.h

@@ -32 +32 @@
 #include <runtime/JSCJSValue.h>
 #include <wtf/Forward.h>
+#include <wtf/NoncopyableFunction.h>
 #include <wtf/RefCounted.h>
 #include <wtf/text/WTFString.h>
@@ -87 +88 @@
 #if ENABLE(INDEXED_DATABASE)
     Vector<String> blobURLsIsolatedCopy() const;
-    void writeBlobsToDiskForIndexedDB(std::function<void (const IDBValue&)> completionHandler);
+    void writeBlobsToDiskForIndexedDB(NoncopyableFunction<void (const IDBValue&)>&& completionHandler);
     IDBValue writeBlobsToDiskForIndexedDBSynchronously();
 #endif // ENABLE(INDEXED_DATABASE)

trunk/Source/WebCore/platform/network/BlobRegistry.h

@@ -34 +34 @@
 #include <functional>
 #include <wtf/Forward.h>
+#include <wtf/NoncopyableFunction.h>
 #include <wtf/Vector.h>
 
@@ -68 +69 @@
     virtual unsigned long long blobSize(const URL&) = 0;
 
-    virtual void writeBlobsToTemporaryFiles(const Vector<String>& blobURLs, std::function<void (const Vector<String>& filePaths)> completionHandler) = 0;
+    virtual void writeBlobsToTemporaryFiles(const Vector<String>& blobURLs, NoncopyableFunction<void (const Vector<String>& filePaths)>&& completionHandler) = 0;
 
     virtual bool isBlobRegistryImpl() const { return false; }

trunk/Source/WebCore/platform/network/BlobRegistryImpl.cpp

@@ -257 +257 @@
 };
 
-void BlobRegistryImpl::writeBlobsToTemporaryFiles(const Vector<String>& blobURLs, std::function<void (const Vector<String>& filePaths)> completionHandler)
+void BlobRegistryImpl::writeBlobsToTemporaryFiles(const Vector<String>& blobURLs, NoncopyableFunction<void (const Vector<String>& filePaths)>&& completionHandler)
 {
     Vector<BlobForFileWriting> blobsForWriting;
@@ -288 +288 @@
         Vector<String> filePaths;
 
-        ScopeGuard completionCaller([completionHandler]() mutable {
-            callOnMainThread([completionHandler = WTFMove(completionHandler)]() {
-                Vector<String> filePaths;
-                completionHandler(filePaths);
-            });
-        });
-
-        for (auto& blob : blobsForWriting) {
-            PlatformFileHandle file;
-            String tempFilePath = openTemporaryFile(ASCIILiteral("Blob"), file);
-
-            ScopeGuard fileCloser([file]() {
-                PlatformFileHandle handle = file;
-                closeFile(handle);
-            });
-            
-            if (tempFilePath.isEmpty() || !isHandleValid(file)) {
-                LOG_ERROR("Failed to open temporary file for writing a Blob to IndexedDB");
-                return;
-            }
-
-            for (auto& part : blob.filePathsOrDataBuffers) {
-                if (part.second.data()) {
-                    int length = part.second.data()->size();
-                    if (writeToFile(file, reinterpret_cast<const char*>(part.second.data()->data()), length) != length) {
-                        LOG_ERROR("Failed writing a Blob to temporary file for storage in IndexedDB");
-                        return;
-                    }
-                } else {
-                    ASSERT(!part.first.isEmpty());
-                    if (!appendFileContentsToFileHandle(part.first, file)) {
-                        LOG_ERROR("Failed copying File contents to a Blob temporary file for storage in IndexedDB (%s to %s)", part.first.utf8().data(), tempFilePath.utf8().data());
-                        return;
+        auto performWriting = [blobsForWriting = WTFMove(blobsForWriting), &filePaths]() {
+            for (auto& blob : blobsForWriting) {
+                PlatformFileHandle file;
+                String tempFilePath = openTemporaryFile(ASCIILiteral("Blob"), file);
+
+                ScopeGuard fileCloser([file]() mutable {
+                    closeFile(file);
+                });
+                
+                if (tempFilePath.isEmpty() || !isHandleValid(file)) {
+                    LOG_ERROR("Failed to open temporary file for writing a Blob to IndexedDB");
+                    return false;
+                }
+
+                for (auto& part : blob.filePathsOrDataBuffers) {
+                    if (part.second.data()) {
+                        int length = part.second.data()->size();
+                        if (writeToFile(file, reinterpret_cast<const char*>(part.second.data()->data()), length) != length) {
+                            LOG_ERROR("Failed writing a Blob to temporary file for storage in IndexedDB");
+                            return false;
+                        }
+                    } else {
+                        ASSERT(!part.first.isEmpty());
+                        if (!appendFileContentsToFileHandle(part.first, file)) {
+                            LOG_ERROR("Failed copying File contents to a Blob temporary file for storage in IndexedDB (%s to %s)", part.first.utf8().data(), tempFilePath.utf8().data());
+                            return false;
+                        }
                     }
                 }
+
+                filePaths.append(tempFilePath.isolatedCopy());
             }
 
-            filePaths.append(tempFilePath.isolatedCopy());
-        }
-
-        completionCaller.disable();
+            return true;
+        };
+
+        if (!performWriting())
+            filePaths.clear();
+
         callOnMainThread([completionHandler = WTFMove(completionHandler), filePaths = WTFMove(filePaths)]() {
             completionHandler(filePaths);

trunk/Source/WebCore/platform/network/BlobRegistryImpl.h

@@ -69 +69 @@
     unsigned long long blobSize(const URL&) override;
 
-    void writeBlobsToTemporaryFiles(const Vector<String>& blobURLs, std::function<void (const Vector<String>& filePaths)> completionHandler) override;
+    void writeBlobsToTemporaryFiles(const Vector<String>& blobURLs, NoncopyableFunction<void (const Vector<String>& filePaths)>&& completionHandler) override;
 
     HashMap<String, RefPtr<BlobData>> m_blobs;