WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
Bug 179231
Preloader ignores type attribute on Source tag causing overdownloading
https://bugs.webkit.org/show_bug.cgi?id=179231
Summary
Preloader ignores type attribute on Source tag causing overdownloading
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
Mac has these URLs preloaded:
http://127.0.0.1:8000/js-test-resources/js-test.js
http://127.0.0.1:8000/resources/slow-script.pl?delay=100
http://127.0.0.1:8000/loading/resources/base-image1.png?0
http://127.0.0.1:8000/loading/resources/base-image1.png?1
http://127.0.0.1:8000/loading/resources/base-image1.png?2
http://127.0.0.1:8000/loading/resources/preload-test.jpg?3
http://127.0.0.1:8000/loading/resources/base-image3.png?4
http://127.0.0.1:8000/loading/resources/preload-test.jpg?5
http://127.0.0.1:8000/loading/resources/base-image1.png?6
http://127.0.0.1:8000/loading/resources/preload-test.jpg?7
http://127.0.0.1:8000/loading/resources/base-image1.png?8
http://127.0.0.1:8000/loading/resources/preload-test.jpg?9
http://127.0.0.1:8000/loading/resources/base-image1.png?10
http://127.0.0.1:8000/loading/resources/base-image2.png?11
http://127.0.0.1:8000/loading/resources/base-image1.png?12
http://127.0.0.1:8000/loading/resources/preload-test.jpg?13
iOS has these URLs preloaded: PRELOADED
http://127.0.0.1:8000/js-test-resources/js-test.js
PRELOADED
http://127.0.0.1:8000/resources/slow-script.pl?delay=100
PRELOADED
http://127.0.0.1:8000/loading/resources/base-image1.png?0
PRELOADED
http://127.0.0.1:8000/loading/resources/base-image1.png?1
PRELOADED
http://127.0.0.1:8000/loading/resources/base-image1.png?2
PRELOADED
http://127.0.0.1:8000/loading/resources/preload-test.jpg?3
PRELOADED
http://127.0.0.1:8000/loading/resources/base-image2.png?4
PRELOADED
http://127.0.0.1:8000/loading/resources/preload-test.jpg?5
PRELOADED
http://127.0.0.1:8000/loading/resources/base-image1.png?6
PRELOADED
http://127.0.0.1:8000/loading/resources/preload-test.jpg?7
PRELOADED
http://127.0.0.1:8000/loading/resources/base-image1.png?8
PRELOADED
http://127.0.0.1:8000/loading/resources/preload-test.jpg?9
PRELOADED
http://127.0.0.1:8000/loading/resources/base-image1.png?10
PRELOADED
http://127.0.0.1:8000/loading/resources/base-image2.png?11
PRELOADED
http://127.0.0.1:8000/loading/resources/base-image1.png?12
PRELOADED
http://127.0.0.1:8000/loading/resources/preload-test.jpg?13
Notice Mac has base-image3.png?4 and iOS has base-image2.png?4. I think iOS is just choosing a different resolution picture for that one, and this is expected, right?
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.
Top of Page
Format For Printing
XML
Clone This Bug