| 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: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||
| Bug Depends on: | 179545, 179632 | ||||||||||||||||||||||||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||||||
|
Description
Colin Bendell
2017-11-02 22:27:12 PDT
Created attachment 325890 [details]
Patch
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 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
Created attachment 325920 [details]
Patch
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 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
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 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? Dean, why is iOS choosing different images from the srcset? 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. Created attachment 326068 [details]
Patch
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.
Created attachment 326069 [details]
Patch
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 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
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 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
Created attachment 326097 [details]
Patch
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 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
Created attachment 326100 [details]
Patch
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 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
Created attachment 326114 [details]
Patch
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 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
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) Other than ChangeLog nits, non-reviewer r+ from my end. Created attachment 326119 [details]
Patch
Comment on attachment 326119 [details] Patch Clearing flags on attachment: 326119 Committed r224498: <https://trac.webkit.org/changeset/224498> All reviewed patches have been landed. Closing bug. 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 (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 Reverted r224498 for reason: The LayoutTest for this change is flaky and affecting EWS results. Committed r224541: <https://trac.webkit.org/changeset/224541> Yes. Seems that the debug logging is racy with the redirect. Refactoring... Created attachment 326325 [details]
Patch
Comment on attachment 326325 [details] Patch Clearing flags on attachment: 326325 Committed r224602: <https://trac.webkit.org/changeset/224602> All reviewed patches have been landed. Closing bug. 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 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. Created attachment 326426 [details]
Patch
@Ryan - should we rollback this patch or should I open up a new bug to 'refine' the test? (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. ok. patch to be applied in bug 179488 Re-opened since this is blocked by bug 179545 *** Bug 179488 has been marked as a duplicate of this bug. *** @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? 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. Re-opened since this is blocked by bug 179632 Created attachment 327041 [details]
Patch
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. Comment on attachment 327041 [details] Patch Clearing flags on attachment: 327041 Committed r224928: <https://trac.webkit.org/changeset/224928> All reviewed patches have been landed. Closing bug. |
