Bug 190350 - [ Mojave WK1 ] Layout Test storage/indexeddb/database-odd-names.html is failing
Summary: [ Mojave WK1 ] Layout Test storage/indexeddb/database-odd-names.html is failing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sihui Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-10-08 08:51 PDT by Truitt Savell
Modified: 2019-03-15 15:32 PDT (History)
17 users (show)

See Also:


Attachments
Patch (30.35 KB, patch)
2019-03-07 12:51 PST, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (40.59 KB, patch)
2019-03-08 11:03 PST, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (68.04 KB, patch)
2019-03-08 15:16 PST, Sihui Liu
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (2.62 MB, application/zip)
2019-03-08 18:25 PST, Build Bot
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (7.19 MB, application/zip)
2019-03-08 18:31 PST, Build Bot
no flags Details
Patch (68.15 KB, patch)
2019-03-12 09:13 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (69.76 KB, patch)
2019-03-13 20:46 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (72.03 KB, patch)
2019-03-14 08:49 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (72.15 KB, patch)
2019-03-15 14:54 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Truitt Savell 2018-10-08 08:51:40 PDT
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
Comment 1 Radar WebKit Bug Importer 2018-10-08 08:52:34 PDT
<rdar://problem/45089503>
Comment 2 Alexey Proskuryakov 2018-10-08 09:48:25 PDT
@@ -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
Comment 3 Ryan Haddad 2018-10-22 16:18:21 PDT
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.
Comment 4 Sihui Liu 2019-03-07 12:51:26 PST
Created attachment 363911 [details]
Patch
Comment 5 Sihui Liu 2019-03-08 11:03:51 PST
Created attachment 364038 [details]
Patch
Comment 6 Brady Eidson 2019-03-08 11:45:58 PST
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
Comment 7 Sihui Liu 2019-03-08 15:16:49 PST
Created attachment 364080 [details]
Patch
Comment 8 Build Bot 2019-03-08 18:25:08 PST
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
Comment 9 Build Bot 2019-03-08 18:25:10 PST
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
Comment 10 Build Bot 2019-03-08 18:31:33 PST
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
Comment 11 Build Bot 2019-03-08 18:31:35 PST
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
Comment 12 Sihui Liu 2019-03-12 09:13:08 PDT
Created attachment 364390 [details]
Patch
Comment 13 youenn fablet 2019-03-12 10:32:35 PDT
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?
Comment 14 Geoffrey Garen 2019-03-12 10:42:53 PDT
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.)
Comment 15 Geoffrey Garen 2019-03-12 10:49:15 PDT
> 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.
Comment 16 Brady Eidson 2019-03-12 11:33:02 PDT
(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.
Comment 17 youenn fablet 2019-03-12 11:40:06 PDT
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.
Comment 18 Sihui Liu 2019-03-13 20:46:12 PDT
Created attachment 364623 [details]
Patch
Comment 19 Sihui Liu 2019-03-14 08:49:08 PDT
Created attachment 364660 [details]
Patch
Comment 20 youenn fablet 2019-03-14 16:03:21 PDT
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?
Comment 21 Geoffrey Garen 2019-03-14 16:49:49 PDT
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.
Comment 22 Sihui Liu 2019-03-15 13:07:37 PDT
(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.
Comment 23 Sihui Liu 2019-03-15 14:54:51 PDT
Created attachment 364850 [details]
Patch for landing
Comment 24 WebKit Commit Bot 2019-03-15 15:32:44 PDT
Comment on attachment 364850 [details]
Patch for landing

Clearing flags on attachment: 364850

Committed r243019: <https://trac.webkit.org/changeset/243019>
Comment 25 WebKit Commit Bot 2019-03-15 15:32:46 PDT
All reviewed patches have been landed.  Closing bug.