Bug 196807 – Use normal loading path for ping loads

• Status: RESOLVED
• Resolution: FIXED
• Component: Service Workers

[youenn fablet]

Use normal loading path for ping loads

[youenn fablet]

Created attachment 367198 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[youenn fablet]

Created attachment 367255 [details]
Patch

[EWS Watchlist]

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.

[youenn fablet]

Created attachment 367328 [details]
Patch

[EWS Watchlist]

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.

[youenn fablet]

Created attachment 367334 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[youenn fablet]

Created attachment 367352 [details]
Patch

[EWS Watchlist]

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.

[youenn fablet]

Created attachment 367359 [details]
Patch

[EWS Watchlist]

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.

[youenn fablet]

Created attachment 368138 [details]
Patch

[EWS Watchlist]

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.

[Alex Christensen]

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?

[youenn fablet]

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

[youenn fablet]

(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?

[Alex Christensen]

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.

[youenn fablet]

Created attachment 368329 [details]
Patch for landing

[youenn fablet]

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

[EWS Watchlist]

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.

[Radar WebKit Bug Importer]

<rdar://problem/50247053>

[Radar WebKit Bug Importer]

<rdar://problem/50247059>

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 368329 [details]:

fetch/fetch-worker-crash.html bug 187257 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 368329 [details]
Patch for landing

Clearing flags on attachment: 368329

Committed r244700: <https://trac.webkit.org/changeset/244700>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alex Christensen]

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