Bug 184892 - Use NetworkLoadChecker for navigation loads
Summary: Use NetworkLoadChecker for navigation loads
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-04-23 11:12 PDT by youenn fablet
Modified: 2018-05-02 14:13 PDT (History)
8 users (show)

See Also:


Attachments
WIP (11.46 KB, patch)
2018-04-26 14:47 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.30 MB, application/zip)
2018-04-26 16:17 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (2.78 MB, application/zip)
2018-04-26 17:50 PDT, Build Bot
no flags Details
Patch (22.91 KB, patch)
2018-04-27 13:57 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-sierra-wk2 (2.87 MB, application/zip)
2018-04-27 15:18 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (9.06 MB, application/zip)
2018-04-27 18:06 PDT, Build Bot
no flags Details
Patch (25.04 KB, patch)
2018-04-30 09:55 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (2.73 KB, patch)
2018-04-30 10:03 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (25.06 KB, patch)
2018-04-30 10:56 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (25.09 KB, patch)
2018-04-30 13:38 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (24.54 KB, patch)
2018-05-02 13:12 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 2018-04-23 11:12:30 PDT
Use NetworkLoadChecker for navigation loads
Comment 1 Radar WebKit Bug Importer 2018-04-23 11:13:12 PDT
<rdar://problem/39652686>
Comment 2 youenn fablet 2018-04-26 14:47:10 PDT
Created attachment 338918 [details]
WIP
Comment 3 Build Bot 2018-04-26 16:17:01 PDT
Comment on attachment 338918 [details]
WIP

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

New failing tests:
http/wpt/service-workers/header-filtering.https.html
Comment 4 Build Bot 2018-04-26 16:17:02 PDT
Created attachment 338935 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 5 Build Bot 2018-04-26 17:50:41 PDT
Comment on attachment 338918 [details]
WIP

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

New failing tests:
http/tests/webarchive/test-preload-resources.html
http/tests/webarchive/cross-origin-stylesheet-crash.html
Comment 6 Build Bot 2018-04-26 17:50:42 PDT
Created attachment 338944 [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
Comment 7 youenn fablet 2018-04-27 13:57:02 PDT
Created attachment 339020 [details]
Patch
Comment 8 Build Bot 2018-04-27 15:18:56 PDT
Comment on attachment 339020 [details]
Patch

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

New failing tests:
http/tests/contentextensions/block-everything-unless-domain-redirect.php
imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-sync-block-scripts.html
http/tests/misc/redirect-to-about-blank.html
http/tests/contentextensions/main-resource-redirect-blocked.php
http/tests/contentextensions/main-resource-redirect-error.html
imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-sync-not-hang-scriptloader.html
fast/files/file-reader-sandbox-iframe.html
imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-sync-block-defer-scripts.html
fast/files/file-reader-file-url.html
http/tests/contentextensions/block-everything-unless-domain-iframe.html
Comment 9 Build Bot 2018-04-27 15:18:57 PDT
Created attachment 339033 [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
Comment 10 Build Bot 2018-04-27 18:06:37 PDT
Comment on attachment 339020 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-sync-block-scripts.html
http/tests/misc/redirect-to-about-blank.html
imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-sync-not-hang-scriptloader.html
fast/files/file-reader-sandbox-iframe.html
imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-sync-block-defer-scripts.html
fast/files/file-reader-file-url.html
Comment 11 Build Bot 2018-04-27 18:06:39 PDT
Created attachment 339045 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 12 youenn fablet 2018-04-30 09:55:47 PDT
Created attachment 339118 [details]
Patch
Comment 13 youenn fablet 2018-04-30 10:03:43 PDT
Created attachment 339121 [details]
Patch
Comment 14 youenn fablet 2018-04-30 10:56:16 PDT
Created attachment 339128 [details]
Patch
Comment 15 youenn fablet 2018-04-30 13:38:52 PDT
Created attachment 339145 [details]
Patch
Comment 16 Chris Dumez 2018-05-02 12:29:19 PDT
Comment on attachment 339145 [details]
Patch

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

r=me

> Source/WebCore/platform/network/ResourceResponseBase.cpp:392
> +void ResourceResponseBase::sanitizeHTTPHeaderFieldsAccordingTainting()

I think AccordingToTainting sounds better.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:325
> +    // FIXME: Enable content blockers for navigation loads.

This would be a pretty bad regression so I do not think we should enable this code path by default until this is supported.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:615
> +        if (!newRequest.url().protocolIsInHTTPFamily() && !newRequest.url().protocolIs("about") && m_redirectCount) {

isBlankURL() ?

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:667
> +    return RuntimeEnabledFeatures::sharedFeatures().restrictedHTTPResponseAccess();

This is still disabled by default, right?
Comment 17 youenn fablet 2018-05-02 12:40:53 PDT
Thanks for the review.

> > Source/WebCore/platform/network/ResourceResponseBase.cpp:392
> > +void ResourceResponseBase::sanitizeHTTPHeaderFieldsAccordingTainting()
> 
> I think AccordingToTainting sounds better.

OK

> > Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:325
> > +    // FIXME: Enable content blockers for navigation loads.
> 
> This would be a pretty bad regression so I do not think we should enable
> this code path by default until this is supported.

Content blockers are enabled in WebProcess so there is no regression.
The idea would be to enable content blockers in NetworkProcess for navigation loads so as to remove the need to handle redirections in WebProcess.

> > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:615
> > +        if (!newRequest.url().protocolIsInHTTPFamily() && !newRequest.url().protocolIs("about") && m_redirectCount) {
> 
> isBlankURL() ?

I saw on iOS "about:" being sent to NetworkProcess in QuickLook code path.

I thought isBlankURL was for "about:blank" but it seems it is for all about URLs.
I am not sure I like the name isBlankURL.

> > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:667
> > +    return RuntimeEnabledFeatures::sharedFeatures().restrictedHTTPResponseAccess();
> 
> This is still disabled by default, right?

This is enabled by default now.
Comment 18 Chris Dumez 2018-05-02 12:42:04 PDT
Comment on attachment 339145 [details]
Patch

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

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:615
>> +        if (!newRequest.url().protocolIsInHTTPFamily() && !newRequest.url().protocolIs("about") && m_redirectCount) {
> 
> isBlankURL() ?

What I am asking is why aren't you using !newRequest.url().isBlankURL() instead of !newRequest.url().protocolIs("about").
Comment 19 Chris Dumez 2018-05-02 12:43:02 PDT
(In reply to youenn fablet from comment #17)
> Thanks for the review.
> 
> > > Source/WebCore/platform/network/ResourceResponseBase.cpp:392
> > > +void ResourceResponseBase::sanitizeHTTPHeaderFieldsAccordingTainting()
> > 
> > I think AccordingToTainting sounds better.
> 
> OK
> 
> > > Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:325
> > > +    // FIXME: Enable content blockers for navigation loads.
> > 
> > This would be a pretty bad regression so I do not think we should enable
> > this code path by default until this is supported.
> 
> Content blockers are enabled in WebProcess so there is no regression.
> The idea would be to enable content blockers in NetworkProcess for
> navigation loads so as to remove the need to handle redirections in
> WebProcess.
> 
> > > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:615
> > > +        if (!newRequest.url().protocolIsInHTTPFamily() && !newRequest.url().protocolIs("about") && m_redirectCount) {
> > 
> > isBlankURL() ?
> 
> I saw on iOS "about:" being sent to NetworkProcess in QuickLook code path.
> 
> I thought isBlankURL was for "about:blank" but it seems it is for all about
> URLs.
> I am not sure I like the name isBlankURL.
> 
> > > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:667
> > > +    return RuntimeEnabledFeatures::sharedFeatures().restrictedHTTPResponseAccess();
> > 
> > This is still disabled by default, right?
> 
> This is enabled by default now.

If so, then I do not think it is OK to land as-is. Losing content blocking for navigated resources seems like a big no-no to me.
Comment 20 Chris Dumez 2018-05-02 12:43:37 PDT
(In reply to Chris Dumez from comment #19)
> (In reply to youenn fablet from comment #17)
> > Thanks for the review.
> > 
> > > > Source/WebCore/platform/network/ResourceResponseBase.cpp:392
> > > > +void ResourceResponseBase::sanitizeHTTPHeaderFieldsAccordingTainting()
> > > 
> > > I think AccordingToTainting sounds better.
> > 
> > OK
> > 
> > > > Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:325
> > > > +    // FIXME: Enable content blockers for navigation loads.
> > > 
> > > This would be a pretty bad regression so I do not think we should enable
> > > this code path by default until this is supported.
> > 
> > Content blockers are enabled in WebProcess so there is no regression.
> > The idea would be to enable content blockers in NetworkProcess for
> > navigation loads so as to remove the need to handle redirections in
> > WebProcess.
> > 
> > > > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:615
> > > > +        if (!newRequest.url().protocolIsInHTTPFamily() && !newRequest.url().protocolIs("about") && m_redirectCount) {
> > > 
> > > isBlankURL() ?
> > 
> > I saw on iOS "about:" being sent to NetworkProcess in QuickLook code path.
> > 
> > I thought isBlankURL was for "about:blank" but it seems it is for all about
> > URLs.
> > I am not sure I like the name isBlankURL.
> > 
> > > > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:667
> > > > +    return RuntimeEnabledFeatures::sharedFeatures().restrictedHTTPResponseAccess();
> > > 
> > > This is still disabled by default, right?
> > 
> > This is enabled by default now.
> 
> If so, then I do not think it is OK to land as-is. Losing content blocking
> for navigated resources seems like a big no-no to me.

^^ Alex&Brady&Geoff.
Comment 21 youenn fablet 2018-05-02 12:44:47 PDT
> > If so, then I do not think it is OK to land as-is. Losing content blocking
> > for navigated resources seems like a big no-no to me.

As I said above, we are not losing content blocking, this is still done in WebProcess.
In the future, we should enable content blocking for navigation loads in NetworkProcess so that we do not need to do these in WebProcess.
Comment 22 Chris Dumez 2018-05-02 12:45:46 PDT
(In reply to youenn fablet from comment #21)
> > > If so, then I do not think it is OK to land as-is. Losing content blocking
> > > for navigated resources seems like a big no-no to me.
> 
> As I said above, we are not losing content blocking, this is still done in
> WebProcess.
> In the future, we should enable content blocking for navigation loads in
> NetworkProcess so that we do not need to do these in WebProcess.

Oh ok, I missed this part, sorry. Then I'm fine with it.
Comment 23 youenn fablet 2018-05-02 12:51:36 PDT
For the record, I disabled content blocking for navigation loads for two reasons.
First, we need to improve the error logging for content blockers like we did for CSP.
Second, content blockers in NetworkProcess are currently mostly useful for sync XHR and ping loads.We might need further work/testing for other loads, in particular navigation loads.
Comment 24 youenn fablet 2018-05-02 13:12:53 PDT
Created attachment 339327 [details]
Patch for landing
Comment 25 WebKit Commit Bot 2018-05-02 14:13:33 PDT
Comment on attachment 339327 [details]
Patch for landing

Clearing flags on attachment: 339327

Committed r231263: <https://trac.webkit.org/changeset/231263>
Comment 26 WebKit Commit Bot 2018-05-02 14:13:35 PDT
All reviewed patches have been landed.  Closing bug.