Bug 179231

Summary:

Preloader ignores type attribute on Source tag causing overdownloading

Product:

WebKit

Reporter:

Colin Bendell <colin>

Component:

Images

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

achristensen, buildbot, cdumez, commit-queue, dino, esprehn+autocc, gyuyoung.kim, rniwa, ryanhaddad, webkit-bug-importer, yoav

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

179545, 179632

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Archive of layout-test-results from ews126 for ios-simulator-wk2	none
Patch	none
Archive of layout-test-results from ews124 for ios-simulator-wk2	none
Patch	none
Patch	none
Archive of layout-test-results from ews101 for mac-elcapitan	none
Archive of layout-test-results from ews114 for mac-elcapitan	none
Patch	none
Archive of layout-test-results from ews101 for mac-elcapitan	none
Patch	none
Archive of layout-test-results from ews126 for ios-simulator-wk2	none
Patch	none
Archive of layout-test-results from ews107 for mac-elcapitan-wk2	none
Patch	none
Patch	none
Patch	none
Patch	none

Colin Bendell

Reported 2017-11-02 22:27:12 PDT

The `type` attribute in <source> tags are ignored by the preloader. A common pattern is to use the <source> tag for content negotiation selection to specify webp for chrome and jp2 for safari. For example: <picture> <source type="image/webp" srcset="foo.webp"> <source type="image/jp2" srcset="foo.jp2"> <img src="foo.jpg"> </picture> However, the HTMLPreloadScanner only considers the media query when selecting (or not) the appropriate <source> element. As a result the preloader greedily selects the foo.webp and then later requests the correct foo.jpg. It should also evaluate the `type` attribute. A more complete example below that demonstrates this bug. Here we use the Souder's cuzillion service to create a slow, blocking js to ensure the preloader is used: <html> <head> <script src="http://1.cuzillion.com/bin/resource.cgi?type=js&sleep=1&expires=-1"></script> </head> <body> <picture> <source type="image/bad" srcset="https://webkit.org/wp-content/themes/webkit/images/squirrelfish-lives.svg"/> <img src="https://webkit.org/wp-content/themes/webkit/images/webkit.svg"> </picture> </body> </html> In this example you will see both the squirrelfish-lives.svg and the webkit.svg file are retrieved. But only the webkit.svg is displayed. Chrome previously had this same issue and it was addressed in Chrome 52 (See Blink bug 615473)

Attachments
Patch (13.07 KB, patch) 2017-11-03 06:53 PDT, Colin Bendell	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (1.02 MB, application/zip) 2017-11-03 08:14 PDT, Build Bot	no flags	Details
Patch (13.09 KB, patch) 2017-11-03 11:23 PDT, Colin Bendell	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (1.13 MB, application/zip) 2017-11-03 12:45 PDT, Build Bot	no flags	Details
Patch (14.23 KB, patch) 2017-11-05 11:07 PST, Colin Bendell	no flags	Details Formatted Diff Diff
Patch (14.24 KB, patch) 2017-11-05 11:15 PST, Colin Bendell	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews101 for mac-elcapitan (1.32 MB, application/zip) 2017-11-05 12:15 PST, Build Bot	no flags	Details
Archive of layout-test-results from ews114 for mac-elcapitan (1.83 MB, application/zip) 2017-11-05 16:01 PST, Build Bot	no flags	Details
Patch (14.31 KB, patch) 2017-11-05 19:50 PST, Colin Bendell	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews101 for mac-elcapitan (1011.04 KB, application/zip) 2017-11-05 20:46 PST, Build Bot	no flags	Details
Patch (14.34 KB, patch) 2017-11-05 21:03 PST, Colin Bendell	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (1.00 MB, application/zip) 2017-11-05 22:24 PST, Build Bot	no flags	Details
Patch (14.34 KB, patch) 2017-11-06 05:14 PST, Colin Bendell	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (1.28 MB, application/zip) 2017-11-06 06:19 PST, Build Bot	no flags	Details
Patch (13.89 KB, patch) 2017-11-06 06:48 PST, Colin Bendell	no flags	Details Formatted Diff Diff
Patch (13.95 KB, patch) 2017-11-08 07:31 PST, Colin Bendell	no flags	Details Formatted Diff Diff
Patch (12.99 KB, patch) 2017-11-08 20:43 PST, Colin Bendell	no flags	Details Formatted Diff Diff
Patch (13.34 KB, patch) 2017-11-15 18:25 PST, Colin Bendell	no flags	Details Formatted Diff Diff
Show Obsolete (10) View All Add attachment proposed patch, testcase, etc.

Colin Bendell

Comment 1 2017-11-03 06:53:10 PDT

Created attachment 325890 [details] Patch

Build Bot

Comment 2 2017-11-03 08:14:51 PDT

Comment on attachment 325890 [details] Patch Attachment 325890 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5090235 New failing tests: http/tests/loading/preload-picture-type.html

Build Bot

Comment 3 2017-11-03 08:14:52 PDT

Created attachment 325895 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6

Colin Bendell

Comment 4 2017-11-03 11:23:43 PDT

Created attachment 325920 [details] Patch

Build Bot

Comment 5 2017-11-03 12:45:39 PDT

Comment on attachment 325920 [details] Patch Attachment 325920 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5093334 New failing tests: http/tests/loading/preload-picture-type.html

Build Bot

Comment 6 2017-11-03 12:45:40 PDT

Created attachment 325938 [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

Yoav Weiss

Comment 7 2017-11-03 13:36:47 PDT

Comment on attachment 325890 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=325890&action=review > Source/WebCore/ChangeLog:3 > + Add mime type check to the picture source preloader to avoid greedy downloading. Could you add a better description here? > Source/WebCore/ChangeLog:11 > + (WebCore::TokenPreloadScanner::StartTagScanner::processAttributes): Could you add a description to the functions? > Source/WebCore/html/parser/HTMLPreloadScanner.cpp:224 > + LOG(MediaQueries, "HTMLPreloadScanner %p processAttribute evaluating content type", this); I don't think the LOG here is necessary

Alex Christensen

Comment 8 2017-11-03 14:44:49 PDT

Alex Christensen

Comment 9 2017-11-03 14:59:29 PDT

Dean, why is iOS choosing different images from the srcset?

Colin Bendell

Comment 10 2017-11-04 06:49:28 PDT

I believe the issue is the DPR is different in the ios-sim and so the selection is different. Yoav pointed me to use srcset-helper.js which will adjust targetScaleFactor = 1. It looks like the preload-picture-srcset-sizes.html test also suffers from this failure - but it's been marked to Skip for iOS (instead of using the srcset-helper.js). The last patch had the wrong relative reference to the helper. fixing now.

Colin Bendell

Comment 11 2017-11-05 11:07:15 PST

Created attachment 326068 [details] Patch

Build Bot

Comment 12 2017-11-05 11:08:32 PST

Attachment 326068 [details] did not pass style-queue: ERROR: LayoutTests/ChangeLog:3: Line contains tab character. [whitespace/tab] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Colin Bendell

Comment 13 2017-11-05 11:15:28 PST

Created attachment 326069 [details] Patch

Build Bot

Comment 14 2017-11-05 12:15:28 PST

Comment on attachment 326069 [details] Patch Attachment 326069 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5115033 New failing tests: http/tests/loading/preload-picture-type.html

Build Bot

Comment 15 2017-11-05 12:15:29 PST

Created attachment 326073 [details] Archive of layout-test-results from ews101 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Build Bot

Comment 16 2017-11-05 16:01:25 PST

Comment on attachment 326069 [details] Patch Attachment 326069 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/5116553 New failing tests: http/tests/loading/preload-picture-type.html

Build Bot

Comment 17 2017-11-05 16:01:26 PST

Created attachment 326085 [details] Archive of layout-test-results from ews114 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Colin Bendell

Comment 18 2017-11-05 19:50:13 PST

Created attachment 326097 [details] Patch

Build Bot

Comment 19 2017-11-05 20:46:55 PST

Comment on attachment 326097 [details] Patch Attachment 326097 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5118420 New failing tests: http/tests/loading/preload-picture-type.html

Build Bot

Comment 20 2017-11-05 20:46:56 PST

Created attachment 326099 [details] Archive of layout-test-results from ews101 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Colin Bendell

Comment 21 2017-11-05 21:03:19 PST

Created attachment 326100 [details] Patch

Build Bot

Comment 22 2017-11-05 22:24:48 PST

Comment on attachment 326100 [details] Patch Attachment 326100 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5118870 New failing tests: http/tests/workers/service/service-worker-clear.html

Build Bot

Comment 23 2017-11-05 22:24:49 PST

Created attachment 326101 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6

Colin Bendell

Comment 24 2017-11-06 05:14:26 PST

Created attachment 326114 [details] Patch

Build Bot

Comment 25 2017-11-06 06:19:17 PST

Comment on attachment 326114 [details] Patch Attachment 326114 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5121711 New failing tests: webrtc/video-replace-muted-track.html

Build Bot

Comment 26 2017-11-06 06:19:18 PST

Created attachment 326118 [details] Archive of layout-test-results from ews107 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Yoav Weiss

Comment 27 2017-11-06 06:24:55 PST

Comment on attachment 326114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=326114&action=review > Source/WebCore/ChangeLog:6 > + https://bugs.webkit.org/show_bug.cgi?id=179231 Can you modify the above to be formatted like commits? (So a fairly short title line, followed by the bug number, then empty line, followed by "Reviewed by", an empty line and detailed description)

Yoav Weiss

Comment 28 2017-11-06 06:25:43 PST

Other than ChangeLog nits, non-reviewer r+ from my end.

Colin Bendell

Comment 29 2017-11-06 06:48:19 PST

Created attachment 326119 [details] Patch

WebKit Commit Bot

Comment 30 2017-11-06 09:56:39 PST

Comment on attachment 326119 [details] Patch Clearing flags on attachment: 326119 Committed r224498: <https://trac.webkit.org/changeset/224498>

WebKit Commit Bot

Comment 31 2017-11-06 09:56:41 PST

All reviewed patches have been landed. Closing bug.

Chris Dumez

Comment 32 2017-11-07 09:24:24 PST

The newly added test (http/tests/loading/preload-picture-type.html) is flaky, please see: https://bugs.webkit.org/show_bug.cgi?id=179338#c10

Chris Dumez

Comment 33 2017-11-07 09:27:55 PST

(In reply to Chris Dumez from comment #32) > The newly added test (http/tests/loading/preload-picture-type.html) is > flaky, please see: > https://bugs.webkit.org/show_bug.cgi?id=179338#c10 --- /Volumes/Data/EWS/WebKit/WebKitBuild/Debug/layout-test-results/http/tests/loading/preload-picture-type-expected.txt +++ /Volumes/Data/EWS/WebKit/WebKitBuild/Debug/layout-test-results/http/tests/loading/preload-picture-type-actual.txt @@ -2,7 +2,7 @@ main frame - didCommitLoadForFrame main frame - willPerformClientRedirectToURL: http://127.0.0.1:8000/loading/preload-picture-type.html main frame - didFinishDocumentLoadForFrame -main frame - didFinishLoadForFrame +main frame - didFailLoadWithError main frame - didStartProvisionalLoadForFrame main frame - didCancelClientRedirectForFrame main frame - didCommitLoadForFrame

Ryan Haddad

Comment 34 2017-11-07 12:02:53 PST

Reverted r224498 for reason: The LayoutTest for this change is flaky and affecting EWS results. Committed r224541: <https://trac.webkit.org/changeset/224541>

Colin Bendell

Comment 35 2017-11-07 14:09:02 PST

Yes. Seems that the debug logging is racy with the redirect. Refactoring...

Colin Bendell

Comment 36 2017-11-08 07:31:11 PST

Created attachment 326325 [details] Patch

WebKit Commit Bot

Comment 37 2017-11-08 15:22:35 PST

Comment on attachment 326325 [details] Patch Clearing flags on attachment: 326325 Committed r224602: <https://trac.webkit.org/changeset/224602>

WebKit Commit Bot

Comment 38 2017-11-08 15:22:37 PST

All reviewed patches have been landed. Closing bug.

Ryan Haddad

Comment 39 2017-11-08 17:41:35 PST

The LayoutTest still appears to be flaky, but with a new diff: https://build.webkit.org/results/Apple%20Sierra%20Release%20WK1%20(Tests)/r224602%20(5966)/results.html https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fpreload%2Fpicture-type.html

Colin Bendell

Comment 40 2017-11-08 20:41:04 PST

the need to reload the page in order for testRunner.setBackingScaleFactor to work is too racy for preloader tests. Need to simplify the srcset evaluation in the test.

Colin Bendell

Comment 41 2017-11-08 20:43:12 PST

Created attachment 326426 [details] Patch

Colin Bendell

Comment 42 2017-11-09 08:56:12 PST

@Ryan - should we rollback this patch or should I open up a new bug to 'refine' the test?

Ryan Haddad

Comment 43 2017-11-09 09:02:36 PST

(In reply to Colin Bendell from comment #42) > @Ryan - should we rollback this patch or should I open up a new bug to > 'refine' the test? If the follow up fix can be reviewed and landed soon, I think it is fine to leave it in. If that isn't the case, we should roll out so that the test doesn't keep making the bots red.

Colin Bendell

Comment 44 2017-11-09 09:23:59 PST

ok. patch to be applied in bug 179488

WebKit Commit Bot

Comment 45 2017-11-10 14:01:46 PST

Re-opened since this is blocked by bug 179545

Ryan Haddad

Comment 46 2017-11-10 14:05:02 PST

*** Bug 179488 has been marked as a duplicate of this bug. ***

Alex Christensen

Comment 47 2017-11-13 11:33:43 PST

http://trac.webkit.org/r224763

Colin Bendell

Comment 48 2017-11-13 12:21:10 PST

@Alex - I'm confused. the changeset that was just applied is the same one that was previously rejected because of picture-type.html failures. Was the root cause of the preloader not executing fixed? or was this changeset applied in error?

Alex Christensen

Comment 49 2017-11-13 13:38:59 PST

I ran the test 50x locally on Mac and iOS, and the test passed. I thought you had uploaded a fixed patch, so I landed it. Now I see builds like https://build.webkit.org/results/Apple%20iOS%2011%20Simulator%20Debug%20WK2%20(Tests)/r224763%20(1010)/results.html failing, so I think I'm going to revert this again.

WebKit Commit Bot

Comment 50 2017-11-13 13:39:54 PST

Re-opened since this is blocked by bug 179632

Colin Bendell

Comment 51 2017-11-15 18:25:03 PST

Created attachment 327041 [details] Patch

Colin Bendell

Comment 52 2017-11-15 19:52:25 PST

The root cause of the previous test instability was caused by parallel tests also requesting the /slow-script.pl?delay=100 with the same url. This cache collision caused a cache hit which subsequently prevented the preloader to fire and the test failed. Busting the cache with a unique signature has resolved this.

WebKit Commit Bot

Comment 53 2017-11-16 11:13:46 PST

Comment on attachment 327041 [details] Patch Clearing flags on attachment: 327041 Committed r224928: <https://trac.webkit.org/changeset/224928>

WebKit Commit Bot

Comment 54 2017-11-16 11:13:48 PST

All reviewed patches have been landed. Closing bug.

Radar WebKit Bug Importer

Comment 55 2017-11-16 11:14:40 PST

<rdar://problem/35593252>

Note You need to log in before you can comment on or make changes to this bug.