Bug 190598 – Add a storage limit for IndexedDB

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

[Sihui Liu]

IndexedDB doesn't have a storage limit now. We need to set a quota for IndexedDB so it won't grow unboundedly.

[Sihui Liu]

Created attachment 352368 [details]
Patch

[Sihui Liu]

<rdar://problem/44654715>

[EWS Watchlist]

Comment on attachment 352368 [details]
Patch

Attachment 352368 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9598360

New failing tests:
storage/indexeddb/key-type-array.html
storage/indexeddb/prefetch-invalidation.html
storage/indexeddb/modern/idbkey-array-equality.html

[EWS Watchlist]

Created attachment 352394 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Don Olmstead]

Comment on attachment 352368 [details]
Patch

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

Informal review of the code.

Some explanation on how sqlite3 would be nice. Also WPE seems to not be happy about the unsigned long long so maybe using uint64_t would be a better choice?

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:844
> +    unsigned long long diskFreeSpaceSize = 0;
> +    WebCore::FileSystem::getVolumeFreeSpace(m_absoluteDatabaseDirectory, diskFreeSpaceSize);
> +    diskFreeSpaceSize *= 0.5;

Better name? This isn't the free disk space after this divide. Also you're converting an int to float here.

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:846
> +    return diskFreeSpaceSize < MB * 500 ? diskFreeSpaceSize : MB * 500;

Could this be configurable?

Also could use a min here rather than a ternary.

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:857
> +    unsigned long long databaseFileSize = SQLiteFileSystem::getDatabaseFileSize(fullDatabasePath());
> +    unsigned long long quota = defaultQuotaForOrigin();
> +    for (auto& directory : FileSystem::listDirectory(m_absoluteDatabaseDirectory, "*")) {
> +        for (auto& file : FileSystem::listDirectory(directory, "*.sqlite3"_s))
> +            diskUsage += SQLiteFileSystem::getDatabaseFileSize(file);
> +    }

Can you add some kind of explanation here in the code as to what the logic is? Its not clear unless you know about sqlite3's behavior.

[Sihui Liu]

Created attachment 352417 [details]
Patch

[Sihui Liu]

(In reply to Don Olmstead from comment #5)
> Comment on attachment 352368 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=352368&action=review
> 
> Informal review of the code.
> 
Thanks for the review!

> Some explanation on how sqlite3 would be nice. Also WPE seems to not be
> happy about the unsigned long long so maybe using uint64_t would be a better
> choice?
> 
What do you think I should explain here? The error code?

Will change it to uint64_t.

> > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:844
> > +    unsigned long long diskFreeSpaceSize = 0;
> > +    WebCore::FileSystem::getVolumeFreeSpace(m_absoluteDatabaseDirectory, diskFreeSpaceSize);
> > +    diskFreeSpaceSize *= 0.5;
> 
> Better name? This isn't the free disk space after this divide. Also you're
> converting an int to float here.
> 
Sure.

> > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:846
> > +    return diskFreeSpaceSize < MB * 500 ? diskFreeSpaceSize : MB * 500;
> 
> Could this be configurable?
> 
We could make this configurable, but it seems we didn't do the configuration for any storage type before. The usual way we do is set a small default limit and ask the client for permission to increase the quota. This could be tricky for IndexedDB as it's totally async, so I only introduce a preliminary hard-coded limit here. The goal of this patch is to solve the problem where IDB eats up all storage space.

> Also could use a min here rather than a ternary.
> 
Yes!

> > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:857
> > +    unsigned long long databaseFileSize = SQLiteFileSystem::getDatabaseFileSize(fullDatabasePath());
> > +    unsigned long long quota = defaultQuotaForOrigin();
> > +    for (auto& directory : FileSystem::listDirectory(m_absoluteDatabaseDirectory, "*")) {
> > +        for (auto& file : FileSystem::listDirectory(directory, "*.sqlite3"_s))
> > +            diskUsage += SQLiteFileSystem::getDatabaseFileSize(file);
> > +    }
> 
> Can you add some kind of explanation here in the code as to what the logic
> is? Its not clear unless you know about sqlite3's behavior.
Sure. The code above is just summing all database files in the same directory(same origin) as the current database, to calculate the disk usage of the origin.

[EWS Watchlist]

Comment on attachment 352417 [details]
Patch

Attachment 352417 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9608042

New failing tests:
storage/indexeddb/key-type-array.html
storage/indexeddb/prefetch-invalidation.html
storage/indexeddb/modern/idbkey-array-equality.html

[EWS Watchlist]

Created attachment 352429 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

[Chris Dumez]

Comment on attachment 352417 [details]
Patch

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

This needs a test.

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:842
> +    uint64_t diskFreeSpaceSize = 0;

Can you please ASSERT(!isMainThread()); since you're doing file system operations?

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:844
> +    return std::min(diskFreeSpaceSize * 0.5, MB * 500.0);

I think 500 * MB would look less backward. Also, I think I would use a global constant for this magic value:
const uint64_t maximumQuota = 500 * MB;
return std::min(diskFreeSpaceSize / 2, maximumQuota);

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:849
> +    // The maximum size for one database file is the quota for its origin, minus size of all databases within that origin,

Can you please ASSERT(!isMainThread()); since you're doing file system operations?

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:851
> +    uint64_t diskUsage = 0;

I think it would look better if we moved this one right before the for loop.

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:859
> +

ASSERT(diskUsage >= databaseFileSize);

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:860
> +    if (quota < diskUsage || quota < databaseFileSize)

If my assertion above is correct, then checking `quota < diskUsage` seems unnecessary, maybe I am missing something?

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:863
> +    return quota - diskUsage + databaseFileSize;

uint64_t maximumSize = quota - diskUsage + databaseFileSize;
ASSERT(maximumSize >= 0);
return maximumSize;

BTW, I am not sure this assertion holds, what guarantees that this returns a positive value? Quota is a factor of free disk space and therefore may become really small.

I think the logic in this method needs to be clarified and be made more robust.

[Sihui Liu]

Created attachment 352528 [details]
Patch

[Sihui Liu]

Comment on attachment 352417 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:842
>> +    uint64_t diskFreeSpaceSize = 0;
> 
> Can you please ASSERT(!isMainThread()); since you're doing file system operations?

Okay.

>> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:844
>> +    return std::min(diskFreeSpaceSize * 0.5, MB * 500.0);
> 
> I think 500 * MB would look less backward. Also, I think I would use a global constant for this magic value:
> const uint64_t maximumQuota = 500 * MB;
> return std::min(diskFreeSpaceSize / 2, maximumQuota);

Got.

>> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:849
>> +    // The maximum size for one database file is the quota for its origin, minus size of all databases within that origin,
> 
> Can you please ASSERT(!isMainThread()); since you're doing file system operations?

Okay.

>> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:851
>> +    uint64_t diskUsage = 0;
> 
> I think it would look better if we moved this one right before the for loop.

Okay.

>> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:859
>> +
> 
> ASSERT(diskUsage >= databaseFileSize);

Okay.

>> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:860
>> +    if (quota < diskUsage || quota < databaseFileSize)
> 
> If my assertion above is correct, then checking `quota < diskUsage` seems unnecessary, maybe I am missing something?

Quota could be smaller than diskUsage. Say free disk space is 200MB, and disk usage for the origin is 300MB. Since we don't have eviction now, we should return the db file's original size.

I should remove quota < databaseFileSize here.

>> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:863
>> +    return quota - diskUsage + databaseFileSize;
> 
> uint64_t maximumSize = quota - diskUsage + databaseFileSize;
> ASSERT(maximumSize >= 0);
> return maximumSize;
> 
> BTW, I am not sure this assertion holds, what guarantees that this returns a positive value? Quota is a factor of free disk space and therefore may become really small.
> 
> I think the logic in this method needs to be clarified and be made more robust.

The prior IF check make sure the return value won't be less than databaseFileSize.

[EWS Watchlist]

Comment on attachment 352528 [details]
Patch

Attachment 352528 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9629966

New failing tests:
storage/indexeddb/storage-limit.html
storage/indexeddb/modern/idbkey-array-equality-private.html
storage/indexeddb/key-type-array-private.html
storage/indexeddb/prefetch-invalidation-private.html

[EWS Watchlist]

Created attachment 352534 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 352528 [details]
Patch

Attachment 352528 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9630098

New failing tests:
storage/indexeddb/storage-limit.html
storage/indexeddb/modern/idbkey-array-equality-private.html
storage/indexeddb/key-type-array-private.html
storage/indexeddb/prefetch-invalidation-private.html

[EWS Watchlist]

Created attachment 352538 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 352528 [details]
Patch

Attachment 352528 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9630106

New failing tests:
storage/indexeddb/storage-limit.html
storage/indexeddb/key-type-array.html
storage/indexeddb/modern/idbkey-array-equality-private.html
storage/indexeddb/key-type-array-private.html
storage/indexeddb/prefetch-invalidation-private.html

[EWS Watchlist]

Created attachment 352540 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 352528 [details]
Patch

Attachment 352528 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9630068

New failing tests:
storage/indexeddb/storage-limit.html
storage/indexeddb/modern/idbkey-array-equality-private.html
storage/indexeddb/key-type-array-private.html
storage/indexeddb/prefetch-invalidation-private.html

[EWS Watchlist]

Created attachment 352542 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 352528 [details]
Patch

Attachment 352528 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9630093

New failing tests:
storage/indexeddb/storage-limit.html
storage/indexeddb/modern/idbkey-array-equality-private.html
storage/indexeddb/key-type-array-private.html
storage/indexeddb/prefetch-invalidation-private.html

[EWS Watchlist]

Created attachment 352543 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Chris Dumez]

Comment on attachment 352528 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        If the size of free disk space is over 100 MB, the default limit is 50 MB; otherwise it is 

Does it ask the user for more space before failing (similarly to WebStorage)? I do not think we should land such a low limit without allowing the user to override, this has the potential to cause Web breakage without the user being able to do anything about it.

[Sihui Liu]

Created attachment 352575 [details]
Patch

[Sihui Liu]

(In reply to Chris Dumez from comment #23)
> Comment on attachment 352528 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=352528&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        If the size of free disk space is over 100 MB, the default limit is 50 MB; otherwise it is 
> 
> Does it ask the user for more space before failing (similarly to
> WebStorage)? I do not think we should land such a low limit without allowing
> the user to override, this has the potential to cause Web breakage without
> the user being able to do anything about it.

No, it doesn't.

I thought 500 MB may be too much for a mobile app. But yes, in case it breaks any existing app, I changed it back to 500 MB.

[Chris Dumez]

(In reply to Sihui Liu from comment #25)
> (In reply to Chris Dumez from comment #23)
> > Comment on attachment 352528 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=352528&action=review
> > 
> > > Source/WebCore/ChangeLog:12
> > > +        If the size of free disk space is over 100 MB, the default limit is 50 MB; otherwise it is 
> > 
> > Does it ask the user for more space before failing (similarly to
> > WebStorage)? I do not think we should land such a low limit without allowing
> > the user to override, this has the potential to cause Web breakage without
> > the user being able to do anything about it.
> 
> No, it doesn't.
> 
> I thought 500 MB may be too much for a mobile app. But yes, in case it
> breaks any existing app, I changed it back to 500 MB.

As you going to ask the user in a follow-up patch?

[EWS Watchlist]

Comment on attachment 352575 [details]
Patch

Attachment 352575 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9637730

New failing tests:
storage/indexeddb/prefetch-invalidation.html

[EWS Watchlist]

Created attachment 352591 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 352575 [details]
Patch

Attachment 352575 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9637823

New failing tests:
storage/indexeddb/prefetch-invalidation.html

[EWS Watchlist]

Created attachment 352594 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Sihui Liu]

Created attachment 352599 [details]
Patch

[Sihui Liu]

(In reply to Chris Dumez from comment #26)
> (In reply to Sihui Liu from comment #25)
> > (In reply to Chris Dumez from comment #23)
> > > Comment on attachment 352528 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=352528&action=review
> > > 
> > > > Source/WebCore/ChangeLog:12
> > > > +        If the size of free disk space is over 100 MB, the default limit is 50 MB; otherwise it is 
> > > 
> > > Does it ask the user for more space before failing (similarly to
> > > WebStorage)? I do not think we should land such a low limit without allowing
> > > the user to override, this has the potential to cause Web breakage without
> > > the user being able to do anything about it.
> > 
> > No, it doesn't.
> > 
> > I thought 500 MB may be too much for a mobile app. But yes, in case it
> > breaks any existing app, I changed it back to 500 MB.
> 
> As you going to ask the user in a follow-up patch?

Yes, I think so. The reasons I chose not to do it with this patch:

1. That seems not so beneficial for security purpose. Users may tend to "Agree" to increase the Quota always.
2. There is risk to break the logic of current IDB and make performance worse(extra messages and wait), which requires more work to be done.
3. Adding a hard limit solve much concern for using up the space.

[EWS Watchlist]

Comment on attachment 352599 [details]
Patch

Attachment 352599 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9639977

New failing tests:
storage/indexeddb/storage-limit.html
storage/indexeddb/key-type-array.html

[EWS Watchlist]

Created attachment 352629 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Chris Dumez]

(In reply to Sihui Liu from comment #32)
> (In reply to Chris Dumez from comment #26)
> > (In reply to Sihui Liu from comment #25)
> > > (In reply to Chris Dumez from comment #23)
> > > > Comment on attachment 352528 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=352528&action=review
> > > > 
> > > > > Source/WebCore/ChangeLog:12
> > > > > +        If the size of free disk space is over 100 MB, the default limit is 50 MB; otherwise it is 
> > > > 
> > > > Does it ask the user for more space before failing (similarly to
> > > > WebStorage)? I do not think we should land such a low limit without allowing
> > > > the user to override, this has the potential to cause Web breakage without
> > > > the user being able to do anything about it.
> > > 
> > > No, it doesn't.
> > > 
> > > I thought 500 MB may be too much for a mobile app. But yes, in case it
> > > breaks any existing app, I changed it back to 500 MB.
> > 
> > As you going to ask the user in a follow-up patch?
> 
> Yes, I think so. The reasons I chose not to do it with this patch:
> 
> 1. That seems not so beneficial for security purpose. Users may tend to
> "Agree" to increase the Quota always.
> 2. There is risk to break the logic of current IDB and make performance
> worse(extra messages and wait), which requires more work to be done.
> 3. Adding a hard limit solve much concern for using up the space.

I guess I am OK landing a temporary very high (500MB) limit without asking the user, for the purpose of limiting the patch size. However, I think we *need* to follow-up and both:
- Reduce limit to something more reasonable (e.g. 50MB)
and
- Ask the user if they want to increase the quota

This is that the specification suggests and this is what we do for WebStorage. We already have the client and WebKit-side implementation for this so it would not add much complexity.

[Ryosuke Niwa]

(In reply to Chris Dumez from comment #35)
>
> I guess I am OK landing a temporary very high (500MB) limit without asking
> the user, for the purpose of limiting the patch size.

We probably should use something smaller than that. 100MB? 200MB?

> However, I think we *need* to follow-up and both:
> - Reduce limit to something more reasonable (e.g. 50MB)
> and
> - Ask the user if they want to increase the quota
> 
> This is that the specification suggests and this is what we do for
> WebStorage. We already have the client and WebKit-side implementation for
> this so it would not add much complexity.

Yeah, we probably need do that. It's neat that we already have UI code hooked up. I didn't know that!

[Sihui Liu]

(In reply to Ryosuke Niwa from comment #36)
> (In reply to Chris Dumez from comment #35)
> >
> > I guess I am OK landing a temporary very high (500MB) limit without asking
> > the user, for the purpose of limiting the patch size.
> 
> We probably should use something smaller than that. 100MB? 200MB?
> 
100, 200, 500 seem to make little difference, considering we would assign only half free disk space when the size is less than 1G.

> > However, I think we *need* to follow-up and both:
> > - Reduce limit to something more reasonable (e.g. 50MB)
> > and
> > - Ask the user if they want to increase the quota
> > 
> > This is that the specification suggests and this is what we do for
> > WebStorage. We already have the client and WebKit-side implementation for
> > this so it would not add much complexity.
> 
> Yeah, we probably need do that. It's neat that we already have UI code
> hooked up. I didn't know that!
>
Yes we have that and it saves effort on the UI side (yeah!), and the challenge is on server side. Web SQL requires the quota permission synchronously when it gets quota error. Unlike Web SQL, processing of IDB is in network process and it's all async. 
Plus current flow for Web SQL seems not correct: it returns an error after user approves the quota increase, and increases quota on next request.

[Sihui Liu]

For the added test failing on win bot: it turned out that malloc failed on the 500MB array. Maybe because it's using 32-bit (but weirdly it's not falling on mac-32bit)... So I will just skip the test on Win.

[Sihui Liu]

Created attachment 352669 [details]
Patch

[Chris Dumez]

(In reply to Sihui Liu from comment #38)
> For the added test failing on win bot: it turned out that malloc failed on
> the 500MB array. Maybe because it's using 32-bit (but weirdly it's not
> falling on mac-32bit)... So I will just skip the test on Win.

Mac-32 bit EWS does not run tests. I would not recommend allocating a 500MB array in your test. The quota should be customizable from the test using internals API for performance reasons.

[Chris Dumez]

(In reply to Sihui Liu from comment #37)
> (In reply to Ryosuke Niwa from comment #36)
> > (In reply to Chris Dumez from comment #35)
> > >
> > > I guess I am OK landing a temporary very high (500MB) limit without asking
> > > the user, for the purpose of limiting the patch size.
> > 
> > We probably should use something smaller than that. 100MB? 200MB?
> > 
> 100, 200, 500 seem to make little difference, considering we would assign
> only half free disk space when the size is less than 1G.

It makes a difference as this mean several origins can fill up your disk quickly without the user being notified about them using a large amount of space.

> 
> > > However, I think we *need* to follow-up and both:
> > > - Reduce limit to something more reasonable (e.g. 50MB)
> > > and
> > > - Ask the user if they want to increase the quota
> > > 
> > > This is that the specification suggests and this is what we do for
> > > WebStorage. We already have the client and WebKit-side implementation for
> > > this so it would not add much complexity.
> > 
> > Yeah, we probably need do that. It's neat that we already have UI code
> > hooked up. I didn't know that!
> >
> Yes we have that and it saves effort on the UI side (yeah!), and the
> challenge is on server side. Web SQL requires the quota permission
> synchronously when it gets quota error. Unlike Web SQL, processing of IDB is
> in network process and it's all async. 
> Plus current flow for Web SQL seems not correct: it returns an error after
> user approves the quota increase, and increases quota on next request.

I still think it is worth doing.

[Sihui Liu]

Created attachment 352755 [details]
Patch

[Sihui Liu]

Created attachment 352772 [details]
Patch

[Sihui Liu]

Created attachment 352792 [details]
Patch

[Sihui Liu]

Created attachment 352800 [details]
Patch

[EWS Watchlist]

Comment on attachment 352800 [details]
Patch

Attachment 352800 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9668755

New failing tests:
storage/indexeddb/storage-limit.html
storage/indexeddb/key-type-array.html

[EWS Watchlist]

Created attachment 352816 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Don Olmstead]

Comment on attachment 352800 [details]
Patch

Looks better now that it can be set. Any way to put the default somewhere shared since then you'd have to keep WebKit/WebKitLegacy in sync for defaults?

[Sihui Liu]

Created attachment 352915 [details]
Patch

[Sihui Liu]

Created attachment 352925 [details]
Patch

[Don Olmstead]

With regards to the WinCairo failure I think you're hitting https://bugs.webkit.org/show_bug.cgi?id=183177 so its fine to land. If need be we will rebuild the buildbot if it fails.

[Chris Dumez]

Comment on attachment 352925 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:657
> +        if (database)

I am pretty sure this null check is not needed, would seem weird to store null in the HashMap.

> Source/WebCore/Modules/indexeddb/server/IDBServer.h:107
> +    uint64_t perOriginQuota() { return m_perOriginQuota; }

should be const.

> Source/WebCore/Modules/indexeddb/server/IDBServer.h:134
> +    uint64_t m_perOriginQuota;

Needs a default initializer, e.g. uint64_t m_perOriginQuota { 0 };

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:855
> +    uint64_t quota = quotaForOrigin();

What if quota is 0 (i.e. uninitialized)?

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.h:203
> +    uint64_t m_quota;

Needs a default initializer: uint64_t m_quota { 0 }; and then handling for the default value if the quota never gets set.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1207
> +    for (auto& sessionID : m_idbServers.keys())

Iterating over the keys and then looking up each key is a terrible pattern for performance. Why aren't you iterating over values directly?

> Source/WebKitLegacy/Storage/WebDatabaseProvider.h:38
> +

Why the blank line?

> Source/WebKitLegacy/Storage/WebDatabaseProvider.h:59
> +    uint64_t m_idbPerOriginQuota { 500 * MB };

This value is currently hardcoded in 2 places. Would it be possible to move it to a shared place? maybe IDBServer ?

> Source/WebKitLegacy/win/Interfaces/IWebDatabaseManager.idl:64
> +    HRESULT setIDBPerOriginQuota([in] unsigned long long quota);

You're not allowed to modify existing interfaces like this or it will break iTunes. Either you create a new IWebDatabaseManager3 with a new uuid and your new method, or my personal favorite, do not add support on Windows and skip the test there.

> Tools/DumpRenderTree/TestRunner.cpp:931
> +    TestRunner* controller = static_cast<TestRunner*>(JSObjectGetPrivate(thisObject));

auto*

> LayoutTests/storage/indexeddb/resources/storage-limit.js:9
> +if (window.testRunner) {

Unnecessary curly brackets

> LayoutTests/storage/indexeddb/resources/storage-limit.js:27
> +    evalAndLog("request = store.add(new Uint8Array(" + quota  + "), 0)");

Question: Why does it fail with quota (seems like it would fit)? I would have expected quota + 1 here.

> LayoutTests/storage/indexeddb/resources/storage-limit.js:36
> +        debug("Add operation should fail because storage limit is reached, but succeeded.");

testFailed("");

[Sihui Liu]

Comment on attachment 352925 [details]
Patch

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

iil

>> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:657
>> +        if (database)
> 
> I am pretty sure this null check is not needed, would seem weird to store null in the HashMap.

Okay.

>> Source/WebCore/Modules/indexeddb/server/IDBServer.h:107
>> +    uint64_t perOriginQuota() { return m_perOriginQuota; }
> 
> should be const.

Okay.

>> Source/WebCore/Modules/indexeddb/server/IDBServer.h:134
>> +    uint64_t m_perOriginQuota;
> 
> Needs a default initializer, e.g. uint64_t m_perOriginQuota { 0 };

Set to 500 MB.

>> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:855
>> +    uint64_t quota = quotaForOrigin();
> 
> What if quota is 0 (i.e. uninitialized)?

If quota is 0, maximumSize() would return current database file size. Then nothing could be add to this database.

>> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.h:203
>> +    uint64_t m_quota;
> 
> Needs a default initializer: uint64_t m_quota { 0 }; and then handling for the default value if the quota never gets set.

Okay.
Quota is set in openBackingStore, so we should always get it set when we use backingStore. If it's still 0 then we use it before opening it, which should not happen?

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1207
>> +    for (auto& sessionID : m_idbServers.keys())
> 
> Iterating over the keys and then looking up each key is a terrible pattern for performance. Why aren't you iterating over values directly?

Changed to use values here.

>> Source/WebKitLegacy/Storage/WebDatabaseProvider.h:38
>> +
> 
> Why the blank line?

Removed.

>> Source/WebKitLegacy/Storage/WebDatabaseProvider.h:59
>> +    uint64_t m_idbPerOriginQuota { 500 * MB };
> 
> This value is currently hardcoded in 2 places. Would it be possible to move it to a shared place? maybe IDBServer ?

Okay.

>> Source/WebKitLegacy/win/Interfaces/IWebDatabaseManager.idl:64
>> +    HRESULT setIDBPerOriginQuota([in] unsigned long long quota);
> 
> You're not allowed to modify existing interfaces like this or it will break iTunes. Either you create a new IWebDatabaseManager3 with a new uuid and your new method, or my personal favorite, do not add support on Windows and skip the test there.

Per Said It should be fine to add to IWebDatabaseManager2, which was added for test.

>> Tools/DumpRenderTree/TestRunner.cpp:931
>> +    TestRunner* controller = static_cast<TestRunner*>(JSObjectGetPrivate(thisObject));
> 
> auto*

Okay.

>> LayoutTests/storage/indexeddb/resources/storage-limit.js:9
>> +if (window.testRunner) {
> 
> Unnecessary curly brackets

Removed.

>> LayoutTests/storage/indexeddb/resources/storage-limit.js:27
>> +    evalAndLog("request = store.add(new Uint8Array(" + quota  + "), 0)");
> 
> Question: Why does it fail with quota (seems like it would fit)? I would have expected quota + 1 here.

The quota sets the size of database file, and I guess the file contains some db related info other than data. Will add "+1" here to make it look more reasonable.

>> LayoutTests/storage/indexeddb/resources/storage-limit.js:36
>> +        debug("Add operation should fail because storage limit is reached, but succeeded.");
> 
> testFailed("");

Okay.

[Chris Dumez]

Comment on attachment 352925 [details]
Patch

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

>>> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:855
>>> +    uint64_t quota = quotaForOrigin();
>> 
>> What if quota is 0 (i.e. uninitialized)?
> 
> If quota is 0, maximumSize() would return current database file size. Then nothing could be add to this database.

Yes, my point is that this would be bad. 0 should mean no limit not "you cannot do anything".

[Sihui Liu]

Created attachment 353405 [details]
Patch

[Chris Dumez]

Comment on attachment 353405 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/server/IDBServer.h:51
> +const uint64_t DefaultPerOriginQuota = 500 * MB;

our constants usually do not start with a capital letter.

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:855
> +    uint64_t quota = quotaForOrigin();

As commented earlier, if the quota does not get set, quotaForOrigin() will be 0 which means the API will not work.

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.h:203
> +    uint64_t m_quota { 0 };

Default behavior right now is to not have a quota of 0 and therefore not work. I think default here should be IDBServer::defaultPerOriginQuota. Either that or we don't have a default value here and we force the IDBServer to pass a quota to the SQLiteIDBBackingStore constructor so that the member is always initialized with a sane value.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:614
> +    setQuota(m_server.perOriginQuota());

Seems kind of weird to:
1. Ask the server to create a backingstore
2. get the quota for the server
3. Set the quota on the backingstore

It seems like m_server.createBackingStore() could take care of setting the quota by itself (preferably by passing the quota to the backingStore constructor, or by calling setQuota() after constructing it which I like a bit less)

> Source/WebKitLegacy/Storage/WebDatabaseProvider.h:59
> +    uint64_t m_idbPerOriginQuota { WebCore::IDBServer::DefaultPerOriginQuota };

To avoid exposing DefaultPerOriginQuota. I think this could either be a std::optional<uint64_t> or use 0 as magic value to indicate that we want to use the default quota.

Then you only need to call setPerOriginQuota() on the idbServer above if m_idbPerOriginQuota is not std::null_opt / 0.

> LayoutTests/storage/indexeddb/resources/storage-limit.js:26
> +    evalAndLog("request = store.add(new Uint8Array(" + (++quota) + "), 0)");

quota + 1

I think it looks very weird to be modifying quota here.

> LayoutTests/storage/indexeddb/resources/storage-limit.js:30
> +        shouldBe("request.error.name", "'QuotaExceededError'");

shouldBeEqualToString("request.error.name", "QuotaExceededError");

[Sihui Liu]

Created attachment 353419 [details]
Patch

[Sihui Liu]

Comment on attachment 353405 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/server/IDBServer.h:51
>> +const uint64_t DefaultPerOriginQuota = 500 * MB;
> 
> our constants usually do not start with a capital letter.

Okay.

>> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:855
>> +    uint64_t quota = quotaForOrigin();
> 
> As commented earlier, if the quota does not get set, quotaForOrigin() will be 0 which means the API will not work.

Ah I see what you mean.

>> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.h:203
>> +    uint64_t m_quota { 0 };
> 
> Default behavior right now is to not have a quota of 0 and therefore not work. I think default here should be IDBServer::defaultPerOriginQuota. Either that or we don't have a default value here and we force the IDBServer to pass a quota to the SQLiteIDBBackingStore constructor so that the member is always initialized with a sane value.

Will add quota setting to constructor of SQLiteIDBBackingStore.

>> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:614
>> +    setQuota(m_server.perOriginQuota());
> 
> Seems kind of weird to:
> 1. Ask the server to create a backingstore
> 2. get the quota for the server
> 3. Set the quota on the backingstore
> 
> It seems like m_server.createBackingStore() could take care of setting the quota by itself (preferably by passing the quota to the backingStore constructor, or by calling setQuota() after constructing it which I like a bit less)

Will move the setting to createBackingStore() and let the IDBServer to initialize quota in backingstore's constructor.

>> Source/WebKitLegacy/Storage/WebDatabaseProvider.h:59
>> +    uint64_t m_idbPerOriginQuota { WebCore::IDBServer::DefaultPerOriginQuota };
> 
> To avoid exposing DefaultPerOriginQuota. I think this could either be a std::optional<uint64_t> or use 0 as magic value to indicate that we want to use the default quota.
> 
> Then you only need to call setPerOriginQuota() on the idbServer above if m_idbPerOriginQuota is not std::null_opt / 0.

Okay.

>> LayoutTests/storage/indexeddb/resources/storage-limit.js:26
>> +    evalAndLog("request = store.add(new Uint8Array(" + (++quota) + "), 0)");
> 
> quota + 1
> 
> I think it looks very weird to be modifying quota here.

Changed.

>> LayoutTests/storage/indexeddb/resources/storage-limit.js:30
>> +        shouldBe("request.error.name", "'QuotaExceededError'");
> 
> shouldBeEqualToString("request.error.name", "QuotaExceededError");

Okay.

[Chris Dumez]

Comment on attachment 353419 [details]
Patch

r=me but the inconsistency between WK1 and WK2 bother me and I think we should align both before landing:
- WK2 has m_idbPerOriginQuota be IDBServer::defaultPerOriginQuota by default and sets it on the IDBServer no matter what
- WK1 has m_idbPerOriginQuota be 0 by default and only sets it on the IDBServer if the value is not 0

Please choose align both (I let you choose which one you prefer).

[Sihui Liu]

Created attachment 353639 [details]
Patch for landing

[Sihui Liu]

(In reply to Chris Dumez from comment #59)
> Comment on attachment 353419 [details]
> Patch
> 
> r=me but the inconsistency between WK1 and WK2 bother me and I think we
> should align both before landing:
> - WK2 has m_idbPerOriginQuota be IDBServer::defaultPerOriginQuota by default
> and sets it on the IDBServer no matter what
> - WK1 has m_idbPerOriginQuota be 0 by default and only sets it on the
> IDBServer if the value is not 0
> 
> Please choose align both (I let you choose which one you prefer).

Made WK1 to use IDBServer::defaultPerOriginQuota.

[WebKit Commit Bot]

Comment on attachment 353639 [details]
Patch for landing

Clearing flags on attachment: 353639

Committed r237700: <https://trac.webkit.org/changeset/237700>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yusuke Morita]

What was the engineering reason behind imposing a quota on IndexedDB?

How was the 500MB value decided upon? I would have expected a cap based on remaining space instead of enforcing a 500MB ceiling.

What version of iOS 12 did this change first ship?

[Ryosuke Niwa]

(In reply to Yusuke Morita from comment #64)
> What was the engineering reason behind imposing a quota on IndexedDB?

This is important because otherwise some website / Safari can consume valuable storage space on user's devices.

> How was the 500MB value decided upon? I would have expected a cap based on
> remaining space instead of enforcing a 500MB ceiling.

This was a value deemed large enough not to break existing sites.

> What version of iOS 12 did this change first ship?

There is no version of iOS either shipped or in beta which contains this change. In general, Apple does not comment on future product plans.

[Yusuke Morita]

(In reply to Ryosuke Niwa from comment #65)
> (In reply to Yusuke Morita from comment #64)
> > What was the engineering reason behind imposing a quota on IndexedDB?
> 
> This is important because otherwise some website / Safari can consume
> valuable storage space on user's devices.
> 
> > How was the 500MB value decided upon? I would have expected a cap based on
> > remaining space instead of enforcing a 500MB ceiling.
> 
> This was a value deemed large enough not to break existing sites.
> 
> > What version of iOS 12 did this change first ship?
> 
> There is no version of iOS either shipped or in beta which contains this
> change. In general, Apple does not comment on future product plans.

I've got an enterprise web application that I need to support that is currently breaking because of this change. In my case, the data stored in IndexedDB is arguably more important than preserving device storage space.

I'm getting the following error message on my iOS 12.2 device (notice the typo):
'Failed to store new database version in database because no enough space for domain'

..that was introduced in this commit: https://github.com/WebKit/webkit/commit/9be59a18dbdf9258c5325a1adef7d2a13f3bd3fc#diff-41f04caf6c79a48f25ea5843c19acc3aR897

I would like to definitively report what version of iOS 12 this change took effect as it seems to have happened in a recent release and it would help greatly to warn our clients not to update their devices to the affected version.

Would you consider adjusting the algorithm to a heuristic based on available storage space rather than an arbitrary 500MB ceiling?

[Sihui Liu]

(In reply to Yusuke Morita from comment #66)
> (In reply to Ryosuke Niwa from comment #65)
> > (In reply to Yusuke Morita from comment #64)
> > > What was the engineering reason behind imposing a quota on IndexedDB?
> > 
> > This is important because otherwise some website / Safari can consume
> > valuable storage space on user's devices.
> > 
> > > How was the 500MB value decided upon? I would have expected a cap based on
> > > remaining space instead of enforcing a 500MB ceiling.
> > 
> > This was a value deemed large enough not to break existing sites.
> > 
> > > What version of iOS 12 did this change first ship?
> > 
> > There is no version of iOS either shipped or in beta which contains this
> > change. In general, Apple does not comment on future product plans.
> 
> I've got an enterprise web application that I need to support that is
> currently breaking because of this change. In my case, the data stored in
> IndexedDB is arguably more important than preserving device storage space.
> 
> I'm getting the following error message on my iOS 12.2 device (notice the
> typo):
> 'Failed to store new database version in database because no enough space
> for domain'
> 
> ..that was introduced in this commit:
> https://github.com/WebKit/webkit/commit/
> 9be59a18dbdf9258c5325a1adef7d2a13f3bd3fc#diff-
> 41f04caf6c79a48f25ea5843c19acc3aR897
> 
> I would like to definitively report what version of iOS 12 this change took
> effect as it seems to have happened in a recent release and it would help
> greatly to warn our clients not to update their devices to the affected
> version.
> 
> Would you consider adjusting the algorithm to a heuristic based on available
> storage space rather than an arbitrary 500MB ceiling?

Hi Yusuke, this change was made because we found IDB storage can be abused and use up all user space. 500MB is a high limit for a single domain and our tests did not discover breakage. We do calculate quota based on space but that happens only when free space is less than 1GB.

Based on your report, we may need to reconsider our approach. How much space does the app need?

This change was introduced in iOS 12.2.

[Yusuke Morita]

(In reply to Sihui Liu from comment #67)
> Based on your report, we may need to reconsider our approach. How much space
> does the app need?
> 
> This change was introduced in iOS 12.2.

I estimate we're pushing right up to about 500MB for the particular user I'm testing with but storage requirements significantly vary by client. Is there a way to determine how much space IDB is currently using in iOS Safari?

If the algorithm was 50% of available space (without a hard limit), we'd be back on track. I suppose there's a threshold where the performance of the backing SQLiteIDBBackingStore would begin to suffer but I'm not familiar with those constraints.

Thank you so much for your time and consideration!

[Geoffrey Garen]

> > Is there
> a way to determine how much space IDB is currently using in iOS Safari?

In the Settings app, you can consult Safari -> Advanced -> Website data to see how much data you're using.

> If the algorithm was 50% of available space (without a hard limit), we'd be
> back on track.

50% available space is much much more than we would allow even for an app that was subject to App Store review. So, we probably need to come up with a solution that's smaller than that.

Out of curiosity, what exactly is broken about your web app? Why does it need so much local storage unconditionally, and why can't it recover by loading data dynamically from the server?

[Yusuke Morita]

(In reply to Geoffrey Garen from comment #69)
> 50% available space is much much more than we would allow even for an app
> that was subject to App Store review. So, we probably need to come up with a
> solution that's smaller than that.

I completely understand. What is the storage limit for native apps? If it's higher than 500MB would you consider making IDB storage limits the same? :)
 
> Out of curiosity, what exactly is broken about your web app? Why does it
> need so much local storage unconditionally, and why can't it recover by
> loading data dynamically from the server?

We are admittedly pushing the limits of web apps and offline browser storage. The app relies on offline storage because we need to operate in environments that have no internet connection. We've managed to maneuver around various changes to Safari storage limits starting with iOS 9 but this latest change will prove to be quite a challenge to overcome.

[Yusuke Morita]

(In reply to Geoffrey Garen from comment #69)
> > > Is there
> > a way to determine how much space IDB is currently using in iOS Safari?
> 
> In the Settings app, you can consult Safari -> Advanced -> Website data to
> see how much data you're using.

This method reports 6.5MB but if this was true, I shouldn't be getting a quota error. Checking in Chrome devtools gives me a little under 500MB which is closer to what I'm expecting.

[Geoffrey Garen]

(In reply to Yusuke Morita from comment #71)
> (In reply to Geoffrey Garen from comment #69)
> > > > Is there
> > > a way to determine how much space IDB is currently using in iOS Safari?
> > 
> > In the Settings app, you can consult Safari -> Advanced -> Website data to
> > see how much data you're using.
> 
> This method reports 6.5MB but if this was true, I shouldn't be getting a
> quota error. Checking in Chrome devtools gives me a little under 500MB which
> is closer to what I'm expecting.

To clarify, you'll only see storage usage that was approved. If you're storing a large blob, and you hit the limit and the storage is rejected, you won't see the number go up.

But in an older OS version, that has no storage limit, you can probably see the true number.