Bug 237044 - Regression(r242729): m_origin in IDBDatabaseIdentifier is incorrect
Summary: Regression(r242729): m_origin in IDBDatabaseIdentifier is incorrect
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Website Storage (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sihui Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-02-22 09:56 PST by Sihui Liu
Modified: 2022-02-25 15:47 PST (History)
9 users (show)

See Also:


Attachments
Patch (25.40 KB, patch)
2022-02-22 16:31 PST, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (34.55 KB, patch)
2022-02-23 15:38 PST, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (32.39 KB, patch)
2022-02-24 16:26 PST, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (20.91 KB, patch)
2022-02-25 09:48 PST, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (20.43 KB, patch)
2022-02-25 10:07 PST, 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 Sihui Liu 2022-02-22 09:56:41 PST
...
Comment 1 Sihui Liu 2022-02-22 16:31:08 PST
Created attachment 452914 [details]
Patch
Comment 2 Sihui Liu 2022-02-23 15:38:28 PST
Created attachment 453036 [details]
Patch
Comment 3 youenn fablet 2022-02-24 05:32:47 PST
Comment on attachment 453036 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        update databaseDirectoryRelativeToRoot function to make it find the correct storage path.

It seems we should keep databaseDirectoryRelativeToRoot to provide version/top/opening.

> Source/WebCore/ChangeLog:900
> +2022-02-22  Sihui Liu  <sihui_liu@apple.com>

Double ChangeLog

> Source/WebKit/ChangeLog:418
> +2022-02-22  Sihui Liu  <sihui_liu@apple.com>

Double change log

> Source/WebCore/Modules/indexeddb/IDBDatabaseIdentifier.cpp:38
> +    , m_origin { WTFMove(mainFrameOrigin), WTFMove(openingOrigin) }

Thanks for fixing this!

> Source/WebCore/Modules/indexeddb/IDBDatabaseIdentifier.cpp:61
>  String IDBDatabaseIdentifier::databaseDirectoryRelativeToRoot(const SecurityOriginData& topLevelOrigin, const SecurityOriginData& openingOrigin, const String& rootDirectory, const String& versionString)

Can we pass a const ClientOrigin& here?

> Source/WebCore/Modules/indexeddb/IDBDatabaseIdentifier.cpp:70
> +    return FileSystem::pathByAppendingComponent(openingFrameDirectory, topLevelOrigin.databaseIdentifier());

Shouldn't we keep version/top/iframe structure and do the migration of folders version/iframe/top?

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1613
> +        FileSystem::closeFile(handle);

It could be a one liner.

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1631
>  void NetworkProcessProxy::createSymLinkForFileUpgrade(const String& indexedDatabaseDirectory)

Maybe we should rename to upgradeIfNeeded?

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1637
> +    if (!FileSystem::fileExists(oldVersionDirectory))

It does not seem consistent with: "If IndexedDB does not have version directory, the origin directory structure is correct".
So we are moving the correct ancient to the incorrect but consistent with v0/v1? Should we do the reverse and move back v0/v1 to correct?

Also, given reverseOriginForDirectoryIfPossible is destructive, we probably only want to execute it once.
I do not see what guarantees that oldVersionDirectory will not exist anymore after calling reverseOriginForDirectoryIfPossible.
Is it overkill to increase the IDB database version?

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1642
>          FileSystem::createSymbolicLink(indexedDatabaseDirectory, oldVersionDirectory);

Usually we do only file changes in background thread. I guess this is fine since it will only happen once.

> Source/WebKit/UIProcess/WebPageProxy.cpp:1162
> +    

Unnecessary.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBPersistence.mm:378
> +    // Database file is moved to [IndexedDBDirectory]/v1/[OpeninigOrigin]/[TopOrigin]/.

s/OpeninigOrigin/OpeningOrigin
Comment 4 Sihui Liu 2022-02-24 14:09:57 PST
Comment on attachment 453036 [details]
Patch

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

>> Source/WebCore/ChangeLog:17
>> +        update databaseDirectoryRelativeToRoot function to make it find the correct storage path.
> 
> It seems we should keep databaseDirectoryRelativeToRoot to provide version/top/opening.

Sure, I will make a change.

>> Source/WebCore/ChangeLog:900
>> +2022-02-22  Sihui Liu  <sihui_liu@apple.com>
> 
> Double ChangeLog

Will remove.

>> Source/WebKit/ChangeLog:418
>> +2022-02-22  Sihui Liu  <sihui_liu@apple.com>
> 
> Double change log

Wil remove.

>> Source/WebCore/Modules/indexeddb/IDBDatabaseIdentifier.cpp:38
>> +    , m_origin { WTFMove(mainFrameOrigin), WTFMove(openingOrigin) }
> 
> Thanks for fixing this!

Np!

>> Source/WebCore/Modules/indexeddb/IDBDatabaseIdentifier.cpp:61
>>  String IDBDatabaseIdentifier::databaseDirectoryRelativeToRoot(const SecurityOriginData& topLevelOrigin, const SecurityOriginData& openingOrigin, const String& rootDirectory, const String& versionString)
> 
> Can we pass a const ClientOrigin& here?

Sure.

>> Source/WebCore/Modules/indexeddb/IDBDatabaseIdentifier.cpp:70
>> +    return FileSystem::pathByAppendingComponent(openingFrameDirectory, topLevelOrigin.databaseIdentifier());
> 
> Shouldn't we keep version/top/iframe structure and do the migration of folders version/iframe/top?

Sure, I made a mistake in thinking that v0's origins are correct.
My new plan is to reverse the wrong origins in v1, and we can keep the structure here.

>> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1613
>> +        FileSystem::closeFile(handle);
> 
> It could be a one liner.

Sure.

>> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1631
>>  void NetworkProcessProxy::createSymLinkForFileUpgrade(const String& indexedDatabaseDirectory)
> 
> Maybe we should rename to upgradeIfNeeded?

We don't actually upgrade here, but just add a new v0 directory; we do the upgrade (adding v1/ and moving files to v1/) in IDBStorageManager::migrateOriginData.

>> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1637
>> +    if (!FileSystem::fileExists(oldVersionDirectory))
> 
> It does not seem consistent with: "If IndexedDB does not have version directory, the origin directory structure is correct".
> So we are moving the correct ancient to the incorrect but consistent with v0/v1? Should we do the reverse and move back v0/v1 to correct?
> 
> Also, given reverseOriginForDirectoryIfPossible is destructive, we probably only want to execute it once.
> I do not see what guarantees that oldVersionDirectory will not exist anymore after calling reverseOriginForDirectoryIfPossible.
> Is it overkill to increase the IDB database version?

I made a mistake here by thinking only ancient files have correct origin: actually ancient files and v0/ files are correct; only v1/ files are not. I will make the according changes.
Yes, it seems overkill to increase the version (v0, v1, v2), because we are about to migrate data to general storage directory.

>> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1642
>>          FileSystem::createSymbolicLink(indexedDatabaseDirectory, oldVersionDirectory);
> 
> Usually we do only file changes in background thread. I guess this is fine since it will only happen once.

I am going to move this to a background thread in network process so the migration can be done in both WebKit and WebKitLegacy.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:1162
>> +    
> 
> Unnecessary.

Will remove.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBPersistence.mm:378
>> +    // Database file is moved to [IndexedDBDirectory]/v1/[OpeninigOrigin]/[TopOrigin]/.
> 
> s/OpeninigOrigin/OpeningOrigin

Will change.
Comment 5 Sihui Liu 2022-02-24 16:26:18 PST
Created attachment 453153 [details]
Patch
Comment 6 Sihui Liu 2022-02-25 09:48:09 PST
Created attachment 453225 [details]
Patch
Comment 7 Sihui Liu 2022-02-25 10:07:27 PST
Created attachment 453227 [details]
Patch
Comment 8 EWS 2022-02-25 15:46:56 PST
Committed r290532 (247812@main): <https://commits.webkit.org/247812@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453227 [details].
Comment 9 Radar WebKit Bug Importer 2022-02-25 15:47:21 PST
<rdar://problem/89496178>