[ Mojave WK1 ] Layout Test storage/indexeddb/database-odd-names.html is failing

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Truitt Savell]

The following layout test is failing on Mojave WK1

storage/indexeddb/database-odd-names.html

Probable cause:

Test is failing continuously on MacOS Mojave after Mojave was launched on OpenSource. 

Flakiness Dashboard:

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=storage%2Findexeddb%2Fdatabase-odd-names.html

[Radar WebKit Bug Importer]

<rdar://problem/45089503>

[Alexey Proskuryakov]

@@ -13,11 +13,9 @@
 indexedDB.open(testData[nextToOpen].name)
 opening a database named fffe
 indexedDB.open(testData[nextToOpen].name)
-opening a database named ffff
-indexedDB.open(testData[nextToOpen].name)
-opening a database named line separator
-indexedDB.open(testData[nextToOpen].name)
+FAIL Error function called unexpectedly: (UnknownError) Unable to open database file on disk
 PASS successfullyParsed is true
+Some tests failed.
 
 TEST COMPLETE

[Ryan Haddad]

Marked test as failing in https://trac.webkit.org/r237338 so the WK1 bots have a chance of being green.

We still need to regress this / find repro steps.

[Sihui Liu]

Created attachment 363911 [details]
Patch

[Sihui Liu]

Created attachment 364038 [details]
Patch

[Brady Eidson]

Comment on attachment 364038 [details]
Patch

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

I think my recommendation changes/additions are straight forward.

This is a bit of a fun filesystem patch, so please have at least one other reviewer look closely.

> Source/WebCore/ChangeLog:9
> +        Start to use hash for database file names so that the files can work with different file systems.

work *on any* filesystem

> Source/WebCore/ChangeLog:11
> +        Covered by existing test.

Nah... Please API test this. We can write some databases with various names out and then in native code verify the expected files exist in the expected locations.

It would also be SUPER great to API test the collision detection code by having two databases in the same origin that hash to the same place.

If it's too difficult to find two database names that would collide (maybe their names would be too massively large or something), then the API test can manually copy a file to a folder on disk that collides with a database name in the test.

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:721
> +    auto versionDirectory = FileSystem::pathByAppendingComponent(m_databaseDirectoryPath, "v1");
> +    if (!FileSystem::fileExists(versionDirectory)) {
> +        auto originPaths = FileSystem::listDirectory(m_databaseDirectoryPath, "*"_s);
> +        auto oldVersionDirectory = FileSystem::pathByAppendingComponent(m_databaseDirectoryPath, "v0");
> +        FileSystem::makeAllDirectories(versionDirectory);
> +        FileSystem::makeAllDirectories(oldVersionDirectory);
> +        for (auto& originPath : originPaths) {
> +            auto origin = FileSystem::lastComponentOfPathIgnoringTrailingSlash(originPath);
> +            FileSystem::moveFile(originPath, FileSystem::pathByAppendingComponent(oldVersionDirectory, origin));
> +        }
> +    }

I discussed in person an approach that could just do a couple renames and one mkdir to do all of this in much less time.

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:785
> +String SQLiteIDBBackingStore::databaseDirectoryWithHash(const String& directory, const String& fileNameHash, const char separator, unsigned number)
> +{
> +    StringBuilder stringBuilder;
> +    stringBuilder.append(fileNameHash);
> +    stringBuilder.append(separator);
> +    stringBuilder.appendNumber(number);
> +    return FileSystem::pathByAppendingComponent(directory, stringBuilder.toString());
> +}

This doesn't need to be a member function, can be a static function in this file.

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:807
> +    int number = 0;

unsigned

[Sihui Liu]

Created attachment 364080 [details]
Patch

[EWS Watchlist]

Comment on attachment 364080 [details]
Patch

Attachment 364080 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11434159

New failing tests:
compositing/video/video-clip-change-src.html

[EWS Watchlist]

Created attachment 364103 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 364080 [details]
Patch

Attachment 364080 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11433826

New failing tests:
fast/viewport/ios/device-width-viewport-after-changing-view-scale.html

[EWS Watchlist]

Created attachment 364104 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Sihui Liu]

Created attachment 364390 [details]
Patch

[youenn fablet]

Comment on attachment 364390 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBDatabaseIdentifier.cpp:66
> +    String mainFrameDirectory = FileSystem::pathByAppendingComponent(versionDirectory, topLevelOrigin.databaseIdentifier());

If we do change the paths of the databases, it might be nice (probably not in this patch since this is already big) to obfuscate the folder names that currently contain topLevelOrigin/openingOrigin information, like done in NetworkCache for instance.

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:468
> +    Vector<String> files = FileSystem::listDirectory(oldDirectory, "*"_s);

Can we make sure we do the upgrade so that we can remove this "vo" handling here?

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:707
> +{

We usually do file operations in a background thread.
Can we do that in the IDBServer background thread?

[Geoffrey Garen]

Comment on attachment 364390 [details]
Patch

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

r=me

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBFileName.mm:72
> +

Can you add a test for removing website data, when the data has only a v0 IndexedDB entry, or only a v1 IndexedDB entry? (It looks like IndexedDBUserDelete.mm will test removing a fully unmigrated database path.)

[Geoffrey Garen]

> Can we make sure we do the upgrade so that we can remove this "vo" handling
> here?

You know, I agree with this feedback.

We've had a hard time getting IndexedDB reliability right, and testing all its edge cases. It's a big challenge to maintain three on-disk states going forward. If we can guarantee that every user does this one-time upgrade, then we only need to test and maintain the upgrade itself, and not test every other code path three ways.

So, I'd recommend migrating all files on startup if not migrated yet. This is a one-time cost after software update, but fast thereafter.

[Brady Eidson]

(In reply to youenn fablet from comment #13)
> Comment on attachment 364390 [details]
> Patch
> > 
> > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:707
> > +{
> 
> We usually do file operations in a background thread.
> Can we do that in the IDBServer background thread?

This is on a background thread already.

> > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:468
> > +    Vector<String> files = FileSystem::listDirectory(oldDirectory, "*"_s);
> 
> Can we make sure we do the upgrade so that we can remove this "vo" handling
> here?

I don't like one time upgrades, which is why I reccommended this approach to Sihui.

(In reply to Geoffrey Garen from comment #15)
> > Can we make sure we do the upgrade so that we can remove this "vo" handling
> > here?
> > 
> We've had a hard time getting IndexedDB reliability right, and testing all
> its edge cases. It's a big challenge to maintain three on-disk states going
> forward. If we can guarantee that every user does this one-time upgrade,
> then we only need to test and maintain the upgrade itself, and not test
> every other code path three ways.
> 
> So, I'd recommend migrating all files on startup if not migrated yet. This
> is a one-time cost after software update, but fast thereafter.

I have experience doing upgrade migrations of WebKit owned data.
Doing a one time upgrade seems neat, but usually introduces fun new edge cases.

A one time upgrade - even one that seems as simple/straightforward as this - can be quick for many/most users, but can be *excruciatingly slow* for some users. Feel free to ask me in person which execs have directly noticed some previous one time upgrades even though testing showed they'd be "pretty quick"

If you do the one time upgrade all at once, you have to prevent the app from quitting if the user quits while the upgrade is in progress.
Additionally - depending on the specifics of the migration - this approach opens you up to a much wider time window where permanent data loss can occur if there's a crash.

If you implement it "interuptably" in a way where it works little by little and can be stopped partway through, then the code to do that might be just as complex as this code.

A user visible symptom of doing a one-time migration will be that websites with legitimate IndexedDB use can stall completely or be inexplicably broken while the upgrade is in progress.

I think the current approach can be effectively tested with API tests and doesn't suffer the above drawbacks.

[youenn fablet]

Comment on attachment 364390 [details]
Patch

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

>>> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:707
>>> +{
>> 
>> We usually do file operations in a background thread.
>> Can we do that in the IDBServer background thread?
> 
> This is on a background thread already.

upgradeFilesIfNecessary() is called in IDBServer constructor.

[Sihui Liu]

Created attachment 364623 [details]
Patch

[Sihui Liu]

Created attachment 364660 [details]
Patch

[youenn fablet]

Comment on attachment 364660 [details]
Patch


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

> Source/WebCore/DerivedSources-output.xcfilelist:1923
> +$(BUILT_PRODUCTS_DIR)/DerivedSources/WebCore/JSWebGPUCommandEncoder.h

To be removed here and above?

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:78
> +    postDatabaseTask(createCrossThreadTask(*this, &IDBServer::upgradeFilesIfNecessary));

We probably do not need to post this task for private sessions.

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:473
> +    Vector<String> files = FileSystem::listDirectory(oldDirectory, "*"_s);

auto?

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:630
> +    if (!SecurityOriginData::fromDatabaseIdentifier(databaseIdentifier).hasValue())

hasValue() not needed?

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:633
> +    Vector<String> directories = FileSystem::listDirectory(originPath, "*"_s);

auto?

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:634
> +    for (auto directory : directories) {

auto&

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:679
> +    if (!m_databaseDirectoryPath.isEmpty()) {

Do we even need to post a task in case of ephemeral session?

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:803
> +    auto size = SQLiteIDBBackingStore::databasesSizeForFolder(oldVersionOriginDirectory) + SQLiteIDBBackingStore::databasesSizeForFolder(newVersionOriginDirectory);

The quota computation happens in two places, here and in SQLiteBackingStore.
This patch will break things but I think that is fine, I should change it so that we only use that routine and stop asking the SQLiteBackingStore.

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:239
> +    m_databaseDirectory = fullDatabaseDirectoryWithUpgrade();

Could we initialize them just above as , m_databaseRootDirectory()...

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1946
> +    String databaseDirectory = m_databaseDirectory;

Can we remove databaseDirectory?

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:2642
> +    String databaseDirectory = m_databaseDirectory;

Can we remove databaseDirectory?

> Source/WebCore/platform/sql/SQLiteFileSystem.cpp:123
> +    // Convert digest to hex.

Why not base64?
It seems other places in WebKit are doing so.
Maybe this should be a routine in FileSystem?

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1227
> +    FileSystem::deleteEmptyDirectory(oldVersionDirectory);

Maybe ChangeLog could describe this? Can we do that in NetworkProcess/IDB/background thread instead?

[Geoffrey Garen]

Comment on attachment 364660 [details]
Patch

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

r=me if you resolve Youenn's comments

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:850
> +    String newVersionDirectory = FileSystem::pathByAppendingComponent(m_databaseDirectoryPath, "v1");

It's probably worth adding a comment here to explain that the UI process creates the v0 symlink.

[Sihui Liu]

(In reply to youenn fablet from comment #20)
> Comment on attachment 364660 [details]
> Patch
> 
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=364660&action=review
> 
> > Source/WebCore/DerivedSources-output.xcfilelist:1923
> > +$(BUILT_PRODUCTS_DIR)/DerivedSources/WebCore/JSWebGPUCommandEncoder.h
> 
> To be removed here and above?
> 
Yeees.

> > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:78
> > +    postDatabaseTask(createCrossThreadTask(*this, &IDBServer::upgradeFilesIfNecessary));
> 
> We probably do not need to post this task for private sessions.
> 
Private session IDBServer will no be created with a databaseDirectoryPath parameter.

> > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:473
> > +    Vector<String> files = FileSystem::listDirectory(oldDirectory, "*"_s);
> 
> auto?
> 
Okay.

> > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:630
> > +    if (!SecurityOriginData::fromDatabaseIdentifier(databaseIdentifier).hasValue())
> 
> hasValue() not needed?
> 
True.

> > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:633
> > +    Vector<String> directories = FileSystem::listDirectory(originPath, "*"_s);
> 
> auto?
> 
Okay.

> > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:634
> > +    for (auto directory : directories) {
> 
> auto&
> 
Okay.

> > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:679
> > +    if (!m_databaseDirectoryPath.isEmpty()) {
> 
> Do we even need to post a task in case of ephemeral session?
> 
Check for ephemeral session is in network process, for example NetworkProcess::deleteWebsiteDataForOrigins.

> > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:803
> > +    auto size = SQLiteIDBBackingStore::databasesSizeForFolder(oldVersionOriginDirectory) + SQLiteIDBBackingStore::databasesSizeForFolder(newVersionOriginDirectory);
> 
> The quota computation happens in two places, here and in SQLiteBackingStore.
> This patch will break things but I think that is fine, I should change it so
> that we only use that routine and stop asking the SQLiteBackingStore.
> 
> > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:239
> > +    m_databaseDirectory = fullDatabaseDirectoryWithUpgrade();
> 
> Could we initialize them just above as , m_databaseRootDirectory()...
> 
Yes.

> > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1946
> > +    String databaseDirectory = m_databaseDirectory;
> 
> Can we remove databaseDirectory?
> 
Removed.

> > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:2642
> > +    String databaseDirectory = m_databaseDirectory;
> 
> Can we remove databaseDirectory?
> 
Removed.

> > Source/WebCore/platform/sql/SQLiteFileSystem.cpp:123
> > +    // Convert digest to hex.
> 
> Why not base64?
> It seems other places in WebKit are doing so.
> Maybe this should be a routine in FileSystem?
> 
Base64 has special characters like '/' we want to escape for file names. And Hex of SHA-256 digest still fits the length requirement.

> > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1227
> > +    FileSystem::deleteEmptyDirectory(oldVersionDirectory);
> 
> Maybe ChangeLog could describe this? Can we do that in
> NetworkProcess/IDB/background thread instead?

Will add to ChangeLog.
This function has one or two simple file system operations and is in UI process.  Creating a background thread for this seems to be overkill.

[Sihui Liu]

Created attachment 364850 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 364850 [details]
Patch for landing

Clearing flags on attachment: 364850

Committed r243019: <https://trac.webkit.org/changeset/243019>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.