Bug 186104 – Stop using StorageTracker.db in LocalStorageDatabaseTracker

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Sihui Liu]

StorageTracker.db acts as an cache for searching files in LocalStorage directory. There are a few reasons we may not want StorageTracker.db any more:
1. We could compute file name from origin, so as long as we have origin information(m_origins), we don't need StorageTracker.db for mapping from origin to file path. This help us simplify the code.
2. Maintaining(open/delete/update) a database could be expensive. 
3. Currently StorageTracker.db stores full paths, which causes problems like not deleting correct files after restoring from iCloud backup(<rdar://problem/31744826>).

[Sihui Liu]

Created attachment 341588 [details]
Patch

[Chris Dumez]

Comment on attachment 341588 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341588&action=review

> Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:63
> +    SQLiteFileSystem::deleteDatabaseFile(databasePath("StorageTracker.db"));

Shouldn't we do this in the dispatch call above so that we do I/O on the background thread instead of the main thread?

> Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:87
> +    if (path.isEmpty())

if (!path.isEmpty())
    SQLiteFileSystem::deleteDatabaseFile(path);

since we'd want to notify the clients even if the path was empty I would  assume?

> Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:98
> +    Vector<String> paths = FileSystem::listDirectory(m_localStorageDirectory, "*.localstorage");

auto paths

> Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:111
> +    importOrigins();

Looks like this unconditionally re-imports all origins from disk, even though they may have already been imported. This seems bad.

> Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:131
> +        origins.append(origin);

This should have been uncheckedAppend() since you reserveInitialCapacity() beforehand. However, in this case, we should rely on pre-existing copyToVector() instead of duplicating the logic for copying a container into a vector.

> Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:171
> +    m_origins.clear();

What if origins were already imported?

> Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:174
>          if (!path.endsWith(".localstorage"))

Why is this needed? Maybe this could be an assertion?

> Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.h:60
> +        double creationTime;

We try to avoid storing timestamps as double nowadays. This and the one below should probably be WallTime instead of double.

[Sihui Liu]

Created attachment 341597 [details]
Patch

[Chris Dumez]

Comment on attachment 341597 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341597&action=review

> Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:63
> +    SQLiteFileSystem::deleteDatabaseFile(databasePath("StorageTracker.db"));

Why are we still doing this on the main thread?

> Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:110
> +    importOrigins();

Why is this OK? This will reimport all origins from disk, even though they have likely already been imported.

> Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:166
>          if (!path.endsWith(".localstorage"))

Why is this still here? Can this be made an assertion?

> Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:172
> +        if (origin)

Should we add a RELEASE_LOG_ERROR() for the case where it fails? It seems like we'd want to know if we fail to import origins from the disk.

[Sihui Liu]

Created attachment 341599 [details]
Patch

[Chris Dumez]

When you re-upload a patch please address ALL comments, either in the patch or via a comment explaining why you chose not to.

[Sihui Liu]

(In reply to Chris Dumez from comment #4)
> Comment on attachment 341597 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341597&action=review
> 
> > Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:63
> > +    SQLiteFileSystem::deleteDatabaseFile(databasePath("StorageTracker.db"));
> 
> Why are we still doing this on the main thread?
> 
Reload and fix.

> > Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:110
> > +    importOrigins();
> 
> Why is this OK? This will reimport all origins from disk, even though they
> have likely already been imported.
> 
See <rdar://problem/22781730>.

> > Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:166
> >          if (!path.endsWith(".localstorage"))
> 
> Why is this still here? Can this be made an assertion?
> 
Database file is accompanied by files "-shm" and "-wal"; e.g. "xxx.localstorage" exists with "xxx.localstorage-shm" and "xxx.localstorage-wal".

[Geoffrey Garen]

Comment on attachment 341599 [details]
Patch

Did you measure the time it takes to load StorageTracker.db vs list the directory? What do those timings look like?

If StorageTracker.db is faster than listing the directory, then we need some other plan for solving the launch-time performance issue cased by LocalStorageDatabaseTracker::LocalStorageDatabaseTracker.

[Chris Dumez]

(In reply to Sihui Liu from comment #7)
> (In reply to Chris Dumez from comment #4)
> > Comment on attachment 341597 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=341597&action=review
> > 
> > > Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:63
> > > +    SQLiteFileSystem::deleteDatabaseFile(databasePath("StorageTracker.db"));
> > 
> > Why are we still doing this on the main thread?
> > 
> Reload and fix.
> 
> > > Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:110
> > > +    importOrigins();
> > 
> > Why is this OK? This will reimport all origins from disk, even though they
> > have likely already been imported.
> > 
> See <rdar://problem/22781730>.

I have looked at this radar and am still not unclear why we'd want to force a reimport. Please clarify.
Either we could add a flag to indicate that we've already done the import, or for some reason we do not trust our in-memory data. If we do not trust our in-memory data for this operation, then why are we trusting it for other operations?

> > > Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:166
> > >          if (!path.endsWith(".localstorage"))
> > 
> > Why is this still here? Can this be made an assertion?
> > 
> Database file is accompanied by files "-shm" and "-wal"; e.g.
> "xxx.localstorage" exists with "xxx.localstorage-shm" and
> "xxx.localstorage-wal".

We are asking for "*.localstorage", not "*.localstorage*" so I would not expect such files to be returned. Are they?

[Chris Dumez]

Comment on attachment 341599 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341599&action=review

> Source/WebCore/ChangeLog:3
> +        Stop using StorageTracker.db in LocalStroageDatabaseTracker

typo: LocalStroageDatabaseTracker

> Source/WebKit/ChangeLog:3
> +        Stop using StorageTracker.db in LocalStroageDatabaseTracker

typo: LocalStroageDatabaseTracker

[Sihui Liu]

(In reply to Chris Dumez from comment #9)
> (In reply to Sihui Liu from comment #7)
> > (In reply to Chris Dumez from comment #4)
> > > Comment on attachment 341597 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=341597&action=review
> > > 
> > > > Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:63
> > > > +    SQLiteFileSystem::deleteDatabaseFile(databasePath("StorageTracker.db"));
> > > 
> > > Why are we still doing this on the main thread?
> > > 
> > Reload and fix.
> > 
> > > > Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:110
> > > > +    importOrigins();
> > > 
> > > Why is this OK? This will reimport all origins from disk, even though they
> > > have likely already been imported.
> > > 
> > See <rdar://problem/22781730>.
> 
> I have looked at this radar and am still not unclear why we'd want to force
> a reimport. Please clarify.
> Either we could add a flag to indicate that we've already done the import,
> or for some reason we do not trust our in-memory data. If we do not trust
> our in-memory data for this operation, then why are we trusting it for other
> operations?
> 
In this bug two UI processes are modifying the same directory, which actually is not an expected use case of WebKit.

In the new patch I removed m_origins which might resolve the confusion and the bug.

> > > > Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:166
> > > >          if (!path.endsWith(".localstorage"))
> > > 
> > > Why is this still here? Can this be made an assertion?
> > > 
> > Database file is accompanied by files "-shm" and "-wal"; e.g.
> > "xxx.localstorage" exists with "xxx.localstorage-shm" and
> > "xxx.localstorage-wal".
> 
> We are asking for "*.localstorage", not "*.localstorage*" so I would not
> expect such files to be returned. Are they?
Yes! I've tried, removed the redundant code.

[Sihui Liu]

Created attachment 341624 [details]
Patch

[Geoffrey Garen]

Comment on attachment 341624 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341624&action=review

> Source/WebKit/ChangeLog:8
> +        Stop using StorageTracker.db and stop caching origins in LocalStorageDatabaseTracker for efficiency 

The specific efficiency we're trying to achieve is an improvement in launch time, especially on computers with spinning disks and large local storage collections.

[Geoffrey Garen]

Comment on attachment 341624 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341624&action=review

r=me

> Source/WebCore/platform/sql/SQLiteFileSystem.h:60
> +    WEBCORE_EXPORT static String appendDatabaseFileNameToPath(const String& path, const String& fileName);

This is something that's nice to explain in your ChangeLog. What you've done here is to call SQLiteFileSystem functions in cases where we used to call FileSystem functions because SQLiteFileSystem is logically more correct.

[Chris Dumez]

Comment on attachment 341624 [details]
Patch

So we're not worried about the performance impact of not having the m_origins cache now and going to disk every time we need the origins?

[Chris Dumez]

(In reply to Chris Dumez from comment #9)
> (In reply to Sihui Liu from comment #7)
> > (In reply to Chris Dumez from comment #4)
> > > Comment on attachment 341597 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=341597&action=review
> > > 
> > > > Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:63
> > > > +    SQLiteFileSystem::deleteDatabaseFile(databasePath("StorageTracker.db"));
> > > 
> > > Why are we still doing this on the main thread?
> > > 
> > Reload and fix.
> > 
> > > > Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:110
> > > > +    importOrigins();
> > > 
> > > Why is this OK? This will reimport all origins from disk, even though they
> > > have likely already been imported.
> > > 
> > See <rdar://problem/22781730>.

This radar is closed, did you point me to the right radar?

[Geoffrey Garen]

> So we're not worried about the performance impact of not having the
> m_origins cache now and going to disk every time we need the origins?

We did some debugging and it looks like we only make use of m_origins when you list website data in the Safari preferences window. Other storage technologies do not appear to cache their lists, so there's little incremental benefit to LocalStorage caching its list. Either way, you have a few seconds of async delay before the list populates.

If we do see more frequent use of this API, I think we'll want to add back the m_origins cache.

(Note that there is an open discussion about what should happen when multiple processes point at the same data store path. If we wanted to revive the m_origins cache, we would need to resolve that discussion and either (a) explain how the cache can be coherent across multiple UI processes or (b) decide that multiple UI processes with concurrent access to the same data store path is not supported.)

[Geoffrey Garen]

> > > > Why is this OK? This will reimport all origins from disk, even though they
> > > > have likely already been imported.
> > > > 
> > > See <rdar://problem/22781730>.
> 
> This radar is closed, did you point me to the right radar?

This Radar explains the original intention behind re-importing all origins, namely: working around a bug that can happen when multiple UI processes use the same data store path.

FWIW, the latest patch removes the extra import in favor of just not caching at all, since we weren't able to demonstrate a benefit to caching.

<rdar://problem/40665000> is the ongoing discussion about whether we should support concurrent access to a data store path or not.

[WebKit Commit Bot]

Comment on attachment 341624 [details]
Patch

Clearing flags on attachment: 341624

Committed r232410: <https://trac.webkit.org/changeset/232410>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40732090>