Summary: | Don't download the wrong image for picture elements | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dave Hyatt <hyatt> | ||||||||||||||||||||||||||
Component: | Images | Assignee: | Dave Hyatt <hyatt> | ||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
Severity: | Normal | CC: | achristensen, ap, buildbot, cdumez, commit-queue, esprehn+autocc, gyuyoung.kim, jonlee, rniwa, ryanhaddad, webkit-bug-importer | ||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=153043 | ||||||||||||||||||||||||||||
Bug Depends on: | 153048, 153118 | ||||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||||
Attachments: |
|
Description
Dave Hyatt
2016-01-12 12:02:32 PST
Created attachment 268796 [details]
Patch
Attachment 268796 [details] did not pass style-queue:
ERROR: Source/WebCore/html/HTMLImageElement.h:92: The parameter name "node" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WebCore/html/parser/HTMLConstructionSite.cpp:38: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Source/WebCore/html/parser/HTMLConstructionSite.cpp:649: Multi line control clauses should use braces. [whitespace/braces] [4]
Total errors found: 3 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 268797 [details]
Patch
Comment on attachment 268797 [details] Patch Attachment 268797 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/683540 New failing tests: imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/update-the-source-set.html http/tests/loading/preload-picture-nested.html Created attachment 268801 [details]
Archive of layout-test-results from ews100 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 268797 [details] Patch Attachment 268797 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/683538 New failing tests: imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/update-the-source-set.html http/tests/loading/preload-picture-nested.html Created attachment 268802 [details]
Archive of layout-test-results from ews113 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 268797 [details] Patch Attachment 268797 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/683608 New failing tests: imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/update-the-source-set.html http/tests/loading/preload-picture-nested.html Created attachment 268803 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Created attachment 268804 [details]
Patch
Comment on attachment 268804 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=268804&action=review > Source/WebCore/html/HTMLImageElement.cpp:88 > + setPictureNode(nullptr); I wonder if it would be better to put the removal code inline: if (gPictureOwnerMap) gPictureOwnerMap->remove(this); I guess there might be other places that set the picture node to null. Doesn't matter to me either way. Comment on attachment 268804 [details] Patch Attachment 268804 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/683765 New failing tests: fast/picture/image-picture-loads-1x.html Created attachment 268807 [details]
Archive of layout-test-results from ews100 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 268804 [details] Patch Attachment 268804 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/683754 New failing tests: fast/picture/image-picture-loads-1x.html Created attachment 268808 [details]
Archive of layout-test-results from ews114 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5
Landed in r194926. I was unable to come up with a test for this, so I welcome any ideas/suggestions anyone has regarding how to test reliably. dumpResourceCallbacks was flaky in that it would hit the cache if other picture tests ran before it, and it would miss the cache when run individually. If there was a way to force the cache to be ignored, that would have worked, but I don't know of a way to do that. Thanks to ap and thorton for suggesting a way to get reliability out of the existing test (making sure the query strings don't match any other picture tests). I landed a test for this in r194928. Re-opened since this is blocked by bug 153048 Rolled out due to LayoutTest crashes and the new test failing on mac-wk1. This run shows both the crash as well as the new test failure: <https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK1%20(Tests)/builds/11157> Created attachment 268964 [details]
Patch
I was unable to reproduce the crashes on iOS, so I just made speculative fixes in the method that was crashing. Specifically, this patch now null checks document().documentElement(), and the mapping from HTMLImageElement -> HTMLPictureElement now uses WeakPtr, so that if the HTMLPictureElement goes away first, there won't be any issue.
Attachment 268964 [details] did not pass style-queue:
ERROR: Source/WebCore/html/HTMLImageElement.cpp:170: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 1 in 9 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 268966 [details]
Patch
Comment on attachment 268966 [details] Patch Attachment 268966 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/690705 New failing tests: fast/picture/hidpi-image-picture-loads.html Created attachment 268973 [details]
Archive of layout-test-results from ews102 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 268974 [details]
Patch
Fixed in r195064. Re-opened since this is blocked by bug 153118 I rolled it out again because the test was failing about 2/3 of the time. It looks pretty benign, but please make the test always pass before landing. Failures looked like this: https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20WK1%20(Tests)/builds/2589 Relanded in r195132 with no test. |