Bug 195283 – Introduce a quota manager for Cache API/Service Worker/IDB storage

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[youenn fablet]

Introduce a quota manager for Cache API/Service Worker/IDB storage

[youenn fablet]

Created attachment 363560 [details]
Patch

[youenn fablet]

Created attachment 363600 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[youenn fablet]

Comment on attachment 363615 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

Errors are probably unrelated

[Chris Dumez]

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 ?

[youenn fablet]

Created attachment 363704 [details]
Patch

[youenn fablet]

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

[youenn fablet]

Created attachment 363705 [details]
Patch

[youenn fablet]

Created attachment 363706 [details]
Patch

[youenn fablet]

Created attachment 363767 [details]
Patch

[Chris Dumez]

Comment on attachment 363767 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 363767 [details]
Patch

Clearing flags on attachment: 363767

Committed r242599: <https://trac.webkit.org/changeset/242599>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48677961>

[Sihui Liu]

*** Bug 106507 has been marked as a duplicate of this bug. ***