Closed Bug 1307185 Opened 3 years ago Closed 3 years ago

Images don't load after comment on Github issue

Categories

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

52 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla52
Tracking Status
firefox50 --- fixed
firefox51 --- fixed
firefox52 + fixed

People

(Reporter: adamopenweb, Assigned: jdm)

References

()

Details

Attachments

(2 files)

Attached image Screenshot in nightly
STR:
1) Load Firefox nightly
2) Navigate to https://github.com/
2) Sign in
3) Find an issue to comment on
4) Comment

Expected Behavior:
All images load after comment is submitted.

Actual Behavior:
New images to timeline do not appear. Your new comment's avatar for example.

Inspector says "Could not load the image."
Tested in OSX.

Range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=696adef2e21a9fe27f5b4251f00dc0c942a5d4f5&tochange=299a09f24493acfbbe8b9457c36e2eccf318d787
Seems like maybe https://bugzilla.mozilla.org/show_bug.cgi?id=1268182?
Flags: needinfo?(josh)
Blocks: 1268182
Yeah, that regression range looks pretty clear to me.
Assignee: nobody → josh
Flags: needinfo?(josh)
Tracking 52+ for this regression.
I see the problem, but the solution is not yet clear to me. The images on github are created in a document fragment as part of the AJAX-based HTML updates; this prevents the image loads from starting, but we happily store the URL that we tried to load as the last selected image source. When the image element is appended the actual github document, we try to start the load again and short-circuit because the URL hasn't changed. The complication comes from the fact that there's no indication that the load failed, since we return from LoadImage with NS_OK in the case where the load is blocked.
We can work around this problem by resetting the last selected source if the element changes trees.
Attachment #8797709 - Flags: review?(echen)
Comment on attachment 8797709 [details] [diff] [review]
Ensure image loads don't short-circuit if the element changed trees since the last load

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

This reminds me bug 1264529. It might also help this issue.
We can take this fix first and I will work on bug 1264529 during this week.
Thank you, Josh.
Attachment #8797709 - Flags: review?(echen) → review+
None of the failures on try look related to this change.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0be8b2713f4
Ensure image loads don't short-circuit if the element changed trees since the last load. r=echen
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d0be8b2713f4
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment on attachment 8797709 [details] [diff] [review]
Ensure image loads don't short-circuit if the element changed trees since the last load

Approval Request Comment
[Feature/regressing bug #]: bug 1268182
[User impact if declined]: Lots of images on github stop loading.
[Describe test coverage new/current, TreeHerder]: New test added verifying this case doesn't regress.
[Risks and why]: At worst we reload an image unnecessarily.
[String/UUID change made/needed]: None.

Don't uplift this unless bug 1268182 is uplifted.
Attachment #8797709 - Flags: approval-mozilla-beta?
Attachment #8797709 - Flags: approval-mozilla-aurora?
Hello Adam, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(astevenson)
Comment on attachment 8797709 [details] [diff] [review]
Ensure image loads don't short-circuit if the element changed trees since the last load

Fixes a recent regression in Fx50, automated tests were updated as well (yay!), Aurora51+, Beta50+
Attachment #8797709 - Flags: approval-mozilla-beta?
Attachment #8797709 - Flags: approval-mozilla-beta+
Attachment #8797709 - Flags: approval-mozilla-aurora?
Attachment #8797709 - Flags: approval-mozilla-aurora+
I can confirm this is fixed in nightly 52.0a1 (2016-10-18). Thanks!
Status: RESOLVED → VERIFIED
Flags: needinfo?(astevenson)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.