Bug 175443

Summary: [Beacon] Add support for quota limitation
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebCore Misc.Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, dbates, ggaren, japhet, rniwa, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
URL: https://w3c.github.io/beacon/#sec-processing-model
Bug Depends on: 175482    
Bug Blocks: 147885, 175723    
Attachments:
Description Flags
WIP Patch
none
WIP Patch
buildbot: commit-queue-
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
WIP Patch
none
WIP Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2017-08-10 14:42:08 PDT
Add support for quota limitation as per:
- https://w3c.github.io/beacon/#sec-processing-model (step 5)

which points to:
- https://fetch.spec.whatwg.org/#http-network-or-cache-fetch (step 9)
Comment 1 Chris Dumez 2017-08-10 14:42:36 PDT
<rdar://problem/33729002>
Comment 2 Chris Dumez 2017-08-16 15:53:48 PDT
Created attachment 318298 [details]
WIP Patch
Comment 3 Chris Dumez 2017-08-16 16:04:06 PDT
Created attachment 318299 [details]
WIP Patch
Comment 4 Build Bot 2017-08-16 17:09:47 PDT
Comment on attachment 318299 [details]
WIP Patch

Attachment 318299 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4326946

New failing tests:
http/tests/security/contentSecurityPolicy/user-style-sheet-font-crasher.php
Comment 5 Build Bot 2017-08-16 17:09:48 PDT
Created attachment 318309 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 6 Build Bot 2017-08-16 17:33:25 PDT
Comment on attachment 318299 [details]
WIP Patch

Attachment 318299 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4326970

New failing tests:
http/tests/security/contentSecurityPolicy/user-style-sheet-font-crasher.php
Comment 7 Build Bot 2017-08-16 17:33:26 PDT
Created attachment 318311 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 8 Chris Dumez 2017-08-17 12:26:59 PDT
Created attachment 318399 [details]
WIP Patch
Comment 9 Chris Dumez 2017-08-17 16:57:08 PDT
Created attachment 318442 [details]
WIP Patch
Comment 10 Chris Dumez 2017-08-17 18:08:22 PDT
Created attachment 318451 [details]
Patch
Comment 11 youenn fablet 2017-08-17 19:10:22 PDT
Comment on attachment 318451 [details]
Patch

Looks good to me.
Two questions though:

What happens if the network process crashes?
Is there a way to call the completion handlers so as to free WebProcess pings?

Since we are having a completion handler, would it be worth passing an error if ping load fails?
That might be helpful to do some logging or use this in Web Inspector.
Comment 12 Chris Dumez 2017-08-17 19:14:08 PDT
(In reply to youenn fablet from comment #11)
> Comment on attachment 318451 [details]
> Patch
> 
> Looks good to me.
> Two questions though:
> 
> What happens if the network process crashes?
> Is there a way to call the completion handlers so as to free WebProcess
> pings?

I'll look into this.

> 
> Since we are having a completion handler, would it be worth passing an error
> if ping load fails?
> That might be helpful to do some logging or use this in Web Inspector.

For now this is not useful. It would be trivial to add it in a follow-up if we need it.
Comment 13 Chris Dumez 2017-08-17 19:18:01 PDT
Created attachment 318457 [details]
Patch
Comment 14 Chris Dumez 2017-08-17 19:19:11 PDT
Comment on attachment 318457 [details]
Patch

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

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:355
> +    for (auto& pingLoadCompletionHandler : pingLoadCompletionHandlers.values())

Now dealing with network process crashes better.
Comment 15 youenn fablet 2017-08-17 21:07:08 PDT
Comment on attachment 318457 [details]
Patch

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

> Source/WebKitLegacy/WebCoreSupport/WebResourceLoadScheduler.h:62
> +    void startPingLoad(WebCore::NetworkingContext*, WebCore::ResourceRequest&, const WebCore::HTTPHeaderMap&, Ref<WebCore::SecurityOrigin>&& sourceOrigin, WebCore::ContentSecurityPolicy*, const WebCore::FetchOptions&, WTF::Function<void()>&& completionHandler) override;

Should be final probably. Do we really need sourceOrigin parameter name here also?

On another topic related to the handling of headers, do we have a test checking referrer value after a cross-origin redirection and with a referrer policy like origin-when-cross-origin?
Looking at the code, I am not sure we are doing the right thing here.
Comment 16 youenn fablet 2017-08-18 07:42:21 PDT
> > Since we are having a completion handler, would it be worth passing an error
> > if ping load fails?
> > That might be helpful to do some logging or use this in Web Inspector.
> 
> For now this is not useful. It would be trivial to add it in a follow-up if
> we need it.

Adding some web inspector console logging would be good when ping load fails.
That might be handy for web developers to debug things like CORS checks, at least until web inspector fully supports ping loads in the network panel.
This would also mirror what is done for fetch.
Comment 17 Chris Dumez 2017-08-18 10:14:37 PDT
(In reply to youenn fablet from comment #16)
> > > Since we are having a completion handler, would it be worth passing an error
> > > if ping load fails?
> > > That might be helpful to do some logging or use this in Web Inspector.
> > 
> > For now this is not useful. It would be trivial to add it in a follow-up if
> > we need it.
> 
> Adding some web inspector console logging would be good when ping load fails.
> That might be handy for web developers to debug things like CORS checks, at
> least until web inspector fully supports ping loads in the network panel.
> This would also mirror what is done for fetch.

I started working on error reporting but this is a larger change. I'll upload a separate follow-up patch for it.
Comment 18 Chris Dumez 2017-08-18 10:16:31 PDT
Created attachment 318516 [details]
Patch
Comment 19 WebKit Commit Bot 2017-08-18 10:58:45 PDT
Comment on attachment 318516 [details]
Patch

Clearing flags on attachment: 318516

Committed r220922: <http://trac.webkit.org/changeset/220922>
Comment 20 WebKit Commit Bot 2017-08-18 10:58:47 PDT
All reviewed patches have been landed.  Closing bug.