Bug 184892 – Use NetworkLoadChecker for navigation loads

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[youenn fablet]

Use NetworkLoadChecker for navigation loads

[Radar WebKit Bug Importer]

<rdar://problem/39652686>

[youenn fablet]

Created attachment 338918 [details]
WIP

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[youenn fablet]

Created attachment 339020 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[youenn fablet]

Created attachment 339118 [details]
Patch

[youenn fablet]

Created attachment 339121 [details]
Patch

[youenn fablet]

Created attachment 339128 [details]
Patch

[youenn fablet]

Created attachment 339145 [details]
Patch

[Chris Dumez]

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?

[youenn fablet]

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.

[Chris Dumez]

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

[Chris Dumez]

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

[Chris Dumez]

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

[youenn fablet]

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

[Chris Dumez]

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

[youenn fablet]

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.

[youenn fablet]

Created attachment 339327 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 339327 [details]
Patch for landing

Clearing flags on attachment: 339327

Committed r231263: <https://trac.webkit.org/changeset/231263>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.