Bug 181789 – Link headers for subresources are not being processed

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Yoav Weiss]

While investigating test failures on https://bugs.webkit.org/show_bug.cgi?id=181657 I've seen that the ios-simulator build does not get the Link header values when called loadLinksFromHeader is called for subresources. I'm not sure regarding the cause, but the test seems to pass in all other platform that preconnect is working on.

[Yoav Weiss]

Looking further into this, this is not ios-simulator specific.

[Yoav Weiss]

Created attachment 332131 [details]
Patch

[EWS Watchlist]

Comment on attachment 332131 [details]
Patch

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

New failing tests:
http/tests/preload/single_download_preload_headers.php
http/tests/preload/preload-encoding.php
http/tests/preload/download_resources_from_invalid_headers.html
http/tests/preload/download_resources_from_header_iframe.html

[EWS Watchlist]

Created attachment 332139 [details]
Archive of layout-test-results from ews101 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 332131 [details]
Patch

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

New failing tests:
http/tests/preload/single_download_preload_headers.php
http/tests/preload/download_resources_from_header_iframe.html
http/tests/preload/download_resources_from_invalid_headers.html

[EWS Watchlist]

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

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

[EWS Watchlist]

Comment on attachment 332131 [details]
Patch

Attachment 332131 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6192908

New failing tests:
http/tests/preload/single_download_preload_headers.php
http/tests/preload/preload-encoding.php
http/tests/preload/download_resources_from_invalid_headers.html
http/tests/preload/download_resources_from_header_iframe.html

[EWS Watchlist]

Created attachment 332142 [details]
Archive of layout-test-results from ews113 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 332131 [details]
Patch

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

New failing tests:
http/tests/preload/single_download_preload_headers.php
http/tests/preload/download_resources_from_header_iframe.html
http/tests/preload/download_resources_from_invalid_headers.html

[EWS Watchlist]

Created attachment 332143 [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.12.6

[EWS Watchlist]

Comment on attachment 332131 [details]
Patch

Attachment 332131 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/6193585

New failing tests:
http/tests/preload/download_resources_from_header_iframe.html
http/tests/preload/single_download_preload_headers.php
http/tests/preload/preload-encoding.php
http/tests/preload/download_resources_from_invalid_headers.html
http/tests/misc/bubble-drag-events.html

[EWS Watchlist]

Created attachment 332145 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Yoav Weiss]

Created attachment 334681 [details]
Patch

[Yoav Weiss]

Created attachment 334684 [details]
Patch

[Yoav Weiss]

Finally found the time to get back to this issue. Alex, Chris or Youenn, care to take a look?

[youenn fablet]

Comment on attachment 334684 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Triggers Link header processing when the Link headers arrive on a subresource.

Sounds fine to me. Some suggestions below.

We might want at some point to move some important parts of loading code in NetworkProcess.
Preload would also benefit from that since that would make it potentially faster.
Current implementation of preloads might not be easy to move since it is placed in various bits.
I wonder what your thoughts are there.

> Source/WebCore/loader/SubresourceLoader.cpp:363
> +    if (m_documentLoader->url() != request().url())

This cheks whether it is a subresource or a document load, right?
I would do the check based on options.mode != Mode::Navigation.

> Source/WebCore/loader/SubresourceLoader.cpp:364
> +        LinkLoader::loadLinksFromHeader(response.httpHeaderField(HTTPHeaderName::Link), m_documentLoader->url(), *m_frame->document(), LinkLoader::MediaAttributeCheck::SkipMediaAttributeCheck);

I am a little unclear on why we need SkipMediaAttributeCheck. Is there a spec that requires that somewhere?

> LayoutTests/http/tests/preload/link-header-on-subresource.html:10
> +<script src="http://127.0.0.1:8000/resources/slow-script.pl?delay=400"></script>

Is there a potential flakiness issue there?
Shouldn't we try several times to check for internals.isPreloaded until it succeeds?

Ideally, we would want these tests in WPT, right?
We could try to use WPT server and doing preloads on URLs served by python scripts that would save some state.
We would then get the state making another WPT server request, to check that all preloads are done/not done.

[Yoav Weiss]

(In reply to youenn fablet from comment #16)
> Comment on attachment 334684 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334684&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Triggers Link header processing when the Link headers arrive on a subresource.
> 
> Sounds fine to me. Some suggestions below.
> 
> We might want at some point to move some important parts of loading code in
> NetworkProcess.
> Preload would also benefit from that since that would make it potentially
> faster.
> Current implementation of preloads might not be easy to move since it is
> placed in various bits.
> I wonder what your thoughts are there.

Faster sounds better :)
Is there a design doc you can share on that move?

> 
> > Source/WebCore/loader/SubresourceLoader.cpp:363
> > +    if (m_documentLoader->url() != request().url())
> 
> This cheks whether it is a subresource or a document load, right?
> I would do the check based on options.mode != Mode::Navigation.

Yeah, that's significantly better :)

> 
> > Source/WebCore/loader/SubresourceLoader.cpp:364
> > +        LinkLoader::loadLinksFromHeader(response.httpHeaderField(HTTPHeaderName::Link), m_documentLoader->url(), *m_frame->document(), LinkLoader::MediaAttributeCheck::SkipMediaAttributeCheck);
> 
> I am a little unclear on why we need SkipMediaAttributeCheck. Is there a
> spec that requires that somewhere?

This is an implementation issue. Link headers for navigations are handled in two separate location depending if they have a media attribute or not. (without media attribute they are handled before the PreloadScanner, and with it they are handled after it, once we know the viewport)
For subresources, there's no need for that check, as the viewport is already known.

> 
> > LayoutTests/http/tests/preload/link-header-on-subresource.html:10
> > +<script src="http://127.0.0.1:8000/resources/slow-script.pl?delay=400"></script>
> 
> Is there a potential flakiness issue there?
> Shouldn't we try several times to check for internals.isPreloaded until it
> succeeds?

I can change the test to do that.

> 
> Ideally, we would want these tests in WPT, right?
> We could try to use WPT server and doing preloads on URLs served by python
> scripts that would save some state.
> We would then get the state making another WPT server request, to check that
> all preloads are done/not done.

There's already a similar test on WPT: https://github.com/w3c/web-platform-tests/blob/master/preload/link-header-on-subresource.html

I should probably take some time soon to unite these test suites.
I guess I should put some time to unite those two test suites

[Yoav Weiss]

Created attachment 334919 [details]
Patch

[youenn fablet]

> > We might want at some point to move some important parts of loading code in
> > NetworkProcess.
> > Preload would also benefit from that since that would make it potentially
> > faster.
> > Current implementation of preloads might not be easy to move since it is
> > placed in various bits.
> > I wonder what your thoughts are there.
> 
> Faster sounds better :)
> Is there a design doc you can share on that move?

Not yet unfortunately.
We would like to handle all redirections in network process with potential interactions with UIProcess but no interaction with WebProcess.

[WebKit Commit Bot]

Comment on attachment 334919 [details]
Patch

Clearing flags on attachment: 334919

Committed r229196: <https://trac.webkit.org/changeset/229196>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/38097208>