Closed Bug 1317901 Opened 8 years ago Closed 8 years ago

Cloned images from <template> do not load.

Categories

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

50 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla53
Tracking Status
firefox-esr45 --- unaffected
firefox50 + wontfix
firefox51 --- verified
firefox52 --- verified
firefox53 --- verified

People

(Reporter: TailsTheKitsune, Assigned: edgar)

References

Details

(Keywords: dev-doc-complete, regression, site-compat)

Attachments

(4 files, 1 obsolete file)

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.
Attached patch bug_1317901_v1.patch (obsolete) — Splinter Review
Attached 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
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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)
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)
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.

Attachment

General

Creator:
Created:
Updated:
Size: