Cloned images from <template> do not load.

VERIFIED FIXED in Firefox 51

Status

()

defect
P1
normal
VERIFIED FIXED
3 years ago
5 months ago

People

(Reporter: TailsTheKitsune, Assigned: edgar)

Tracking

({dev-doc-complete, regression, site-compat})

50 Branch
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox50+ wontfix, firefox51 verified, firefox52 verified, firefox53 verified)

Details

Attachments

(4 attachments, 1 obsolete attachment)

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.
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
Blocks: 1268182
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
Flags: needinfo?(josh)
Edgar, can you take a look?
Flags: needinfo?(echen)
Priority: -- → P1
Assignee: nobody → echen
Flags: needinfo?(echen)
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.
Posted patch bug_1317901_v1.patch (obsolete) — Splinter Review
Posted patch Patch, v2Splinter Review
Fix type.
Attachment #8812132 - Attachment is obsolete: true
(In reply to Edgar Chen [:edgar][:echen] from comment #5)
> Created attachment 8812134 [details] [diff] [review]
> Patch, v2
> 
> Fix type.

s/type/typo
Comment on attachment 8812134 [details] [diff] [review]
Patch, v2

Hi Josh, could you help to review this patch? Thank you.
Flags: needinfo?(josh)
Attachment #8812134 - Flags: review?(josh)
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.
Attachment #8812134 - Flags: review?(josh) → review+
(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
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.
Flags: needinfo?(rkothari)
Once you all have landed a possible fix let's verify it and uplift at least to beta 51.
Flags: needinfo?(echen)
Pushed by echen@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/ab60afa100d3
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Duplicate of this bug: 1318889
Flags: needinfo?(echen)
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.
Attachment #8813492 - Flags: approval-mozilla-aurora?
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.
Attachment #8813492 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Hi Andrei,
Can someone from QE help verify the fix in latest nightly?
Flags: needinfo?(andrei.vaida)
Duplicate of this bug: 1319046
Hi Brindusa,
Can you help verify if the patch fix bug 1318889, bug 1319046, and this bug on latest nightly?
Flags: needinfo?(andrei.vaida) → needinfo?(brindusa.tot)
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.
Creator of this bug here, confirming fixed.
Comment on attachment 8813492 [details] [diff] [review]
[beta] Patch, v2, r=jdm

fix loading cloned images in template, take in aurora52
Attachment #8813492 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #8813492 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(brindusa.tot)
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.
Status: RESOLVED → VERIFIED
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
Flags: needinfo?(echen)
Comment on attachment 8813492 [details] [diff] [review]
[beta] Patch, v2, r=jdm

This patch can be applied to beta.
Attachment #8813492 - Attachment description: [aurora/beta] Patch, v2, r=jdm → [beta] Patch, v2, r=jdm
Attachment #8813492 - Attachment filename: bug_1317901_aurora_v2.patch → bug_1317901_beta_v2.patch
Patch for aurora.
Flags: needinfo?(echen) → needinfo?(cbook)
Hello XFox, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(rkothari) → needinfo?(TailsTheKitsune)
(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.
Flags: needinfo?(TailsTheKitsune)
Duplicate of this bug: 1323126
Verified fixed FX 52.0a2 (2016-12-19), 51b9 on Win 7.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.