Bug 181789 - Link headers for subresources are not being processed
Summary: Link headers for subresources are not being processed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Other
: P2 Minor
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-18 01:37 PST by Yoav Weiss
Modified: 2018-03-03 00:59 PST (History)
11 users (show)

See Also:


Attachments
Patch (4.55 KB, patch)
2018-01-24 00:58 PST, Yoav Weiss
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.19 MB, application/zip)
2018-01-24 01:58 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews105 for mac-sierra-wk2 (2.57 MB, application/zip)
2018-01-24 02:03 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews113 for mac-sierra (2.92 MB, application/zip)
2018-01-24 02:25 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.17 MB, application/zip)
2018-01-24 02:27 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews206 for win-future (12.08 MB, application/zip)
2018-01-24 03:56 PST, EWS Watchlist
no flags Details
Patch (4.85 KB, patch)
2018-02-27 01:46 PST, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (7.20 KB, patch)
2018-02-27 02:13 PST, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (6.99 KB, patch)
2018-03-02 12:58 PST, Yoav Weiss
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yoav Weiss 2018-01-18 01:37:08 PST
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.
Comment 1 Yoav Weiss 2018-01-24 00:55:50 PST
Looking further into this, this is not ios-simulator specific.
Comment 2 Yoav Weiss 2018-01-24 00:58:17 PST
Created attachment 332131 [details]
Patch
Comment 3 EWS Watchlist 2018-01-24 01:58:18 PST
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
Comment 4 EWS Watchlist 2018-01-24 01:58:19 PST
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
Comment 5 EWS Watchlist 2018-01-24 02:03:35 PST
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
Comment 6 EWS Watchlist 2018-01-24 02:03:37 PST
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
Comment 7 EWS Watchlist 2018-01-24 02:25:03 PST
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
Comment 8 EWS Watchlist 2018-01-24 02:25:04 PST
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
Comment 9 EWS Watchlist 2018-01-24 02:27:28 PST
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
Comment 10 EWS Watchlist 2018-01-24 02:27:29 PST
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
Comment 11 EWS Watchlist 2018-01-24 03:56:17 PST
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
Comment 12 EWS Watchlist 2018-01-24 03:56:28 PST
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
Comment 13 Yoav Weiss 2018-02-27 01:46:17 PST
Created attachment 334681 [details]
Patch
Comment 14 Yoav Weiss 2018-02-27 02:13:59 PST
Created attachment 334684 [details]
Patch
Comment 15 Yoav Weiss 2018-02-27 05:45:27 PST
Finally found the time to get back to this issue. Alex, Chris or Youenn, care to take a look?
Comment 16 youenn fablet 2018-03-02 08:07:00 PST
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.
Comment 17 Yoav Weiss 2018-03-02 12:49:48 PST
(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
Comment 18 Yoav Weiss 2018-03-02 12:58:32 PST
Created attachment 334919 [details]
Patch
Comment 19 youenn fablet 2018-03-02 13:26:08 PST
> > 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.
Comment 20 WebKit Commit Bot 2018-03-03 00:57:38 PST
Comment on attachment 334919 [details]
Patch

Clearing flags on attachment: 334919

Committed r229196: <https://trac.webkit.org/changeset/229196>
Comment 21 WebKit Commit Bot 2018-03-03 00:57:40 PST
All reviewed patches have been landed.  Closing bug.
Comment 22 Radar WebKit Bug Importer 2018-03-03 00:59:26 PST
<rdar://problem/38097208>