Bug 203971 – Cross-thread version StorageQuotaManager

• Component: New Bugs
• Status: RESOLVED FIXED

[Sihui Liu]

...

[Sihui Liu]

Created attachment 383062 [details]
WIP

[Sihui Liu]

Created attachment 383365 [details]
WIP2

[Sihui Liu]

Created attachment 383374 [details]
WIP3

[Sihui Liu]

Created attachment 383406 [details]
Patch

[Sihui Liu]

Created attachment 383414 [details]
Patch

[Sihui Liu]

Created attachment 383482 [details]
Patch

[Sihui Liu]

Created attachment 383483 [details]
Patch

[Sihui Liu]

Created attachment 383486 [details]
Patch

[Sihui Liu]

Created attachment 383557 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/57290349>

[youenn fablet]

Comment on attachment 383557 [details]
Patch

Didn't have the time to fully review the patch.
Below some feedback.
It seems this patch is fixing some existing bugs, maybe they should be fixed as their own patch.

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

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:694
> +    RefPtr<StorageQuotaManager> storageQuotaManager = m_quotaManagerGetter(origin);

auto I guess.
But why do we need to have a ref it?
Can we simply get a StorageQuotaManager&
Or could we just write something like:
return m_quotaManagerGetter(origin, taskSize)
And default implementation of the lambda would return Deny.

> Source/WebCore/Modules/indexeddb/server/IDBServer.h:60
> +    using StorageQuotaManagerGetter = WTF::Function<RefPtr<StorageQuotaManager>(const ClientOrigin&)>;

No need for WTF::?

> Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.cpp:198
> +    m_objectStoresByName.set(newName, objectStore);

This probably deserves its own patch. Can we split it?

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:811
> +    auto decision = m_server->requestSpace(origin, taskSize);

Could be a one-liner with if

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:936
> +    postDatabaseTask(createCrossThreadTask(*this, &UniqueIDBDatabase::performClearObjectStore, m_identifier.origin(), callbackID, transaction.info().identifier(), objectStoreIdentifier));

UniqueIDBDatabase origin is never changing so we should not need to pass it cross thread every time we need to do a hop.
Can we store it somewhere so that we can use it directly in database thread?

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:-1517
> -    uint64_t count;

Can we keep count here?

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

I am not sure we want one version of the API that is main thread and callback based and another version that is background thread but blocking.
We already have the infrastructure
Can we expose a single requestSpace method?
I guess this would mean that the background thread version would not block and would use a callback version as well.

> Source/WebKit/UIProcess/API/C/WKContextPrivate.h:129
> +WK_EXPORT void WKContextResetQuota(WKContextRef);

You can talk with Alex but I guess he might prefer that this API goes through WebsiteDataStore.

> Source/WebKit/UIProcess/WebProcessPool.cpp:556
> +        parameters.defaultDataStoreParameters.cacheStorageDirectory = cacheStorageDirectory;

WTFMove

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2088
> +        parameters.cacheStorageDirectory = cacheStorageDirectory;

WTFMove() I guess

[Sihui Liu]

Comment on attachment 383557 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:694
>> +    RefPtr<StorageQuotaManager> storageQuotaManager = m_quotaManagerGetter(origin);
> 
> auto I guess.
> But why do we need to have a ref it?
> Can we simply get a StorageQuotaManager&
> Or could we just write something like:
> return m_quotaManagerGetter(origin, taskSize)
> And default implementation of the lambda would return Deny.

ref is used to make sure storageQuotaManager is not destroyed when we use it, since we may create/destroy it from different threads.

We can probably replace this m_quotaManagerGetter with some m_spaceRequester I think.

>> Source/WebCore/Modules/indexeddb/server/IDBServer.h:60
>> +    using StorageQuotaManagerGetter = WTF::Function<RefPtr<StorageQuotaManager>(const ClientOrigin&)>;
> 
> No need for WTF::?

Will remove.

>> Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.cpp:198
>> +    m_objectStoresByName.set(newName, objectStore);
> 
> This probably deserves its own patch. Can we split it?

Yes, will remove.

>> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:811
>> +    auto decision = m_server->requestSpace(origin, taskSize);
> 
> Could be a one-liner with if

Okay.

>> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:936
>> +    postDatabaseTask(createCrossThreadTask(*this, &UniqueIDBDatabase::performClearObjectStore, m_identifier.origin(), callbackID, transaction.info().identifier(), objectStoreIdentifier));
> 
> UniqueIDBDatabase origin is never changing so we should not need to pass it cross thread every time we need to do a hop.
> Can we store it somewhere so that we can use it directly in database thread?

Sure. Will change this.

>> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:-1517
>> -    uint64_t count;
> 
> Can we keep count here?

Sure.

>> Source/WebCore/storage/StorageQuotaManager.cpp:50
>> +void StorageQuotaManager::requestSpaceOnMainThread(uint64_t spaceRequested, RequestCallback&& callback)
> 
> I am not sure we want one version of the API that is main thread and callback based and another version that is background thread but blocking.
> We already have the infrastructure
> Can we expose a single requestSpace method?
> I guess this would mean that the background thread version would not block and would use a callback version as well.

Basically the main-thread case is just a special case of the background thread case. The WorkQueue of StorageQuotaManager is requesting on behalf the main thread.

Our idea here is to synchronously getting the request decision: simply holding the StorageQuotaManager lock until one request is done. 

The reason I didn't use the callback version for requestSpace on background thread is it's making the code much cleaner: we don't need all this *AfterQuotaCheck functions or ref-ing objects to make sure they are alive when we do the callback. Also we don't need to maintain the StorageQuotaManager states (e.g. WaitingForUIResponse).

We can expose one requestSpace method and choose which internal function to use based on isMainThread();

>> Source/WebKit/UIProcess/API/C/WKContextPrivate.h:129
>> +WK_EXPORT void WKContextResetQuota(WKContextRef);
> 
> You can talk with Alex but I guess he might prefer that this API goes through WebsiteDataStore.

Sure.

>> Source/WebKit/UIProcess/WebProcessPool.cpp:556
>> +        parameters.defaultDataStoreParameters.cacheStorageDirectory = cacheStorageDirectory;
> 
> WTFMove

cacheStorageDirectory is used on the line below... but I guess I can move it forward

>> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2088
>> +        parameters.cacheStorageDirectory = cacheStorageDirectory;
> 
> WTFMove() I guess

Sure.

[Sihui Liu]

Created attachment 383972 [details]
Patch

[Chris Dumez]

Comment on attachment 383972 [details]
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?

[Sihui Liu]

(In reply to Chris Dumez from comment #14)
> Comment on attachment 383972 [details]
> 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?
> 
Request 0 space is a way to protect the order of the tasks. If we don't do this, when a delete task is scheduled after a put task, the delete task would run before the put because it does not need to wait until space request finishes.

I should probably give this a better name.

> 
> > 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?
> 

Will change to use requestSpaceOnMainThread which returns result asynchronously and requestSpaceOnBackground which returns result synchronously. I was trying to provide one unified interface requestSpace() as comment above suggested.

> 
> > 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.
> 
Got it.

> > 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?
> 

1. requestSpaceOnBackgroundThread would schedule a task to main thread(to ask UI process for quota increase) and wait for callback of the task. This would be a deadlock if requestSpaceOnMainThread holds the lock. 

2. we don't want to the main thread to wait on the lock when the lock can be held by a background thread for a long time (e.g. waiting for response of UI process about quota increase). tryLock will let the main thread give up as soon as possible.

3. we can grant space request on main thread if m_quotaCountDown(estimated space available) allows, because then we don't need to stat file system for disk usage or wait for UI process's response for quota increase.


> > 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?
> 

We can pipe the callback. I was thinking that since we are passing the call back from main to work queue, and then to main, it may be enough just store it. Will change this.

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

> > 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
> 
Will add.

> > 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.
> 
Will remove.

> > 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.
> 
Will remove.

> > Source/WebCore/storage/StorageQuotaManager.h:53
> > +    WEBCORE_EXPORT void resetQuotaUpdatedBasedOnUsage();
> 
> Can it be named "ForTesting" then?
> 
Will change.

> > Source/WebCore/storage/StorageQuotaManager.h:54
> > +    WEBCORE_EXPORT void resetQuota();
> 
> ditto?
> 
> Then we would not need a comment.
> 
Will change.

> > 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.
> 
Will change.

> > 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.
> 
Will change.

> > 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.
> 
We are not necessarily on the main thread here. isolatedCopy?

> > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2443
> > +        usage += CacheStorage::Engine::diskUsage(cacheRootPath, origin);
> 
> Why the += ? We can initialize usage here.
> 
True.

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

> > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:201
> > +    uint64_t directorySize = 0;
> 
> We should assert that we are not on the main thread here.
> 
Will add.

> > 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.
> 
Will add.

> > 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.
> 
A layout test (forgot which one..) shows we have an underflow and spaceRequired becomes max value of uint64_t in that case. I probably should file another bug for this. 

> > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1372
> > +    if (!canSendMessage()) {
> 
> Not needed. sendWithAsyncReply() will do the right thing.
> 
Will remove.

> > Tools/TestWebKitAPI/Tests/WebCore/StorageQuotaManager.cpp:-69
> > -TEST(StorageQuotaManagerTest, Basic)
> 
> So we're losing all API test coverage for quota?
This one is a unit test to ensure correct internal behavior (correct state change on different cases) of StorageQuotaManager. Since StorageQuotaManager does not maintain any state know, we don't need this test.

For correct quota behavior, we have layout tests like:
LayoutTests/storage/indexeddb/storage-limit.html
LayoutTests/http/wpt/cache-storage/cache-quota.any.html
or API test in StorageQuota.mm

[Sihui Liu]

Created attachment 383990 [details]
Patch

[Sihui Liu]

(In reply to Sihui Liu from comment #15)
> (In reply to Chris Dumez from comment #14)
> > Comment on attachment 383972 [details]
> > 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?
> > 
> Request 0 space is a way to protect the order of the tasks. If we don't do
> this, when a delete task is scheduled after a put task, the delete task
> would run before the put because it does not need to wait until space
> request finishes.
> 
> I should probably give this a better name.
> 

Never mind. We need this because requestSpace was on the main thread. Now the requestSpace and task are performed on the same thread, so there will be no ordering issue. Removed.

> A layout test (forgot which one..) shows we have an underflow and
> spaceRequired becomes max value of uint64_t in that case. I probably should
> file another bug for this. 
> 

Looks like the estimated space increase could be less than 0 (usage decreases); code added to handle the underflow case.

[Sihui Liu]

Created attachment 384062 [details]
Patch

[Sihui Liu]

Created attachment 384064 [details]
Patch

[Chris Dumez]

Ok, taking another look

[Chris Dumez]

Comment on attachment 384064 [details]
Patch

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

r=me with changes

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:701
> +    auto oldVersionOriginDirectory = IDBDatabaseIdentifier::databaseDirectoryRelativeToRoot(origin.topOrigin, origin.clientOrigin, rootDirectory, "v0");

"v0"_str

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:702
> +    auto newVersionOriginDirectory = IDBDatabaseIdentifier::databaseDirectoryRelativeToRoot(origin.topOrigin, origin.clientOrigin, rootDirectory, "v1");

"v1"_str

> Source/WebCore/Modules/indexeddb/server/IDBServer.h:114
> +    WEBCORE_EXPORT static uint64_t diskUsage(const String&, const ClientOrigin&);

I don't think we should omit the name of the first parameter since it is not obvious (to me at least).

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:1278
> +    

Should probably not add spaces.

> Source/WebCore/Modules/indexeddb/shared/InProcessIDBServer.cpp:64
> +RefPtr<StorageQuotaManager> InProcessIDBServer::quotaManager(const ClientOrigin& origin)

Why this change? It does not look like you are transferring ownership to the caller. The caller can always ref it if it needs to but I don't think there is any need to return a RefPtr<> here.

> Source/WebCore/Modules/indexeddb/shared/InProcessIDBServer.cpp:80
> +    m_server = IDBServer::IDBServer::create(sessionID, [this, weakThis = makeWeakPtr(*this)](const ClientOrigin& origin, uint64_t spaceRequested) mutable {

I think this should stay in the initializer list to that the member can stay a Ref<>. You can always add a utility function to construct the IDBServer and call it in the initializer list so that it looks nice.

> Source/WebCore/Modules/indexeddb/shared/InProcessIDBServer.cpp:91
> +    m_server = IDBServer::IDBServer::create(sessionID, databaseDirectoryPath, [this, weakThis = makeWeakPtr(*this)](const ClientOrigin& origin, uint64_t spaceRequested) mutable {

ditto.

> Source/WebCore/Modules/indexeddb/shared/InProcessIDBServer.h:66
> +    IDBServer::IDBServer& server() { return *m_server; }

This does not looks super safe.

> Source/WebCore/Modules/indexeddb/shared/InProcessIDBServer.h:67
> +    IDBServer::IDBServer& idbServer() { return *m_server; }

Not quite sure why we have 2 differently named methods returning the same thing but technically this is not new in your patch. Seems very odd though...

> Source/WebCore/Modules/indexeddb/shared/InProcessIDBServer.h:133
> +    RefPtr<IDBServer::IDBServer> m_server;

I think this should stay a Ref<> since it cannot be null. It would make your getters look safer above.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:461
> +    auto [iter, isNewEntry] = m_sessionStorageQuotaManagers.ensure(sessionID, [defaultQuota, defaultThirdPartyQuota, cacheRootPath] {

&cacheRootPath

> Source/WebKit/NetworkProcess/NetworkProcess.h:477
> +        RefPtr<WebCore::StorageQuotaManager> ensureOriginStorageQuotaManager(WebCore::ClientOrigin origin, uint64_t quota, WebCore::StorageQuotaManager::UsageGetter&& usageGetter, WebCore::StorageQuotaManager::QuotaIncreaseRequester&& quotaIncreaseRequester)

Seems like this should return a Ref<>, not a RefPtr<>

[Sihui Liu]

Created attachment 384192 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 384192 [details]
Patch for landing

Clearing flags on attachment: 384192

Committed r252805: <https://trac.webkit.org/changeset/252805>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This caused flaky failures on several tests.

http/wpt/cache-storage/cache-quota-add.any.html
http/wpt/cache-storage/cache-quota-after-restart.any.html
http/wpt/cache-storage/cache-quota.any.html

https://build.webkit.org/results/Apple%20iPadOS%2013%20Simulator%20Release%20WK2%20(Tests)/r252835%20(816)/results.html

This also caused ASan crashes on what most or all cache storage tests.

==34492==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000019091 at pc 0x000107dbf668 bp 0x70000afd9790 sp 0x70000afd8f50
READ of size 2 at 0x602000019091 thread T8
    #0 0x107dbf667 in wrap_strlen (libclang_rt.asan_iossim_dynamic.dylib:x86_64+0x14667)
    #1 0x119684f39 in WTF::String::fromUTF8(unsigned char const*) (JavaScriptCore:x86_64+0x192f39)
    #2 0x110be4514 in WebKit::CacheStorage::Engine::readSizeFile(WTF::String const&) (WebKit:x86_64+0x638514)
    #3 0x110be3ff9 in WebKit::CacheStorage::Engine::diskUsage(WTF::String const&, WebCore::ClientOrigin const&) (WebKit:x86_64+0x637ff9)
    #4 0x110a56888 in WebKit::NetworkProcess::storageQuotaManager(PAL::SessionID, WebCore::ClientOrigin const&)::$_68::operator()() const (WebKit:x86_64+0x4aa888)
    #5 0x122f08076 in WebCore::StorageQuotaManager::requestSpaceOnBackgroundThread(unsigned long long) (WebCore:x86_64+0x49ae076)
    #6 0x122f0f2e2 in WebCore::StorageQuotaManager::requestSpaceOnMainThread(unsigned long long, WTF::CompletionHandler<void (WebCore::StorageQuotaManager::Decision)>&&)::$_0::operator()() (WebCore:x86_64+0x49b52e2)
    #7 0x107deb65a in __wrap_dispatch_async_block_invoke (libclang_rt.asan_iossim_dynamic.dylib:x86_64+0x4065a)
    #8 0x7fff53ad0950 in _dispatch_call_block_and_release (libdispatch.dylib:x86_64+0x1950)
    #9 0x7fff53ad18ca in _dispatch_client_callout (libdispatch.dylib:x86_64+0x28ca)
    #10 0x7fff53ad760b in _dispatch_lane_serial_drain (libdispatch.dylib:x86_64+0x860b)
    #11 0x7fff53ad8043 in _dispatch_lane_invoke (libdispatch.dylib:x86_64+0x9043)
    #12 0x7fff53ae20c3 in _dispatch_workloop_worker_thread (libdispatch.dylib:x86_64+0x130c3)
    #13 0x7fff53cf671a in _pthread_wqthread (libsystem_pthread.dylib:x86_64+0x271a)
    #14 0x7fff53cf657a in start_wqthread (libsystem_pthread.dylib:x86_64+0x257a)

[WebKit Commit Bot]

Re-opened since this is blocked by bug 204553

[Alexey Proskuryakov]

Rolled back in r252839.

[Sihui Liu]

Created attachment 384667 [details]
Patch

[Chris Dumez]

Comment on attachment 384667 [details]
Patch

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

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:579
> +    return String::fromUTF8(buffer.data(), totalBytesRead).toUInt64Strict();

Why construct a String? Could we use charactersToUInt64(buffer.data(), totalBytesRead) ?

[Sihui Liu]

Comment on attachment 384667 [details]
Patch

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

>> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:579
>> +    return String::fromUTF8(buffer.data(), totalBytesRead).toUInt64Strict();
> 
> Why construct a String? Could we use charactersToUInt64(buffer.data(), totalBytesRead) ?

Yes, will change to charactersToUInt64Strict.

[Sihui Liu]

Created attachment 384671 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 384671 [details]
Patch

Clearing flags on attachment: 384671

Committed r253025: <https://trac.webkit.org/changeset/253025>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.