Bug 184763 – Make WebLoaderStrategy send to NetworkResourceLoader necessary parameters to handle full loads in NetworkProcess

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[youenn fablet]

We need to pass preflight policy, CSP response headers, origin and content blockers from DocumentThreadableLoader up to NetworkProcess through WebLoaderStrategy

[youenn fablet]

Created attachment 338287 [details]
Patch

[EWS Watchlist]

Comment on attachment 338287 [details]
Patch

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

New failing tests:
http/tests/inspector/network/fetch-network-data.html

[EWS Watchlist]

Created attachment 338292 [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 338287 [details]
Patch

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

New failing tests:
http/wpt/service-workers/header-filtering.https.html

[EWS Watchlist]

Created attachment 338294 [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 338324 [details]
Patch

[youenn fablet]

gtk-wk2 failure is probably not relevant, log shows "virtual memory exhausted: Cannot allocate memory"

[Chris Dumez]

Comment on attachment 338324 [details]
Patch

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

r=me

> Source/WebCore/ChangeLog:10
> +       Add PreflightPolicy and CSP response headers as ResourceLoaderOptions.

Indentation problem.

> Source/WebCore/ChangeLog:13
> +       Allow getting the original headers from a SubresourceLoader.

ditto.

> Source/WebCore/loader/ResourceLoaderOptions.h:116
> +    ConsiderPreflight,

I think we could even consider dropping the "Preflight" suffix given the enum name.

> Source/WebCore/loader/SubresourceLoader.cpp:787
> +std::optional<HTTPHeaderMap> SubresourceLoader::originalHeaders() const

If we returned a HTTPHeaderMap* instead of an std::optional<>, we could avoid copying the headers.

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:287
> +    if (resourceLoader.isSubresourceLoader()) {

It would be nice to add the template specializations so that is<SubresourceLoader>() / downcast<SubresourceLoader>() works. Would be safer too.

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:289
> +            loadParameters.originalRequestHeaders = WTFMove(headers.value());

I get that we have to copy the headers here anyway, I'd still prefer if the API on SubresourceLoader did not requires the clients to in case they do not need/want copying.

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:295
> +        // FIXME: Instead of passing userContentControllerIdentifier, we should just pass webPageId to NetworkProcess.

I thought we already passed the PageID / FrameID to the network process so I do not understand this comment.

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:296
> +        WebFrameLoaderClient* webFrameLoaderClient = toWebFrameLoaderClient(resourceLoader.frame()->loader().client());

auto*

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:297
> +        WebFrame* webFrame = webFrameLoaderClient ? webFrameLoaderClient->webFrame() : nullptr;

auto*

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:298
> +        WebPage* webPage = webFrame ? webFrame->page() : nullptr;

auto*

[youenn fablet]

Thanks for the review.

> > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:287
> > +    if (resourceLoader.isSubresourceLoader()) {
> 
> It would be nice to add the template specializations so that
> is<SubresourceLoader>() / downcast<SubresourceLoader>() works. Would be
> safer too.

Agreed, I was tempted to do it in that patch but I resisted it.

> > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:289
> > +            loadParameters.originalRequestHeaders = WTFMove(headers.value());
> 
> I get that we have to copy the headers here anyway, I'd still prefer if the
> API on SubresourceLoader did not requires the clients to in case they do not
> need/want copying.

OK, will see what I can do.
Note that we would not have to copy it if we were passing it as a direct IPC parameter.
Maybe we should do that instead.

> > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:295
> > +        // FIXME: Instead of passing userContentControllerIdentifier, we should just pass webPageId to NetworkProcess.
> 
> I thought we already passed the PageID / FrameID to the network process so I
> do not understand this comment.

NetworkProcess has webPageID but no logic yet to compute userContentControllerIdentifier from it.

[youenn fablet]

Created attachment 338624 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 338624 [details]
Patch

Clearing flags on attachment: 338624

Committed r230942: <https://trac.webkit.org/changeset/230942>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.