Removing IndexedDatabases that have stored blobs doesn't remove the blob files <rdar://problem/27495748>
Created attachment 284352 [details] Patch
Attachment 284352 [details] did not pass style-queue: ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/StoreBlobThenDelete.mm:30: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/StoreBlobThenDelete.mm:90: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 2 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 284352 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=284352&action=review Presumably a step in the right direction but I think there may be more to be done here. What happens if the process crashes or battery runs out part way through the deletion process? Is there some mechanism to make sure that we return and delete what remains? If so, then it seems important to delete the database file last, since if the file does not exist, we skip the entire deletion process. > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:561 > + Vector<String> blobPaths = listDirectory(databasePath, "[0-9]*.blob"); > + for (auto& blobPath : blobPaths) { The local variable for blobPaths isn’t needed. The for loop extends its lifetime appropriately. It wouldn’t if we were calling a function on it, but it does because it’s the value of the thing after the colon. But maybe you prefer this style. > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:564 > + // We need to validate each filename contains only digits before deleting it. This comment comes close to saying “why” but doesn’t quite do it. Where is the comment that explains the basic strategy. Why do we need to any of these files? Why do we need to delete all of these files? Why do we need to delete only these files? I have a guess about the strategy, but it would be really nice to be explicit. > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:570 > + if (filename.length() < 6) I suggest putting length() in a local variable since it’s ned below in a loop and its implementation is not necessarily trivial enough to be hoisted out of it. > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:577 > + if (!(filename[i] >= '0' && filename[i] <= '9')) { I think isASCIIDigit would be better here.
All great comments I will address. Thanks for the review!
Created attachment 284372 [details] Patch
Attachment 284372 [details] did not pass style-queue: ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/StoreBlobThenDelete.mm:30: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/StoreBlobThenDelete.mm:90: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 2 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
This new patch addresses all feedback, except for: (In reply to comment #3) > What happens if the process crashes or battery runs out part way through the > deletion process? Is there some mechanism to make sure that we return and > delete what remains? If so, then it seems important to delete the database > file last, since if the file does not exist, we skip the entire deletion > process. I did address deleting the sqlite3 database file last, which means if the process crashes/battery runs out partway through deleting blobs, the *next* attempt to delete the database will pick up where it left off. I did *not* attempt to address the point that if there's a crash/power-outage, that the deletion process should automatically pick up where it left off. There are strategies that could be applied to accomplish that, but I'm not sure they're worth the effort for this edge case. A lot of interesting tricks can be played to make the entire thing be atomic once APFS is widely deployed...
Comment on attachment 284372 [details] Patch Clearing flags on attachment: 284372 Committed r203622: <http://trac.webkit.org/changeset/203622>
All reviewed patches have been landed. Closing bug.