Bug 189801 - WebSQL: User cannot grant quota increase if the JS provides an expected usage value that is too low
Summary: WebSQL: User cannot grant quota increase if the JS provides an expected usage...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-09-20 13:33 PDT by Chris Dumez
Modified: 2018-09-21 16:19 PDT (History)
8 users (show)

See Also:


Attachments
Patch (7.69 KB, patch)
2018-09-20 13:39 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.53 MB, application/zip)
2018-09-20 14:48 PDT, EWS Watchlist
no flags Details
Patch (7.77 KB, patch)
2018-09-20 15:55 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (7.97 KB, patch)
2018-09-21 10:47 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (7.97 KB, patch)
2018-09-21 11:03 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (9.76 KB, patch)
2018-09-21 11:52 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (14.07 KB, patch)
2018-09-21 12:13 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (14.11 KB, patch)
2018-09-21 12:18 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2018-09-20 13:33:30 PDT
User cannot grant quota increase for WebSQL if the JS provides an expected usage value that is too low.
Comment 1 Chris Dumez 2018-09-20 13:33:46 PDT
<rdar://problem/43592498>
Comment 2 Chris Dumez 2018-09-20 13:39:44 PDT
Created attachment 350258 [details]
Patch
Comment 3 EWS Watchlist 2018-09-20 14:48:16 PDT
Comment on attachment 350258 [details]
Patch

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

New failing tests:
storage/websql/transaction-database-expand-quota.html
Comment 4 EWS Watchlist 2018-09-20 14:48:17 PDT
Created attachment 350267 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 5 Geoffrey Garen 2018-09-20 15:22:47 PDT
Regressions: Unexpected text-only failures (1)
  storage/websql/transaction-database-expand-quota.html [ Failure ]
Comment 6 Chris Dumez 2018-09-20 15:53:33 PDT
(In reply to Geoffrey Garen from comment #5)
> Regressions: Unexpected text-only failures (1)
>   storage/websql/transaction-database-expand-quota.html [ Failure ]

Yes, WK1 ignores the estimatedSize parameter and gets it from the DatabaseTracker instead. I will have a fix shortly.
Comment 7 Chris Dumez 2018-09-20 15:55:28 PDT
Created attachment 350275 [details]
Patch
Comment 8 youenn fablet 2018-09-21 09:38:09 PDT
Comment on attachment 350275 [details]
Patch

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

> Source/WebCore/Modules/webdatabase/Database.cpp:797
> +    if (estimatedSize() <= oldQuota) {

Is the name oldQuota misleading?
It seems like oldQuota is the current database size while estimatedSize is the 'quota' defined by the page.

> Source/WebCore/Modules/webdatabase/Database.cpp:800
> +        setEstimatedSize(oldQuota + 5 * 1024. * 1024.);

Should we move this as a macro with a name like QUOTA_SIZE_INCREMENT?

> Source/WebCore/Modules/webdatabase/Database.h:92
> +    void setEstimatedSize(unsigned);

Can we make setEstimatedSize private?
And maybe estimatedSize as well.
Comment 9 Chris Dumez 2018-09-21 10:04:48 PDT
Comment on attachment 350275 [details]
Patch

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

>> Source/WebCore/Modules/webdatabase/Database.cpp:797
>> +    if (estimatedSize() <= oldQuota) {
> 
> Is the name oldQuota misleading?
> It seems like oldQuota is the current database size while estimatedSize is the 'quota' defined by the page.

I think oldQuota is correct. It is really the current quota. However, because we now we exceeded the quota, we know the database size has reached the quota.

>> Source/WebCore/Modules/webdatabase/Database.cpp:800
>> +        setEstimatedSize(oldQuota + 5 * 1024. * 1024.);
> 
> Should we move this as a macro with a name like QUOTA_SIZE_INCREMENT?

If you mean a quotaSizeIncrement global variable, then yes.

>> Source/WebCore/Modules/webdatabase/Database.h:92
>> +    void setEstimatedSize(unsigned);
> 
> Can we make setEstimatedSize private?
> And maybe estimatedSize as well.

Certainly.
Comment 10 Chris Dumez 2018-09-21 10:47:35 PDT
Created attachment 350380 [details]
Patch
Comment 11 Chris Dumez 2018-09-21 11:03:26 PDT
Created attachment 350388 [details]
Patch
Comment 12 Chris Dumez 2018-09-21 11:52:58 PDT
Created attachment 350397 [details]
Patch
Comment 13 Chris Dumez 2018-09-21 12:13:13 PDT
Created attachment 350400 [details]
Patch
Comment 14 Chris Dumez 2018-09-21 12:18:03 PDT
Created attachment 350401 [details]
Patch
Comment 15 WebKit Commit Bot 2018-09-21 12:59:49 PDT
Comment on attachment 350401 [details]
Patch

Clearing flags on attachment: 350401

Committed r236348: <https://trac.webkit.org/changeset/236348>
Comment 16 WebKit Commit Bot 2018-09-21 12:59:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Truitt Savell 2018-09-21 15:54:55 PDT
Looks like the new test added in https://trac.webkit.org/changeset/236348/webkit

is a flakey timeout: storage/websql/transaction-database-expand-quota.html

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=storage%2Fwebsql%2Ftransaction-database-expand-quota.html
Comment 18 Chris Dumez 2018-09-21 16:02:56 PDT
(In reply to Truitt Savell from comment #17)
> Looks like the new test added in
> https://trac.webkit.org/changeset/236348/webkit
> 
> is a flakey timeout: storage/websql/transaction-database-expand-quota.html
> 
> History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=storage%2Fwebsql%2Ftransaction-database-expand-
> quota.html

Looking...
Comment 19 Chris Dumez 2018-09-21 16:11:32 PDT
(In reply to Chris Dumez from comment #18)
> (In reply to Truitt Savell from comment #17)
> > Looks like the new test added in
> > https://trac.webkit.org/changeset/236348/webkit
> > 
> > is a flakey timeout: storage/websql/transaction-database-expand-quota.html
> > 
> > History:
> > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> > html#showAllRuns=true&tests=storage%2Fwebsql%2Ftransaction-database-expand-
> > quota.html
> 
> Looking...

I think the test is just slow in debug because of constructing the 8MB array. I'll see how I can speed it up.
Comment 20 Chris Dumez 2018-09-21 16:19:31 PDT
(In reply to Chris Dumez from comment #19)
> (In reply to Chris Dumez from comment #18)
> > (In reply to Truitt Savell from comment #17)
> > > Looks like the new test added in
> > > https://trac.webkit.org/changeset/236348/webkit
> > > 
> > > is a flakey timeout: storage/websql/transaction-database-expand-quota.html
> > > 
> > > History:
> > > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> > > html#showAllRuns=true&tests=storage%2Fwebsql%2Ftransaction-database-expand-
> > > quota.html
> > 
> > Looking...
> 
> I think the test is just slow in debug because of constructing the 8MB
> array. I'll see how I can speed it up.

Sped up the test in <https://trac.webkit.org/changeset/236370> by using a much smaller array.