1317901 - Cloned images from <template> do not load.

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[XFox]

Attachment: Cloned Images from Template do not load.htm

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161104212021

Steps to reproduce:

1. Store an <img /> inside <template>
2. Clone the <img /> from the <template>'s content property.
3. Append it somewhere in the document


Actual results:

The image will not load. Its alt text will be displayed instead. Per the DOM Inspector, the src is preserved but shows "Could not load the image" when hovering over it.


Expected results:

The image should be shown. There were no problems in Firefox 49. Bug has shown up in Firefox 50.0 and is present in Nightly.

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=696adef2e21a9fe27f5b4251f00dc0c942a5d4f5&tochange=299a09f24493acfbbe8b9457c36e2eccf318d787

Suspect: c4b39cdfb64	Josh Matthews — Bug 1268182 - Allow image loads to short-circuit after selecting a source if the new source URL matches the previous one URL. r=echen

Comment 2

[Andrew Overholt [:overholt]]

Edgar, can you take a look?

Comment 3

[Edgar Chen [:edgar] (PTO until 02/09)]

The problem is similar to https://bugzilla.mozilla.org/show_bug.cgi?id=1307185#c4, the image element is created in document fragment and then adopted into active document. But UnbindFromTree() doesn't be triggered to clear mLastSelectedSource since the element has no parent.

And I see another problem, we try to start the load of an image which is in a document fragment. This seem not follow the spec, see step 1 of https://html.spec.whatwg.org/multipage/embedded-content.html#updating-the-image-data. Filed bug 1318624.

Comment 4

[Edgar Chen [:edgar] (PTO until 02/09)]

Attachment: bug_1317901_v1.patch

Comment 5

[Edgar Chen [:edgar] (PTO until 02/09)]

Attachment: Patch, v2

Fix type.

Comment 6

[Edgar Chen [:edgar] (PTO until 02/09)]

(In reply to Edgar Chen [:edgar][:echen] from comment #5)
> Created attachment 8812134 [details] [diff] [review]
> Patch, v2
> 
> Fix type.

s/type/typo

Comment 7

[Edgar Chen [:edgar] (PTO until 02/09)]

Comment on attachment 8812134 [details] [diff] [review]
Patch, v2

Hi Josh, could you help to review this patch? Thank you.

Comment 8

[Edgar Chen [:edgar] (PTO until 02/09)]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4de19530e61d09d8f94a8b09b7adff118502e245&group_state=expanded&filter-tier=1

Comment 9

[Josh Matthews [:jdm]]

Comment on attachment 8812134 [details] [diff] [review]
Patch, v2

Review of attachment 8812134 [details] [diff] [review]:
-----------------------------------------------------------------

If the test fails without the code change, this looks fine to me.

::: dom/html/reftests/image-load-shortcircuit-2.html
@@ +3,5 @@
> +<template id="template">
> +<img src="pass.png" alt="Alt Text" />
> +</template>
> +<script>
> +  document.body.appendChild(document.getElementById('template').content.children[0].cloneNode(1));

Are we sure that this test does not pass with these changes? I'm surprised that a cloned node has this problem.

Comment 10

[Edgar Chen [:edgar] (PTO until 02/09)]

(In reply to Josh Matthews [:jdm] from comment #9)
> Comment on attachment 8812134 [details] [diff] [review]
> Patch, v2
> 
> Review of attachment 8812134 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If the test fails without the code change, this looks fine to me.
> 
> ::: dom/html/reftests/image-load-shortcircuit-2.html
> @@ +3,5 @@
> > +<template id="template">
> > +<img src="pass.png" alt="Alt Text" />
> > +</template>
> > +<script>
> > +  document.body.appendChild(document.getElementById('template').content.children[0].cloneNode(1));
> 
> Are we sure that this test does not pass with these changes? I'm surprised
> that a cloned node has this problem.

Yes, I have verified it. The test doesn't pass without the code change.

The problem is,
The cloned image starts a load even if it's document is not active one [1] and fail to load, but store URL as last selected source. And since it has no parent, appending/adopting into active document won't trigger UnbindFromTree() to clear mLastSelectedSource. Then we try to start the load again and short-circuit because the URL hasn't changed.

[1] http://searchfox.org/mozilla-central/source/dom/html/HTMLImageElement.cpp#829

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Ritu, this may be a candidate for the 50.1.0 release. Loic suggested it in bug 1268182 since it affects some corporate sites. This may also fix bug 1318889 and the other dependencies of bug 1268182.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Once you all have landed a possible fix let's verify it and uplift at least to beta 51.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab60afa100d3
Ensure image loads don't short-circuit if element's adopting steps are run; r=jdm

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ab60afa100d3

Comment 16

[Edgar Chen [:edgar] (PTO until 02/09)]

Attachment: [beta] Patch, v2, r=jdm

Comment 17

[Edgar Chen [:edgar] (PTO until 02/09)]

Comment on attachment 8813492 [details] [diff] [review]
[beta] Patch, v2, r=jdm

Approval Request Comment
[Feature/regressing bug #]:
bug 1268182

[User impact if declined]:
Lots of images on some corporate sites stop loading. 

[Describe test coverage new/current, TreeHerder]:
New test added verifying this case.

[Risks and why]:
At worst we reload an image unnecessarily.

[String/UUID change made/needed]:
None.

Comment 18

[Edgar Chen [:edgar] (PTO until 02/09)]

Comment on attachment 8813492 [details] [diff] [review]
[beta] Patch, v2, r=jdm

Approval Request Comment
[Feature/regressing bug #]:
bug 1268182.

[User impact if declined]:
Lots of images on some corporate sites stop loading. 

[Describe test coverage new/current, TreeHerder]:
New test added verifying this case.

[Risks and why]:
At worst we reload an image unnecessarily.

[String/UUID change made/needed]:
None.

Comment 19

[Gerry Chang [:gchang]]

Hi Andrei,
Can someone from QE help verify the fix in latest nightly?

Comment 21

[Gerry Chang [:gchang]]

Hi Brindusa,
Can you help verify if the patch fix bug 1318889, bug 1319046, and this bug on latest nightly?

Comment 22

[uwe.hausbrand]

Hi,

I'm the one who created https://bugzilla.mozilla.org/show_bug.cgi?id=1319046 and I've tried the latest nightly build for WIN 10 (firefox 53) and can verify that the bug is fixed.

Comment 23

[XFox]

Creator of this bug here, confirming fixed.

Comment 24

[Julien Cristau [:jcristau]]

Comment on attachment 8813492 [details] [diff] [review]
[beta] Patch, v2, r=jdm

fix loading cloned images in template, take in aurora52

Comment 25

[Gerry Chang [:gchang]]

Comment on attachment 8813492 [details] [diff] [review]
[beta] Patch, v2, r=jdm

Fix a regression and was verified. Beta51+. Should be in 51 beta 3.

Comment 26

[ovidiu boca[:Ovidiu]]

Hi,

I tested this bug and the bugs from comment 21 with FF Nightly 53.0a1 on Mac OS X 10.12, Windows 10 x64 and Ubuntu 16.04 x64 and here are the results: 

bug 1319046 can't be verified because the test case from the bug doesn't work on my end but I see in comment 22 that the reporter of that bug confirmed the fix.

I can confirm the fix on bug 1217901 and bug 1318889, everything works as expected.

Comment 27

[Carsten Book [:Tomcat]]

failed to apply to aurora

named 1317901 -> bug_1317901_aurora_v2.patch
applying bug_1317901_aurora_v2.patch
patching file dom/html/HTMLImageElement.h
Hunk #1 FAILED at 87
1 out of 1 hunks FAILED -- saving rejects to file dom/html/HTMLImageElement.h.rej
patching file dom/html/reftests/reftest.list
Hunk #1 succeeded at 32 with fuzz 2 (offset 0 lines).
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug_1317901_aurora_v2.patch

Comment 28

[Edgar Chen [:edgar] (PTO until 02/09)]

Comment on attachment 8813492 [details] [diff] [review]
[beta] Patch, v2, r=jdm

This patch can be applied to beta.

Comment 29

[Edgar Chen [:edgar] (PTO until 02/09)]

Attachment: [aurora] Patch, v2, r=jdm

Patch for aurora.

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b488c41c4a24

Comment 31

[Carsten Book [:Tomcat]]

thanks edgar, landed this in https://hg.mozilla.org/releases/mozilla-beta/rev/a2ccc1a66d96a413213b2433d3348c4a6884038e

Comment 32

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello XFox, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 33

[XFox]

(In reply to Ritu Kothari (:ritu) from comment #32)
> Hello XFox, could you please verify this issue is fixed as expected on a
> latest Nightly build? Thanks!

Confirmed working in today's Nightly.

Comment 34

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/img-in-template-will-not-be-loaded-when-added-to-document/

Comment 36

[Paul Silaghi, QA [:pauly]]

Verified fixed FX 52.0a2 (2016-12-19), 51b9 on Win 7.