Bug 153027 - Don't download the wrong image for picture elements
Summary: Don't download the wrong image for picture elements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dave Hyatt
URL:
Keywords: InRadar
Depends on: 153048 153118
Blocks:
  Show dependency treegraph
 
Reported: 2016-01-12 12:02 PST by Dave Hyatt
Modified: 2016-01-15 09:07 PST (History)
11 users (show)

See Also:


Attachments
Patch (7.14 KB, patch)
2016-01-12 12:06 PST, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch (7.10 KB, patch)
2016-01-12 12:09 PST, Dave Hyatt
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-yosemite (1009.39 KB, application/zip)
2016-01-12 12:41 PST, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-yosemite (976.79 KB, application/zip)
2016-01-12 12:47 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (889.10 KB, application/zip)
2016-01-12 13:04 PST, Build Bot
no flags Details
Patch (17.56 KB, patch)
2016-01-12 13:09 PST, Dave Hyatt
dino: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-yosemite (876.34 KB, application/zip)
2016-01-12 13:57 PST, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-yosemite (890.43 KB, application/zip)
2016-01-12 14:04 PST, Build Bot
no flags Details
Patch (19.89 KB, patch)
2016-01-14 07:45 PST, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch (19.89 KB, patch)
2016-01-14 07:50 PST, Dave Hyatt
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (823.18 KB, application/zip)
2016-01-14 08:51 PST, Build Bot
no flags Details
Patch (21.05 KB, patch)
2016-01-14 09:05 PST, Dave Hyatt
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Hyatt 2016-01-12 12:02:32 PST
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.
Comment 1 Dave Hyatt 2016-01-12 12:06:18 PST
Created attachment 268796 [details]
Patch
Comment 2 WebKit Commit Bot 2016-01-12 12:07:46 PST
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.
Comment 3 Dave Hyatt 2016-01-12 12:09:53 PST
Created attachment 268797 [details]
Patch
Comment 4 Build Bot 2016-01-12 12:40:59 PST
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
Comment 5 Build Bot 2016-01-12 12:41:04 PST
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 6 Build Bot 2016-01-12 12:47:52 PST
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
Comment 7 Build Bot 2016-01-12 12:47:55 PST
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 8 Build Bot 2016-01-12 13:04:02 PST
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
Comment 9 Build Bot 2016-01-12 13:04:05 PST
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
Comment 10 Dave Hyatt 2016-01-12 13:09:19 PST
Created attachment 268804 [details]
Patch
Comment 11 Dean Jackson 2016-01-12 13:18:22 PST
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 12 Jon Lee 2016-01-12 13:27:18 PST
rdar://problem/24142331
Comment 13 Build Bot 2016-01-12 13:57:34 PST
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
Comment 14 Build Bot 2016-01-12 13:57:38 PST
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 15 Build Bot 2016-01-12 14:04:13 PST
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
Comment 16 Build Bot 2016-01-12 14:04:17 PST
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
Comment 17 Dave Hyatt 2016-01-12 14:19:59 PST
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.
Comment 18 Dave Hyatt 2016-01-12 14:31:03 PST
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.
Comment 19 WebKit Commit Bot 2016-01-12 16:27:34 PST
Re-opened since this is blocked by bug 153048
Comment 20 Ryan Haddad 2016-01-12 16:37:05 PST
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>
Comment 21 Dave Hyatt 2016-01-14 07:45:40 PST
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.
Comment 22 WebKit Commit Bot 2016-01-14 07:48:09 PST
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.
Comment 23 Dave Hyatt 2016-01-14 07:50:07 PST
Created attachment 268966 [details]
Patch
Comment 24 Build Bot 2016-01-14 08:51:20 PST
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
Comment 25 Build Bot 2016-01-14 08:51:24 PST
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
Comment 26 Dave Hyatt 2016-01-14 09:05:51 PST
Created attachment 268974 [details]
Patch
Comment 27 Dave Hyatt 2016-01-14 11:13:54 PST
Fixed in r195064.
Comment 28 WebKit Commit Bot 2016-01-14 23:01:10 PST
Re-opened since this is blocked by bug 153118
Comment 29 Alex Christensen 2016-01-14 23:06:24 PST
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
Comment 30 Dave Hyatt 2016-01-15 09:07:22 PST
Relanded in r195132 with no test.