Bug 196807

Summary: Use normal loading path for ping loads
Product: WebKit Reporter: youenn fablet <youennf>
Component: Service WorkersAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, cdumez, commit-queue, cvazac, dbates, ews-watchlist, hi, japhet, joepeck, mkwst, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
ews-watchlist: commit-queue-
Archive of layout-test-results from ews101 for mac-highsierra
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews102 for mac-highsierra
none
Archive of layout-test-results from ews104 for mac-highsierra-wk2
none
Archive of layout-test-results from ews115 for mac-highsierra
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description youenn fablet 2019-04-10 20:57:39 PDT
Use normal loading path for ping loads
Comment 1 youenn fablet 2019-04-10 21:01:15 PDT
Created attachment 367198 [details]
Patch
Comment 2 EWS Watchlist 2019-04-10 21:03:19 PDT
Attachment 367198 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoadMap.h:58:  'take' is incorrectly named. It should be named 'protector' or 'protectedResourceLoadIdentifier'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/loader/LoaderStrategy.h:70:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 2 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 EWS Watchlist 2019-04-10 22:09:15 PDT
Comment on attachment 367198 [details]
Patch

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

New failing tests:
http/tests/security/contentSecurityPolicy/report-blocked-uri-and-do-not-follow-redirect-when-sending-report.php
http/tests/security/xssAuditor/report-script-tag-full-block-and-do-not-follow-redirect-when-sending-report.html
http/tests/security/xssAuditor/report-script-tag-and-do-not-follow-redirect-when-sending-report.html
http/tests/security/contentSecurityPolicy/user-style-sheet-font-crasher.php
Comment 4 EWS Watchlist 2019-04-10 22:09:17 PDT
Created attachment 367201 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 5 youenn fablet 2019-04-11 15:58:41 PDT
Created attachment 367255 [details]
Patch
Comment 6 EWS Watchlist 2019-04-11 16:01:14 PDT
Attachment 367255 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoadMap.h:58:  'take' is incorrectly named. It should be named 'protector' or 'protectedResourceLoadIdentifier'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/loader/LoaderStrategy.h:70:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 2 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 youenn fablet 2019-04-12 10:13:02 PDT
Created attachment 367328 [details]
Patch
Comment 8 EWS Watchlist 2019-04-12 10:15:20 PDT
Attachment 367328 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoadMap.h:58:  'take' is incorrectly named. It should be named 'protector' or 'protectedResourceLoadIdentifier'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/loader/LoaderStrategy.h:70:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 2 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 youenn fablet 2019-04-12 11:06:24 PDT
Created attachment 367334 [details]
Patch
Comment 10 EWS Watchlist 2019-04-12 11:08:41 PDT
Attachment 367334 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoadMap.h:58:  'take' is incorrectly named. It should be named 'protector' or 'protectedResourceLoadIdentifier'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/loader/LoaderStrategy.h:70:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 2 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 EWS Watchlist 2019-04-12 12:12:18 PDT
Comment on attachment 367334 [details]
Patch

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

New failing tests:
http/tests/security/contentSecurityPolicy/user-style-sheet-font-crasher.php
Comment 12 EWS Watchlist 2019-04-12 12:12:21 PDT
Created attachment 367339 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 13 EWS Watchlist 2019-04-12 12:25:58 PDT
Comment on attachment 367334 [details]
Patch

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

New failing tests:
http/tests/ssl/referer-301.html
Comment 14 EWS Watchlist 2019-04-12 12:25:59 PDT
Created attachment 367340 [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 15 EWS Watchlist 2019-04-12 13:10:05 PDT
Comment on attachment 367334 [details]
Patch

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

New failing tests:
http/tests/security/contentSecurityPolicy/user-style-sheet-font-crasher.php
Comment 16 EWS Watchlist 2019-04-12 13:10:07 PDT
Created attachment 367342 [details]
Archive of layout-test-results from ews115 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 17 youenn fablet 2019-04-12 15:49:18 PDT
Created attachment 367352 [details]
Patch
Comment 18 EWS Watchlist 2019-04-12 15:51:38 PDT
Attachment 367352 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoadMap.h:59:  'take' is incorrectly named. It should be named 'protector' or 'protectedResourceLoadIdentifier'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/loader/LoaderStrategy.h:70:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 2 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 youenn fablet 2019-04-12 17:15:35 PDT
Created attachment 367359 [details]
Patch
Comment 20 EWS Watchlist 2019-04-12 17:17:46 PDT
Attachment 367359 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoadMap.h:59:  'take' is incorrectly named. It should be named 'protector' or 'protectedResourceLoadIdentifier'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/loader/LoaderStrategy.h:70:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 2 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 youenn fablet 2019-04-24 10:33:38 PDT
Created attachment 368138 [details]
Patch
Comment 22 EWS Watchlist 2019-04-24 10:35:50 PDT
Attachment 368138 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoadMap.h:59:  'take' is incorrectly named. It should be named 'protector' or 'protectedResourceLoadIdentifier'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/loader/LoaderStrategy.h:70:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 2 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 23 Alex Christensen 2019-04-25 22:35:31 PDT
Comment on attachment 368138 [details]
Patch

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

I like this.  I would like it even more if we could completely remove PingLoad, but I guess that's coming.  This makes our loading code more like the current specification.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:775
>      case CachedResource::Type::Beacon:
> +    case CachedResource::Type::Ping:

Do we want these to be distinct types?  Will we add another type for fetch keep-alive?

> Source/WebKit/NetworkProcess/NetworkSession.h:109
> +    HashSet<Ref<NetworkResourceLoader>> m_keptAliveLoads;

I think it would be better if this were on the NetworkSession.  That would make cleanup a lot more intuitive when we remove a private browsing session, at which point I assume we would want to cancel all the outstanding pings, beacons, and keep-alive fetches, right?

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:577
> +    return !RuntimeEnabledFeatures::sharedFeatures().fetchAPIKeepAliveEnabled();

Won't we soon never want to use ping load in modern WebKit?
Comment 24 youenn fablet 2019-04-26 07:36:52 PDT
> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:775
> >      case CachedResource::Type::Beacon:
> > +    case CachedResource::Type::Ping:
> 
> Do we want these to be distinct types?  Will we add another type for fetch
> keep-alive?

Fetch keep-alive will be tied to regular fetch.
Ping are tied to some specific loads that are worth marking as such.

> > Source/WebKit/NetworkProcess/NetworkSession.h:109
> > +    HashSet<Ref<NetworkResourceLoader>> m_keptAliveLoads;
> 
> I think it would be better if this were on the NetworkSession.  That would
> make cleanup a lot more intuitive when we remove a private browsing session,
> at which point I assume we would want to cancel all the outstanding pings,
> beacons, and keep-alive fetches, right?

m_keptAliveLoads is in the NetworkSession.
I am not sure what you are suggesting here.

> > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:577
> > +    return !RuntimeEnabledFeatures::sharedFeatures().fetchAPIKeepAliveEnabled();
> 
> Won't we soon never want to use ping load in modern WebKit?

Definitely, I hope to enable it soon after it runs a few cycle on our bots.
Comment 25 youenn fablet 2019-04-26 07:38:08 PDT
(In reply to Alex Christensen from comment #23)
> Comment on attachment 368138 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=368138&action=review
> 
> I like this.  I would like it even more if we could completely remove
> PingLoad, but I guess that's coming.  This makes our loading code more like
> the current specification.

You r-ed it but I do not see any objection or suggestion for improvement.
Did you hit the wrong button?
Comment 26 Alex Christensen 2019-04-26 08:56:03 PDT
Comment on attachment 368138 [details]
Patch

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

r=me

> Source/WebKit/ChangeLog:11
> +        and add it to a kept-alive NetworkProcess load set. The loader is only kept alive if it

Oh, I still had this in my mind.  I guess this needs to be updated.
Comment 27 youenn fablet 2019-04-26 10:16:06 PDT
Created attachment 368329 [details]
Patch for landing
Comment 28 youenn fablet 2019-04-26 10:17:09 PDT
*** Bug 194392 has been marked as a duplicate of this bug. ***
Comment 29 EWS Watchlist 2019-04-26 10:17:46 PDT
Attachment 368329 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoadMap.h:59:  'take' is incorrectly named. It should be named 'protector' or 'protectedResourceLoadIdentifier'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/loader/LoaderStrategy.h:70:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 2 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 30 Radar WebKit Bug Importer 2019-04-26 10:19:31 PDT
<rdar://problem/50247053>
Comment 31 Radar WebKit Bug Importer 2019-04-26 10:19:34 PDT
<rdar://problem/50247059>
Comment 32 WebKit Commit Bot 2019-04-26 11:09:24 PDT
The commit-queue encountered the following flaky tests while processing attachment 368329 [details]:

fetch/fetch-worker-crash.html bug 187257 (author: youennf@gmail.com)
The commit-queue is continuing to process your patch.
Comment 33 WebKit Commit Bot 2019-04-26 11:10:23 PDT
Comment on attachment 368329 [details]
Patch for landing

Clearing flags on attachment: 368329

Committed r244700: <https://trac.webkit.org/changeset/244700>
Comment 34 WebKit Commit Bot 2019-04-26 11:10:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 35 Alex Christensen 2019-11-12 16:57:10 PST
Comment on attachment 368329 [details]
Patch for landing

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

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:488
> +    if (m_isKeptAlive) {
> +        m_responseCompletionHandler = WTFMove(completionHandler);
> +        RunLoop::main().dispatch([protectedThis = makeRef(*this)] {
> +            protectedThis->didFinishLoading(NetworkLoadMetrics { });
> +        });
> +        return;
> +    }

I'm simplifying this in https://bugs.webkit.org/show_bug.cgi?id=204134