[webkit-reviews] review denied: [Bug 203971] Cross-thread version StorageQuotaManager : [Attachment 383972] Patch

Wed Nov 20 12:45:18 PST 2019

Chris Dumez <cdumez at apple.com> has denied Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 203971: Cross-thread version StorageQuotaManager
https://bugs.webkit.org/show_bug.cgi?id=203971

Attachment 383972: Patch

https://bugs.webkit.org/attachment.cgi?id=383972&action=review

--- Comment #14 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 383972
  --> https://bugs.webkit.org/attachment.cgi?id=383972
Patch

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

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:856
> +    if (m_server->requestSpace(m_origin, 0) ==
StorageQuotaManager::Decision::Deny) {

What is this for? Why are we requesting 0 space for deleting something?
Shouldn't deleting something always succeed no matter the quota?

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:940
> +    if (m_server->requestSpace(m_origin, 0) ==
StorageQuotaManager::Decision::Deny) {

Same question.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:1263
> +    if (m_server->requestSpace(m_origin, 0) ==
StorageQuotaManager::Decision::Deny) {

Ditto.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:1280
> +    if (m_server->requestSpace(m_origin, 0) ==
StorageQuotaManager::Decision::Deny) {

Ditto.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:1305
> +    if (m_server->requestSpace(m_origin, 0) ==
StorageQuotaManager::Decision::Deny) {

Ditto.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:1340
> +    if (m_server->requestSpace(m_origin, 0) ==
StorageQuotaManager::Decision::Deny) {

Ditto.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:1378
> +    if (m_server->requestSpace(m_origin, 0) ==
StorageQuotaManager::Decision::Deny) {

Ditto.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:1412
> +    if (m_server->requestSpace(m_origin, 0) ==
StorageQuotaManager::Decision::Deny) {

Ditto.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:1446
> +    if (m_server->requestSpace(m_origin, 0) ==
StorageQuotaManager::Decision::Deny) {

Ditto.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:1605
> +    if (m_server->requestSpace(m_origin, 0) ==
StorageQuotaManager::Decision::Deny) {

Ditto.

> Source/WebCore/Modules/indexeddb/shared/InProcessIDBServer.cpp:86
> +	   storageQuotaManager->requestSpace(spaceRequested, [&result](auto
decision) mutable {

This code looks dangerous, it looks like you're doing an asynchronous operation
to populate result, but then use result synchronously. Even worse, result is
not even initialized. I am assuming requestSpace() works synchronously here?
But I assume it is sometimes async since it takes a lambda instead of returning
the decision synchronously?

> Source/WebCore/Modules/indexeddb/shared/InProcessIDBServer.cpp:104
> +	   storageQuotaManager->requestSpace(spaceRequested, [&result](auto
decision) mutable {

Ditto.

> Source/WebCore/storage/StorageQuotaManager.cpp:50
> +void StorageQuotaManager::requestSpace(uint64_t spaceRequested,
RequestCallback&& callback)

I think this may look clearer if we had 2 versions of this. One that should
only be called from the main thread and another that should only be called from
background threads.
Then the one that is called from a background thread would return the decision
synchronously (via a method return value) and not take in a lambda.

Having a callback that may or may not get called synchronously, and having some
call sites relying on the fact that it gets called synchronously is a foot-gun
and it WILL bite us at some point.

> Source/WebCore/storage/StorageQuotaManager.cpp:64
> +    if (m_quotaCountDownLock.tryLock()) {

What's the reasoning behind this? We do we tryLock and fall back to an async
dispatch. Why is it better that simply lock() with no fallback? Or better than
always dispatch without locking?

> Source/WebCore/storage/StorageQuotaManager.cpp:73
> +    m_callbacks.append(WTFMove(callback));

What is m_callbacks for? Why cannot we pipe the callback though by capturing it
it through the lambdas below?

> Source/WebCore/storage/StorageQuotaManager.cpp:74
> +    m_workQueue->dispatch([this, protectedThis = makeRef(*this),
spaceRequested] {

mutable.

> Source/WebCore/storage/StorageQuotaManager.cpp:76
> +	   callOnMainThread([this, protectedThis = makeRef(*this), decision] {

protectedThis = WTFMove(protectedThis)

> Source/WebCore/storage/StorageQuotaManager.cpp:101
> +    callOnMainThread([this, protectedThis = makeRef(*this), spaceRequested,
&semaphore] {

mutable

> Source/WebCore/storage/StorageQuotaManager.cpp:103
> +	   m_quotaIncreaseRequester(m_quota, m_usage, spaceRequested, [this,
protectedThis = makeRef(*this), &semaphore](Optional<uint64_t> newQuota) {

WTFMove(protectedThis)

> Source/WebCore/storage/StorageQuotaManager.h:40
> +    using UsageGetter = WTF::Function<uint64_t()>;

WTF:: should not be needed.

> Source/WebCore/storage/StorageQuotaManager.h:41
> +    using QuotaIncreaseRequester = WTF::Function<void(uint64_t currentQuota,
uint64_t currentUsage, uint64_t requestedIncrease,
CompletionHandler<void(Optional<uint64_t>)>&&)>;

WTF:: should not be needed.

> Source/WebCore/storage/StorageQuotaManager.h:53
> +    WEBCORE_EXPORT void resetQuotaUpdatedBasedOnUsage();

Can it be named "ForTesting" then?

> Source/WebCore/storage/StorageQuotaManager.h:54
> +    WEBCORE_EXPORT void resetQuota();

ditto?

Then we would not need a comment.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:461
> +    auto[iter, isNewEntry] = m_sessionStorageQuotaManagers.add(sessionID,
nullptr);

I think ensure() with a lambda looks a lot better than adding null and then
retro-actively modify the iterator value.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2222
> +	   storageQuotaManager->requestSpace(spaceRequested, [&result](auto
decision) mutable {

Again, this looks unsafe.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2344
> +void NetworkProcess::resetQuota(PAL::SessionID sessionID,
CompletionHandler<void()>&& completionHandler)

Why is this taking a completion handler? it is synchronous.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2437
> +    idbRootPath = sessionStorageQuotaManager->idbRootPath();

Are we on the main thread here? It would help to have an assertion to document
which thread this is expected to run on. 

My main concern is that you are capturing this string in a lambda below, and
that lambda uses that same thing on a background thread. Our strings are not
thread safe so it would not be safe if we were not on the same background
thread here.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2443
> +	   usage += CacheStorage::Engine::diskUsage(cacheRootPath, origin);

Why the += ? We can initialize usage here.

> Source/WebKit/NetworkProcess/NetworkProcess.h:479
> +	       auto[iter, isNewEntry] = m_storageQuotaManagers.add(origin,
nullptr);

ensure()

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:201
> +    uint64_t directorySize = 0;

We should assert that we are not on the main thread here.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:248
> +    if (!m_networkProcess)

We should add a threading assertion here. I assume we should be on the main
thread since you're using m_networkProcess.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:408
> +    int64_t spaceRequired = 0;

Why? It seems like an unsigned value would be nicer. And the code below should
make sure we don't underflow.

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1372
> +    if (!canSendMessage()) {

Not needed. sendWithAsyncReply() will do the right thing.

> Tools/TestWebKitAPI/Tests/WebCore/StorageQuotaManager.cpp:-69
> -TEST(StorageQuotaManagerTest, Basic)

So we're losing all API test coverage for quota?