Bug 197463 - Reuse existing WebPageProxy quota handler for NetworkProcessProxy quota requests
Summary: Reuse existing WebPageProxy quota handler for NetworkProcessProxy quota requests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-05-01 08:44 PDT by youenn fablet
Modified: 2019-05-15 10:40 PDT (History)
7 users (show)

See Also:


Attachments
Patch (14.77 KB, patch)
2019-05-01 09:25 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (21.84 KB, patch)
2019-05-08 15:51 PDT, youenn fablet
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (3.00 MB, application/zip)
2019-05-08 17:01 PDT, Build Bot
no flags Details
Patch (35.59 KB, patch)
2019-05-10 13:22 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (36.21 KB, patch)
2019-05-14 12:51 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (36.39 KB, patch)
2019-05-14 14:59 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (36.26 KB, patch)
2019-05-14 15:48 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews215 for win-future (13.74 MB, application/zip)
2019-05-14 18:10 PDT, Build Bot
no flags Details
Patch for landing (36.38 KB, patch)
2019-05-15 09:50 PDT, 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-05-01 08:44:23 PDT
Reuse existing WebPageProxy quota handler for NetworkProcessProxy quota requests
Comment 1 youenn fablet 2019-05-01 08:46:15 PDT
<rdar://problem/47403621>
Comment 2 youenn fablet 2019-05-01 09:25:51 PDT
Created attachment 368677 [details]
Patch
Comment 3 youenn fablet 2019-05-08 15:51:08 PDT
Created attachment 369439 [details]
Patch
Comment 4 Build Bot 2019-05-08 17:01:54 PDT
Comment on attachment 369439 [details]
Patch

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

New failing tests:
http/wpt/cache-storage/quota-third-party.https.html
http/tests/IndexedDB/storage-limit.https.html
imported/w3c/IndexedDB-private-browsing/keyorder.html
inspector/unit-tests/objectStore/clear.html
inspector/unit-tests/objectStore/put.html
inspector/unit-tests/objectStore/putObject.html
inspector/unit-tests/objectStore/delete.html
http/wpt/cache-storage/cache-quota.any.html
http/wpt/cache-storage/cache-quota-after-restart.any.html
imported/w3c/IndexedDB-private-browsing/idbcursor_iterating.html
inspector/unit-tests/objectStore/basic.html
http/tests/IndexedDB/storage-limit-1.https.html
http/tests/IndexedDB/storage-limit-2.https.html
imported/w3c/IndexedDB-private-browsing/idbindex-multientry-big.html
http/wpt/cache-storage/cache-quota-add.any.html
inspector/unit-tests/objectStore/deleteObject.html
Comment 5 Build Bot 2019-05-08 17:01:55 PDT
Created attachment 369449 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 6 youenn fablet 2019-05-10 13:22:18 PDT
Created attachment 369591 [details]
Patch
Comment 7 Build Bot 2019-05-10 13:23:49 PDT
Attachment 369591 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:29:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:162:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:179:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 3 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 youenn fablet 2019-05-13 11:11:52 PDT
ping review
Comment 9 Alex Christensen 2019-05-13 15:05:57 PDT
Comment on attachment 369591 [details]
Patch

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

Seems ok, but needs some refinement.

> Source/WebKit/UIProcess/WebPageProxy.cpp:365
> +void WebPageProxy::forMostVisibleWebPageIfAny(PAL::SessionID sessionID, const SecurityOriginData& origin, CompletionHandler<void(WebPageProxy*)>&& completionHandler)

This seems kind of hacky.  It assumes that there is one dominant WKWebView, which I don't think is a good assumption in some apps.

> Source/WebKit/UIProcess/WebPageProxy.cpp:4385
> +    m_isQuotaIncreaseDenied = false;

Is this called during fragment navigations?  If it is, we might want to put this elsewhere, or keep track of this some other way.  Otherwise, a website could just do a quick hash navigation then ask again.  Please add a test that tries this.

> Source/WebKit/UIProcess/WebProcessProxy.cpp:125
> +void WebProcessProxy::forWebPages(PAL::SessionID sessionID, const SecurityOriginData& origin, const WTF::Function<void(WebPageProxy&)>& callback)

forEachWebPage
WTF:: unnecessary here and in header.  Same with WTF::CompletionHandler in WebPageProxy.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStoreClient.h:41
> +    virtual bool implementsRequestStorageSpaceHandler() const { return false; }
>      virtual void requestStorageSpace(const WebCore::SecurityOriginData& topOrigin, const WebCore::SecurityOriginData& frameOrigin, uint64_t quota, uint64_t currentSize, uint64_t spaceRequired, CompletionHandler<void(Optional<uint64_t>)>&& completionHandler)

I think we prefer to just pass in a CompletionHandler& and if it's null afterwards then it was used otherwise do something else with it.  That keeps our client interfaces a bit cleaner than adding a separate function to do the check.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:2
> + * Copyright (C) 2016 Apple Inc. All rights reserved.

2019

> Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:215
> +#if PLATFORM(MAC)
> +    [webView1.get().window setIsVisible:YES];
> +#else
> +    webView1.get().window.hidden = NO;
> +#endif

This is used 3 times here.  Maybe a good utility function.
Comment 10 youenn fablet 2019-05-13 15:45:32 PDT
(In reply to Alex Christensen from comment #9)
> Comment on attachment 369591 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=369591&action=review
> 
> Seems ok, but needs some refinement.
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:365
> > +void WebPageProxy::forMostVisibleWebPageIfAny(PAL::SessionID sessionID, const SecurityOriginData& origin, CompletionHandler<void(WebPageProxy*)>&& completionHandler)
> 
> This seems kind of hacky.  It assumes that there is one dominant WKWebView,
> which I don't think is a good assumption in some apps.

It assumes that in most cases, there is only one web view with a given origin that is visible at a time.
This heuristic can be improved over time.
If we find that the heuristic is broken in too many places, we will have to rely on the current WebsiteDataStore SPI.

> > Source/WebKit/UIProcess/WebPageProxy.cpp:4385
> > +    m_isQuotaIncreaseDenied = false;
> 
> Is this called during fragment navigations?  If it is, we might want to put
> this elsewhere, or keep track of this some other way.  Otherwise, a website
> could just do a quick hash navigation then ask again.  Please add a test
> that tries this.

A fragment navigation is never replacing the context so this cannot happen.

> 
> > Source/WebKit/UIProcess/WebProcessProxy.cpp:125
> > +void WebProcessProxy::forWebPages(PAL::SessionID sessionID, const SecurityOriginData& origin, const WTF::Function<void(WebPageProxy&)>& callback)
> 
> forEachWebPage
> WTF:: unnecessary here and in header.  Same with WTF::CompletionHandler in
> WebPageProxy.
> 
> > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStoreClient.h:41
> > +    virtual bool implementsRequestStorageSpaceHandler() const { return false; }
> >      virtual void requestStorageSpace(const WebCore::SecurityOriginData& topOrigin, const WebCore::SecurityOriginData& frameOrigin, uint64_t quota, uint64_t currentSize, uint64_t spaceRequired, CompletionHandler<void(Optional<uint64_t>)>&& completionHandler)
> 
> I think we prefer to just pass in a CompletionHandler& and if it's null
> afterwards then it was used otherwise do something else with it.  That keeps
> our client interfaces a bit cleaner than adding a separate function to do
> the check.

I have not seen this pattern and I do not really like it.
Also, this additional API is not visible to any application developer so this is purely internal.

I'll change the behavior so that calling completionHandler(WTF::nullOpt) means that there is no decision taken and a default decision should be made by WebKit.
Comment 11 youenn fablet 2019-05-14 12:51:51 PDT
Created attachment 369883 [details]
Patch
Comment 12 Build Bot 2019-05-14 12:53:09 PDT
Attachment 369883 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:29:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:162:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:179:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 3 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 youenn fablet 2019-05-14 13:28:51 PDT
(In reply to youenn fablet from comment #10)
> (In reply to Alex Christensen from comment #9)
> > Comment on attachment 369591 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=369591&action=review
> > 
> > Seems ok, but needs some refinement.
> > 
> > > Source/WebKit/UIProcess/WebPageProxy.cpp:365
> > > +void WebPageProxy::forMostVisibleWebPageIfAny(PAL::SessionID sessionID, const SecurityOriginData& origin, CompletionHandler<void(WebPageProxy*)>&& completionHandler)
> > 
> > This seems kind of hacky.  It assumes that there is one dominant WKWebView,
> > which I don't think is a good assumption in some apps.
> 
> It assumes that in most cases, there is only one web view with a given
> origin that is visible at a time.
> This heuristic can be improved over time.
> If we find that the heuristic is broken in too many places, we will have to
> rely on the current WebsiteDataStore SPI.

Discussed this with Alex and cleared up the fact that the selected page has the same origin as the request.
Comment 14 youenn fablet 2019-05-14 14:59:50 PDT
Created attachment 369897 [details]
Patch
Comment 15 Build Bot 2019-05-14 15:03:10 PDT
Attachment 369897 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:29:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:162:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:183:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 3 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 youenn fablet 2019-05-14 15:48:27 PDT
Created attachment 369902 [details]
Patch
Comment 17 Build Bot 2019-05-14 15:51:47 PDT
Attachment 369902 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:29:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:162:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:183:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 3 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Build Bot 2019-05-14 18:10:38 PDT
Comment on attachment 369902 [details]
Patch

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

New failing tests:
fast/shadow-dom/svg-thref-href-change-in-shadow-tree.html
Comment 19 Build Bot 2019-05-14 18:10:40 PDT
Created attachment 369916 [details]
Archive of layout-test-results from ews215 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews215  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 20 Alex Christensen 2019-05-15 07:34:24 PDT
Comment on attachment 369902 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:297
> +    bool m_areBeingProcessing { false };

m_isProcessing or m_requestsAreBeingProcessed

> Source/WebKit/UIProcess/WebPageProxy.cpp:7262
> +            StorageRequests::singleton().processNextIfAny();

I think it would be a better abstraction if the StorageRequests made its own lambda that called this.

> Source/WebKit/UIProcess/WebProcessProxy.cpp:125
> +void WebProcessProxy::forWebPages(PAL::SessionID sessionID, const SecurityOriginData& origin, const Function<void(WebPageProxy&)>& callback)

forWebPagesWithOrigin
Comment 21 youenn fablet 2019-05-15 08:50:40 PDT
Thanks for the review.

(In reply to Alex Christensen from comment #20)
> Comment on attachment 369902 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=369902&action=review
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:297
> > +    bool m_areBeingProcessing { false };
> 
> m_isProcessing or m_requestsAreBeingProcessed

OK

> > Source/WebKit/UIProcess/WebPageProxy.cpp:7262
> > +            StorageRequests::singleton().processNextIfAny();
> 
> I think it would be a better abstraction if the StorageRequests made its own
> lambda that called this.

Agreed, I thought about this.
My initial idea was to pass to the request completion handler another completion handler for the purpose of calling StorageRequests::singleton().processNextIfAny().
I concluded this was less readable since we end up with two completion handlers.

> > Source/WebKit/UIProcess/WebProcessProxy.cpp:125
> > +void WebProcessProxy::forWebPages(PAL::SessionID sessionID, const SecurityOriginData& origin, const Function<void(WebPageProxy&)>& callback)
> 
> forWebPagesWithOrigin

OK
Comment 22 youenn fablet 2019-05-15 09:50:45 PDT
Created attachment 369960 [details]
Patch for landing
Comment 23 Build Bot 2019-05-15 09:52:37 PDT
Attachment 369960 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:29:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:162:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/StorageQuota.mm:183:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 3 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 24 WebKit Commit Bot 2019-05-15 10:40:07 PDT
Comment on attachment 369960 [details]
Patch for landing

Clearing flags on attachment: 369960

Committed r245328: <https://trac.webkit.org/changeset/245328>
Comment 25 WebKit Commit Bot 2019-05-15 10:40:09 PDT
All reviewed patches have been landed.  Closing bug.