Bug 156523 - Modern IDB (Blob support): Support deleting stored blob files
Summary: Modern IDB (Blob support): Support deleting stored blob files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords:
Depends on:
Blocks: 149117 143193
  Show dependency treegraph
 
Reported: 2016-04-12 15:56 PDT by Brady Eidson
Modified: 2016-04-13 09:28 PDT (History)
4 users (show)

See Also:


Attachments
Patch v1 (14.37 KB, patch)
2016-04-12 17:11 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch v2 - Fix the recordID condition check and remove the unused variable. (13.68 KB, patch)
2016-04-12 21:35 PDT, Brady Eidson
achristensen: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2016-04-12 15:56:17 PDT
Modern IDB (Blob support): Support deleting stored blob files

This includes:
-When an individual record is deleted
-When an objectStore is deleted
-When a whole database is deleted
Comment 1 Brady Eidson 2016-04-12 17:11:52 PDT
Created attachment 276293 [details]
Patch v1
Comment 2 Alex Christensen 2016-04-12 19:06:08 PDT
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?
Comment 3 Brady Eidson 2016-04-12 21:34:06 PDT
(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
Comment 4 Brady Eidson 2016-04-12 21:35:35 PDT
Created attachment 276303 [details]
Patch v2 - Fix the recordID condition check and remove the unused variable.
Comment 5 Brady Eidson 2016-04-13 09:28:46 PDT
http://trac.webkit.org/changeset/199499