Bug 185849 – NetworkLoadChecker should check cached redirections

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[youenn fablet]

NetworkLoadChecker should check cached redirections

[youenn fablet]

Created attachment 340918 [details]
Patch

[EWS Watchlist]

Comment on attachment 340918 [details]
Patch

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

New failing tests:
http/tests/cache/disk-cache/redirect-chain-limits.html
http/tests/fetch/redirectmode-and-preload.html

[EWS Watchlist]

Created attachment 340931 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 340918 [details]
Patch

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

New failing tests:
http/tests/cache/disk-cache/redirect-chain-limits.html
http/tests/fetch/redirectmode-and-preload.html

[EWS Watchlist]

Created attachment 340934 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[youenn fablet]

Created attachment 340949 [details]
Patch

[EWS Watchlist]

Comment on attachment 340949 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/redirected-response.https.html
http/tests/cache/disk-cache/redirect-chain-limits.html

[EWS Watchlist]

Created attachment 340951 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[youenn fablet]

Created attachment 340991 [details]
Patch

[youenn fablet]

Created attachment 340994 [details]
Patch

[youenn fablet]

Created attachment 341022 [details]
Patch

[Chris Dumez]

Comment on attachment 341022 [details]
Patch

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

> Source/WebCore/loader/SubresourceLoader.cpp:212
> +                ResourceError error { errorDomainWebKitInternal, 0, request().url(), makeString("Load cannot follow redirection for ", request().url().string()), ResourceError::Type::AccessControl };

"redirection to" sounds better than "redirection for". Also, the previous text was clear that it was a security error by saying "not allowed". The new message merely says it cannot follow the redirection, without giving the reason.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:108
> +        handler(accessControlErrorForValidationHandler(makeString("Load cannot follow redirection for ", redirectResponse.url().string())));

ditto.

[youenn fablet]

OK, will replace with the following:
"Not allowed to follow a redirection while loading XXX"

(In reply to Chris Dumez from comment #12)
> Comment on attachment 341022 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341022&action=review
> 
> > Source/WebCore/loader/SubresourceLoader.cpp:212
> > +                ResourceError error { errorDomainWebKitInternal, 0, request().url(), makeString("Load cannot follow redirection for ", request().url().string()), ResourceError::Type::AccessControl };
> 
> "redirection to" sounds better than "redirection for". Also, the previous
> text was clear that it was a security error by saying "not allowed". The new
> message merely says it cannot follow the redirection, without giving the
> reason.
> 
> > Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:108
> > +        handler(accessControlErrorForValidationHandler(makeString("Load cannot follow redirection for ", redirectResponse.url().string())));
> 
> ditto.

[youenn fablet]

Created attachment 341050 [details]
Patch

[Chris Dumez]

Comment on attachment 341050 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:679
> +    networkLoadMetrics.markComplete();

Why all this? The previous code was just using a default initialized NetworkLoadMetrics.

[youenn fablet]

> > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:679
> > +    networkLoadMetrics.markComplete();
> 
> Why all this? The previous code was just using a default initialized
> NetworkLoadMetrics.

Since we finish the load, we should be marking the metric as complete.
We haven't received any body yet, so we can set 0 to body received/decoded.
I will remove setting the other fields though since we do not have that information.

[youenn fablet]

Created attachment 341108 [details]
Remove setting some metric values

[WebKit Commit Bot]

Comment on attachment 341108 [details]
Remove setting some metric values

Clearing flags on attachment: 341108

Committed r232121: <https://trac.webkit.org/changeset/232121>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40494164>