Your last point seems to say that localstorage is ultimately deleted correctly, it just sometimes takes longer than you'd expect?

This sounds a lot like the comment in DOMStorageArea::DeleteOrigin btw, explaining that there might be such a delay if there is uncommitted data:
    // TODO(michaeln): This logically deletes the data immediately,
    // and in a matter of a second, deletes the rows from the backing
    // database file, but the file itself will linger until shutdown
    // or purge time. Ideally, this should delete the file more
    // quickly.

Not sure why that codepath doesn't call ScheduleImmediateCommit to cause the deletion to happen sooner...

You're right, it is only a temporary issue and the entry disappears at some point.
I tried to add ScheduleImmediateCommit() after the call to Clear() in DOMStorageArea::DeleteOrigin but it doesn't seem to help with the problem. I will ask if the current behavior is ok or if we should try to improve it, thanks for your help.

Ah yes, ScheduleImmediateCommit will cause the data to be deleted from the sqlite database sooner, but it still won't actually delete the database file itself until later.

In some ways this issue should go away automatically by the localstorage backend rewrite I'm currently working on. Since it will no longer have a separate database file per origin the second step of deleting the actual database if it is empty is no longer needed; just deleting all the data for the origin in the one shared database should be enough.

Hi,

I wanted to ask about the status of  https://crbug.com/586194 . 

I noticed that I still get localstorage entries with 0 B after I go to chrome://settings/content/cookies and click "Remove All".

Do you already use a single database for all sites? 

So with M61 that does indeed happen. But it's unfortunately still possible for entries with 0 B to show up because the code currently is erring on the side of reporting too many origins rather than not enough. Problematic cases are origins with data, but that data hasn't been committed to disk yet. We definitely do want to show those origins (even though their actual disk usage is 0 bytes), but the current code that takes care of that includes any origin that is currently "open" rather than trying to limit itself to just origins with uncommitted data. The hard case here is that it is not as trivial as you might expect to actually determine if an origin has uncommitted data or not.

Getting into implementation details: the current renderer side code guarantees that a (blocking) LevelDBWrapper::GetAll call is made before any change that might cause an origin to end up with uncommitted data. This is however not required/guaranteed by the LevelDBWrapper interface. What this means is that (at least from the browser process's point of view) the following is possible:
1- receive open storage request from renderer
2- consumer already queued up a write to that storage
3- queue up open request while connection to underlying leveldb is established
4- bunch of async work to actually get leveldb connection setup and verified
5- actually connect open request to a leveldbwrapperimpl instance
6- LevelDBWrapperImpl receives write from 2
7- LevelDBWrapperImpl has to do a bunch of its own async work to load data before processing write
8- LevelDBWrapperImpl finally processes write from 2 and marks itself as having uncommitted data, and schedules a commit

So there is at least two or three (mostly opaque) queues here where uncommitted data can be hiding before the localstorage code actually knows for sure that an origin has uncommitted data. To be safe we now just consider any origin that's anywhere between after step 2, and not yet closed, to have uncommitted data (which does unfortunately mean that we sometimes show too many 0 byte entries).

One relatively easy improvement we could make would be for LevelDBWrapperImpl to somehow expose that it completed loading (and completed processing all OnLoadComplete tasks), and have it expose if it currently has uncommitted data, and use those two flags together to only include as 0 byte origins origins that have not been completely loaded yet, or origins that have been loaded and where we know for sure that they have uncommitted data.

 Issue 764934  has been merged into this issue.

Upping the priority, as we're getting user reports that are likely attributable to this (unless there are similar problems with other site data storage backends).

So just to be clear, as far as I know there is no bug that data isn't always deleted completely. The remaining bug here is that sometimes the UI might make it seem like data isn't being deleted.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/58a0b3a73c1e1389cdca75c6aec4816bb2da360a

commit 58a0b3a73c1e1389cdca75c6aec4816bb2da360a
Author: Marijn Kruisselbrink <mek@chromium.org>
Date: Sat Sep 30 23:35:39 2017

Filter out origins that definitely don't have uncommitted localstorage data.

LocalStorageContext::GetStorageUsage needs to include origins with
uncommitted data to ensure those can be cleared, but the logic used to
decide which origins to include was a bit too liberal, often including
origins where we know for sure no data actually exists for that origin.
That then lead to confusion when the UI might make it seem like storage
isn't being cleared correctly. This should fix that confusion.

Bug:  679344 
Change-Id: Id84ade9752beb0e874b6823c0a9783b22b23aabd
Reviewed-on: https://chromium-review.googlesource.com/688194
Reviewed-by: Michael Nordman <michaeln@chromium.org>
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505477}
[modify] https://crrev.com/58a0b3a73c1e1389cdca75c6aec4816bb2da360a/content/browser/dom_storage/local_storage_context_mojo.cc
[modify] https://crrev.com/58a0b3a73c1e1389cdca75c6aec4816bb2da360a/content/browser/leveldb_wrapper_impl.h

I think this should pretty much be fixed now. Certainly the original issue is no longer relevant.