Bug 234925 – Manage IndexedDB storage by origin

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sihui Liu]

...

[Sihui Liu]

Created attachment 448525 [details]
WIP

[Sihui Liu]

Created attachment 448895 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/87555014>

[Sihui Liu]

Created attachment 450778 [details]
Patch

[Sihui Liu]

Created attachment 450805 [details]
Patch

[Sihui Liu]

Created attachment 450863 [details]
Patch

[Sihui Liu]

Created attachment 450867 [details]
Patch

[Sihui Liu]

Created attachment 450868 [details]
Patch

[Sihui Liu]

Created attachment 451054 [details]
Patch

[Chris Dumez]

Comment on attachment 451054 [details]
Patch

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

Only a partial review for now as I need to go to a meeting. Will resume later today unless someone else beats me to it.

> Source/WTF/wtf/FileSystem.cpp:663
> +String getSymbolicLinkTarget(const String& symbolicLinkPath)

Why can't you use the existing FileSystem::realPath() function which already resolves symlinks if present?

Also, no "get" prefix please. And we should add API test coverage (we already have plenty for the FileSytem API) if we do really need a new function here for some reason.

> Source/WTF/wtf/FileSystem.cpp:666
> +    if (!std::filesystem::is_symlink(fileSystemPath))

You need to pass an 'ec' parameter to is_symlink() or the function call may throw.

> Source/WTF/wtf/FileSystem.cpp:669
> +    return fromStdFileSystemPath(std::filesystem::read_symlink(fileSystemPath));

You need to pass an 'ec' parameter to std::filesystem::read_symlink() or it may throw exceptions.

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:742
> +void IDBServer::requestSpace(const ClientOrigin& origin, uint64_t taskSize, CompletionHandler<void(bool)>&& completionHandler) WTF_IGNORES_THREAD_SAFETY_ANALYSIS

Since it is synchronous, it is unclear to me why adding a completion handler is better.

> Source/WebCore/Modules/indexeddb/server/IDBServer.h:91
> +    void registerConnection(UniqueIDBDatabaseConnection&) final;

We usually have a `// UniqueIDBDatabaseManager.` comment before the flow of functions to indicate that those are overrides for UniqueIDBDatabaseManager.

> Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.h:43
> +    WEBCORE_EXPORT MemoryIDBBackingStore(const IDBDatabaseIdentifier&);

Can we please mark this as explicit now that it has a single parameter?

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:920
>      filename.replace('.', "%2E");

replaceWithLiteral() would probably be more efficient here.

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:928
> +        return { };

This returns a null String, not an empty string, did you mean to return emptyString() here?

Otherwise, it doesn't match the encodeDatabaseName() function, which is confusing.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:840
> +static int myCount = 0;

Unused? Please remove.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:893
> +        indexKeys = generateIndexKeyMapForValue(globalObject, objectStoreInfo, key, value);

What's the type of indexKeys? Don't we need to crossThreadCopy() it?

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.h:84
> +    using SpaceCheckResult = std::optional<bool>;

Personally, I find std::optional<bool> a little confusing, especially with a typedef. I find it makes the code using it a little opaque. 
It might look better with an enum class that has 3 values?

[Chris Dumez]

Comment on attachment 451054 [details]
Patch

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

r=me with fixes.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseConnection.cpp:51
> +        m_manager = WeakPtr { *manager };

WeakPtr { } is not needed, you can just assign `*manager`.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2212
> +    if (auto* storageManager = session->storageManager())

You are failing to call the completion handler in the else case.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:248
> +    if (auto* storageManager = session->storageManager())

Callback is not getting called in the else case.

> Source/WebKit/NetworkProcess/storage/IDBStorageManager.cpp:45
> +    for (auto name : fileNames) {

auto&, assuming this is a String.

> Source/WebKit/NetworkProcess/storage/IDBStorageManager.cpp:244
> +    if (identifier.isTransient())

Probably can merge with the previous if condition given that the return is the same.

> Source/WebKit/NetworkProcess/storage/IDBStorageManager.h:53
> +    ~IDBStorageManager() final { }

Seem we shouldn't need this?

> Source/WebKit/NetworkProcess/storage/IDBStorageRegistry.h:47
> +    WebCore::IDBServer::IDBConnectionToClient& getOrCreateConnectionToClient(IPC::Connection::UniqueID, WebCore::IDBConnectionIdentifier);

Maybe ensureConnectionToClient() would look better?

> Source/WebKit/NetworkProcess/storage/IDBStorageRegistry.h:51
> +    WebCore::IDBServer::UniqueIDBDatabaseConnection* getConnection(uint64_t identifier);

No "get" prefixes in getters.

> Source/WebKit/NetworkProcess/storage/IDBStorageRegistry.h:54
> +    WebCore::IDBServer::UniqueIDBDatabaseTransaction* getTransaction(WebCore::IDBResourceIdentifier);

ditto.

> Source/WebKit/NetworkProcess/storage/QuotaManager.cpp:59
> +    auto clearIsHandlingRequests = makeScopeExit([&] {

I think this is a clear use case for SetForScope?

SetForScope<bool> isHandlingRequests(m_isHandlingRequests, true);

> Source/WebKit/NetworkProcess/storage/QuotaManager.cpp:83
> +    bool shouldUpdateQuotaBasedonUsage = !m_usage;

OnUsage (case issue)

> Source/WebKit/NetworkProcess/storage/QuotaManager.cpp:125
> +    m_quotaCountdown = 0;

Maybe this could call resetQuotaForTesting()?

> Source/WebKit/NetworkProcess/storage/QuotaManager.h:41
> +    enum class Decision { Deny, Grant };

enum class Decision : bool { Deny, Grant };

> Tools/ChangeLog:17
> [phone]  Sihui Liu  <[e-mail]>

Double changelog issue.

[Sihui Liu]

Comment on attachment 451054 [details]
Patch

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

>> Source/WTF/wtf/FileSystem.cpp:663
>> +String getSymbolicLinkTarget(const String& symbolicLinkPath)
> 
> Why can't you use the existing FileSystem::realPath() function which already resolves symlinks if present?
> 
> Also, no "get" prefix please. And we should add API test coverage (we already have plenty for the FileSytem API) if we do really need a new function here for some reason.

Didn't see that. I will use realPath instead.

>> Source/WTF/wtf/FileSystem.cpp:666
>> +    if (!std::filesystem::is_symlink(fileSystemPath))
> 
> You need to pass an 'ec' parameter to is_symlink() or the function call may throw.

Will remove this change.

>> Source/WTF/wtf/FileSystem.cpp:669
>> +    return fromStdFileSystemPath(std::filesystem::read_symlink(fileSystemPath));
> 
> You need to pass an 'ec' parameter to std::filesystem::read_symlink() or it may throw exceptions.

Will remove this change.

>> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:742
>> +void IDBServer::requestSpace(const ClientOrigin& origin, uint64_t taskSize, CompletionHandler<void(bool)>&& completionHandler) WTF_IGNORES_THREAD_SAFETY_ANALYSIS
> 
> Since it is synchronous, it is unclear to me why adding a completion handler is better.

This is the virtual function in UniqueIDBDatabaseManager; IDBServer just implements the function in a synchronous way now.

>> Source/WebCore/Modules/indexeddb/server/IDBServer.h:91
>> +    void registerConnection(UniqueIDBDatabaseConnection&) final;
> 
> We usually have a `// UniqueIDBDatabaseManager.` comment before the flow of functions to indicate that those are overrides for UniqueIDBDatabaseManager.

Sure.

>> Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.h:43
>> +    WEBCORE_EXPORT MemoryIDBBackingStore(const IDBDatabaseIdentifier&);
> 
> Can we please mark this as explicit now that it has a single parameter?

Sure.

>> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:920
>>      filename.replace('.', "%2E");
> 
> replaceWithLiteral() would probably be more efficient here.

Sure.

>> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:928
>> +        return { };
> 
> This returns a null String, not an empty string, did you mean to return emptyString() here?
> 
> Otherwise, it doesn't match the encodeDatabaseName() function, which is confusing.

Sure, will change to emptyString().

>> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:840
>> +static int myCount = 0;
> 
> Unused? Please remove.

ouch.. will remove

>> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:893
>> +        indexKeys = generateIndexKeyMapForValue(globalObject, objectStoreInfo, key, value);
> 
> What's the type of indexKeys? Don't we need to crossThreadCopy() it?

It's map of IDBKeyDatas. Will add crossThreadCopy().

>> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.h:84
>> +    using SpaceCheckResult = std::optional<bool>;
> 
> Personally, I find std::optional<bool> a little confusing, especially with a typedef. I find it makes the code using it a little opaque. 
> It might look better with an enum class that has 3 values?

Okay, will change it to a enum class.

>> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseConnection.cpp:51
>> +        m_manager = WeakPtr { *manager };
> 
> WeakPtr { } is not needed, you can just assign `*manager`.

Will change.

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2212
>> +    if (auto* storageManager = session->storageManager())
> 
> You are failing to call the completion handler in the else case.

Will add!

>> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:248
>> +    if (auto* storageManager = session->storageManager())
> 
> Callback is not getting called in the else case.

Oops, wil change.

>> Source/WebKit/NetworkProcess/storage/IDBStorageManager.cpp:45
>> +    for (auto name : fileNames) {
> 
> auto&, assuming this is a String.

Okay.

>> Source/WebKit/NetworkProcess/storage/IDBStorageManager.cpp:244
>> +    if (identifier.isTransient())
> 
> Probably can merge with the previous if condition given that the return is the same.

Sure.

>> Source/WebKit/NetworkProcess/storage/IDBStorageManager.h:53
>> +    ~IDBStorageManager() final { }
> 
> Seem we shouldn't need this?

Will remove.

>> Source/WebKit/NetworkProcess/storage/IDBStorageRegistry.h:47
>> +    WebCore::IDBServer::IDBConnectionToClient& getOrCreateConnectionToClient(IPC::Connection::UniqueID, WebCore::IDBConnectionIdentifier);
> 
> Maybe ensureConnectionToClient() would look better?

Sure.

>> Source/WebKit/NetworkProcess/storage/IDBStorageRegistry.h:51
>> +    WebCore::IDBServer::UniqueIDBDatabaseConnection* getConnection(uint64_t identifier);
> 
> No "get" prefixes in getters.

Will remove.

>> Source/WebKit/NetworkProcess/storage/IDBStorageRegistry.h:54
>> +    WebCore::IDBServer::UniqueIDBDatabaseTransaction* getTransaction(WebCore::IDBResourceIdentifier);
> 
> ditto.

Will remove.

>> Source/WebKit/NetworkProcess/storage/QuotaManager.cpp:59
>> +    auto clearIsHandlingRequests = makeScopeExit([&] {
> 
> I think this is a clear use case for SetForScope?
> 
> SetForScope<bool> isHandlingRequests(m_isHandlingRequests, true);

Yes!

>> Source/WebKit/NetworkProcess/storage/QuotaManager.cpp:83
>> +    bool shouldUpdateQuotaBasedonUsage = !m_usage;
> 
> OnUsage (case issue)

Will update.

>> Source/WebKit/NetworkProcess/storage/QuotaManager.cpp:125
>> +    m_quotaCountdown = 0;
> 
> Maybe this could call resetQuotaForTesting()?

Sure.

>> Source/WebKit/NetworkProcess/storage/QuotaManager.h:41
>> +    enum class Decision { Deny, Grant };
> 
> enum class Decision : bool { Deny, Grant };

Sure.

>> Tools/ChangeLog:17
>> [phone]  Sihui Liu  <[e-mail]>
> 
> Double changelog issue.

Will remove.

[Sihui Liu]

Created attachment 451180 [details]
Patch

[Sihui Liu]

Created attachment 451190 [details]
Patch

[Sihui Liu]

Created attachment 451197 [details]
Patch

[Sihui Liu]

Created attachment 451216 [details]
Patch

[Sihui Liu]

Created attachment 451257 [details]
Patch

[Sihui Liu]

Created attachment 451278 [details]
Patch

[Sihui Liu]

Created attachment 451316 [details]
Patch

[Sihui Liu]

Created attachment 451380 [details]
Patch for landing

[EWS]

Committed r289474 (247017@main): <https://commits.webkit.org/247017@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 451380 [details].