Bug 153027 – Don't download the wrong image for picture elements

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Dave Hyatt]

Make sure to connect images to their picture owners at parse time, before they get inserted, to avoid kicking off loads of the wrong images.

[Dave Hyatt]

Created attachment 268796 [details]
Patch

[WebKit Commit Bot]

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.

[Dave Hyatt]

Created attachment 268797 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Dave Hyatt]

Created attachment 268804 [details]
Patch

[Dean Jackson]

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.

[Jon Lee]

rdar://problem/24142331

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Dave Hyatt]

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.

[Dave Hyatt]

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.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 153048

[Ryan Haddad]

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>

[Dave Hyatt]

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.

[WebKit Commit Bot]

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.

[Dave Hyatt]

Created attachment 268966 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Dave Hyatt]

Created attachment 268974 [details]
Patch

[Dave Hyatt]

Fixed in r195064.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 153118

[Alex Christensen]

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

[Dave Hyatt]

Relanded in r195132 with no test.