Regression(r242729): m_origin in IDBDatabaseIdentifier is incorrect

• Component: Website Storage
• Status: RESOLVED FIXED

[Sihui Liu]

...

[Sihui Liu]

Created attachment 452914 [details]
Patch

[Sihui Liu]

Created attachment 453036 [details]
Patch

[youenn fablet]

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
> [phone]  Sihui Liu  <[e-mail]>

Double ChangeLog

> Source/WebKit/ChangeLog:418
> [phone]  Sihui Liu  <[e-mail]>

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

[Sihui Liu]

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
>> [phone]  Sihui Liu  <[e-mail]>
> 
> Double ChangeLog

Will remove.

>> Source/WebKit/ChangeLog:418
>> [phone]  Sihui Liu  <[e-mail]>
> 
> 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.

[Sihui Liu]

Created attachment 453153 [details]
Patch

[Sihui Liu]

Created attachment 453225 [details]
Patch

[Sihui Liu]

Created attachment 453227 [details]
Patch

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/89496178>