Bug 40767 – DOM storage should delete databases when they are empty.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Hans Wennborg]

Even if a page clears its local storage, the database file for it stays around, and there is currently no mechanism that deletes them.

After doing the "final sync" of a storage area, the StorageAreaSyncMaster should see if the database is empty, and in that case delete it. This way, an empty local storage will never have a database file.

[Jeremy Orlow]

Sounds like a good plan.

[Hans Wennborg]

Created attachment 59079 [details]
Patch

[WebKit Review Bot]

Attachment 59079 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/storage/StorageAreaSync.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hans Wennborg]

(In reply to comment #3)
> Attachment 59079 [details] did not pass style-queue:
> 
> Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
> WebCore/storage/StorageAreaSync.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
> Total errors found: 1 in 8 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Sorry about that. I now realise the sort should be case sensitive (and the original file must have been in error to begin with).

[Hans Wennborg]

Created attachment 59081 [details]
Patch

[Jeremy Orlow]

Comment on attachment 59081 [details]
Patch

Looks good to me....2 double-checking type questions tho...


WebCore/storage/StorageAreaSync.cpp:421
 +          m_database.close();
Are we super extra ultra sure that there's no way the database can be used after it's closed?  I almost wondr if we should make it an OwnPtr and null it out here so that if we do try to use it later we crash.

WebCore/storage/StorageAreaSync.cpp:403
 +      if (!m_database.isOpen())
When can this happen?  Only if it was never opened or something?

[Hans Wennborg]

(In reply to comment #6)
> (From update of attachment 59081 [details])
> Looks good to me....2 double-checking type questions tho...
> 
> 
> WebCore/storage/StorageAreaSync.cpp:421
>  +          m_database.close();
> Are we super extra ultra sure that there's no way the database can be used after it's closed?  I almost wondr if we should make it an OwnPtr and null it out here so that if we do try to use it later we crash.
Pretty sure at least. The DeleteEmptyDatabase task is scheduled in scheduleFinalSync(), which is called by StorageAreaImpl::close() which is called by StorageNamespaceImpl::close() which is called by StorageNamespaceImpl's destructor. In effect, StorageAreaImpl should be destructed after deleteEmptyDatabases().

Anyway, the member functions in StorageAreaImpl that use m_database all check for it to be open first, so I don't really see any danger here.

> 
> WebCore/storage/StorageAreaSync.cpp:403
>  +      if (!m_database.isOpen())
> When can this happen?  Only if it was never opened or something?
Exactly. Since http://trac.webkit.org/changeset/61023, if there was no previous database file, no new database file is created (thus no db opened) unless something needs to be written to it.

[WebKit Commit Bot]

Comment on attachment 59081 [details]
Patch

Clearing flags on attachment: 59081

Committed r61542: <http://trac.webkit.org/changeset/61542>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.