Bug 156523

Created attachment 276293 [details] Patch v1

Comment on attachment 276293 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=276293&action=review > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1305 > + for (auto& file : removedBlobFilenames) > + transaction.addRemovedBlobFile(file); In addRemovedBlobFile, we assert that they are all unique. That means that it was empty before this loop. Can we just transfer the HashSet, or use a Vector somewhere to be more efficient? > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1352 > + if (recordID < -1) { Would -1 also be an error? > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1661 > + ThreadSafeDataBuffer resultBuffer; Is this used? Please remove. > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1988 > + LOG_ERROR("Error getting all blob filenames to be deleted"); ASSERT_NOT_REACHED()? > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1994 > + deleteFile(fullPath); Why do we only delete the file when we delete the backing store? What if we remove all references to the file?

(In reply to comment #2) > Comment on attachment 276293 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=276293&action=review > > > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1305 > > + for (auto& file : removedBlobFilenames) > > + transaction.addRemovedBlobFile(file); > > In addRemovedBlobFile, we assert that they are all unique. That means that > it was empty before this loop. Can we just transfer the HashSet, or use a > Vector somewhere to be more efficient? If we only ran through this loop once, we could transfer the collection over. But deleteUnusedBlobFileRecords can be called multiple times per transaction. The ASSERT is valid - there should never be a scenario where the same filename is removed more than once. > > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1352 > > + if (recordID < -1) { > > Would -1 also be an error? Yes. So would 0. It was meant to be "recordID < 1" > > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1661 > > + ThreadSafeDataBuffer resultBuffer; > > Is this used? Please remove. > > > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1988 > > + LOG_ERROR("Error getting all blob filenames to be deleted"); > > ASSERT_NOT_REACHED()? The error condition can occur due to file data on disk, and I'm not willing to say it's appropriate to ASSERT based on it. Sometimes, yes. For a reason I can't articulate, I don't think so this time. > > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1994 > > + deleteFile(fullPath); > > Why do we only delete the file when we delete the backing store? What if we > remove all references to the file? If we remove all references to the file, the file is added to the SQLiteIDBTransactions set of removed files. Which then does the delete at commit time inside of SQLiteIDBTransaction::deleteBlobFilesIfNecessary

Created attachment 276303 [details] Patch v2 - Fix the recordID condition check and remove the unused variable.