Bug 195283 - Introduce a quota manager for Cache API/Service Worker/IDB storage
Summary: Introduce a quota manager for Cache API/Service Worker/IDB storage
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks: 195302
  Show dependency treegraph
 
Reported: 2019-03-04 10:03 PST by youenn fablet
Modified: 2019-03-07 09:41 PST (History)
6 users (show)

See Also:


Attachments
Patch (38.41 KB, patch)
2019-03-04 15:42 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (38.73 KB, patch)
2019-03-04 22:05 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (8.79 MB, application/zip)
2019-03-05 00:44 PST, Build Bot
no flags Details
Patch (40.07 KB, patch)
2019-03-05 15:52 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (40.18 KB, patch)
2019-03-05 15:59 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (40.17 KB, patch)
2019-03-05 16:33 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (40.85 KB, patch)
2019-03-06 11:56 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2019-03-04 10:03:00 PST
Introduce a quota manager for Cache API/Service Worker/IDB storage
Comment 1 youenn fablet 2019-03-04 15:42:12 PST
Created attachment 363560 [details]
Patch
Comment 2 youenn fablet 2019-03-04 22:05:48 PST
Created attachment 363600 [details]
Patch
Comment 3 Build Bot 2019-03-05 00:44:40 PST
Comment on attachment 363600 [details]
Patch

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

New failing tests:
fast/events/beforeunload-alert-user-interaction2.html
fast/shadow-dom/click-text-inside-linked-slot.html
fast/scrolling/ios/hit-testing-iframe-003.html
fast/forms/datalist/datalist-textinput-suggestions-order.html
fast/scrolling/ios/mixing-user-and-programmatic-scroll-006.html
editing/selection/thai-word-at-document-end.html
fast/events/click-handler-on-body-simple.html
Comment 4 Build Bot 2019-03-05 00:44:42 PST
Created attachment 363615 [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
Comment 5 youenn fablet 2019-03-05 10:18:53 PST
Comment on attachment 363615 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

Errors are probably unrelated
Comment 6 Chris Dumez 2019-03-05 13:37:43 PST
Comment on attachment 363600 [details]
Patch

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

> Source/WebKit/ChangeLog:13
> +

Extra blank line.

> Source/WebCore/storage/QuotaManager.cpp:61
> +void QuotaManager::requestSpaceIncreaseCompleted(Optional<uint64_t> quota)

quota -> newQuota ?

> Source/WebCore/storage/QuotaManager.cpp:66
> +    auto pendingRequests = WTFMove(m_pendingRequests);

Seems weird to take all pending requests here...

> Source/WebCore/storage/QuotaManager.cpp:77
> +        requestSpace(request.spaceIncrease, WTFMove(request.callback));

... just to add them all back here.

This also means that if the request.callback() call above keeps making quota requests, we'll never process following requests. I think this needs to be refactored a bit to be more reliable. We really want to process the requests in the order they came in and it looks like you are not guaranteeing this here.

> Source/WebCore/storage/QuotaManager.h:38
> +class QuotaManager : public CanMakeWeakPtr<QuotaManager> {

The name seem too generic given that it is not in a "Storage" namespace. I would suggest StorageQuotaManager. A quota could be for many things otherwise.

Should be fast allocated.
We should also likely prevent copying.

> Source/WebCore/storage/QuotaManager.h:48
> +    void addQuotaUser(QuotaUser& user)

QuotaUser should probably be const.

> Source/WebCore/storage/QuotaManager.h:54
> +    void removeQuotaUser(QuotaUser& user)

QuotaUser should probably be const.

> Source/WebCore/storage/QuotaManager.h:71
> +    HashSet<QuotaUser*> m_quotaUsers;

QuotaUser should probably be const. Given the number of quota users we'll have, we can probably use a Vector instead of a HashSet.

> Source/WebCore/storage/QuotaUser.h:30
> +class QuotaUser {

I think this would be nicer as a "User" class inside the "StorageQuotaManager" class scope. I.e. StorageQuotaManager::User.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2307
> +    return *m_quotaManagers.ensure(origin, [this, sessionID, &origin] {

I think something needs to destroy quota managers when the session is destroyed, otherwise this is ever growing and really bad for private browsing where we have a private session per tab.

> Source/WebKit/NetworkProcess/NetworkProcess.h:326
> +    WebCore::QuotaManager& quotaManager(PAL::SessionID, const WebCore::ClientOrigin&);

storageQuotaManager ?

> Source/WebKit/NetworkProcess/NetworkProcess.h:530
> +    HashMap<WebCore::ClientOrigin, std::unique_ptr<WebCore::QuotaManager>> m_quotaManagers;

m_storageQuotaManager?

> Source/WebKit/NetworkProcess/NetworkProcess.h:531
> +    HashMap<PAL::SessionID, uint64_t> m_quotas;

m_perSessionStorageQuotas ?
Comment 7 youenn fablet 2019-03-05 15:52:20 PST
Created attachment 363704 [details]
Patch
Comment 8 youenn fablet 2019-03-05 15:55:56 PST
> This also means that if the request.callback() call above keeps making quota
> requests, we'll never process following requests. I think this needs to be
> refactored a bit to be more reliable. We really want to process the requests
> in the order they came in and it looks like you are not guaranteeing this
> here.

In practice, this should not happen but yes, I refactored it to make sure we keep a reliable order.

> > Source/WebCore/storage/QuotaManager.h:38
> > +class QuotaManager : public CanMakeWeakPtr<QuotaManager> {
> 
> The name seem too generic given that it is not in a "Storage" namespace. I
> would suggest StorageQuotaManager. A quota could be for many things
> otherwise.

Went with StorageQuotaManager/StorageQuotaUser

> I think this would be nicer as a "User" class inside the
> "StorageQuotaManager" class scope. I.e. StorageQuotaManager::User.

I kept it separate, one small advantage is reduced include.

Other points should be fixed in the above patch.
Comment 9 youenn fablet 2019-03-05 15:59:39 PST
Created attachment 363705 [details]
Patch
Comment 10 youenn fablet 2019-03-05 16:33:09 PST
Created attachment 363706 [details]
Patch
Comment 11 youenn fablet 2019-03-06 11:56:25 PST
Created attachment 363767 [details]
Patch
Comment 12 Chris Dumez 2019-03-07 08:38:15 PST
Comment on attachment 363767 [details]
Patch

r=me
Comment 13 WebKit Commit Bot 2019-03-07 09:40:46 PST
Comment on attachment 363767 [details]
Patch

Clearing flags on attachment: 363767

Committed r242599: <https://trac.webkit.org/changeset/242599>
Comment 14 WebKit Commit Bot 2019-03-07 09:40:47 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2019-03-07 09:41:24 PST
<rdar://problem/48677961>