1098958 - Images cut off

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Alice0775 White]

[Tracking Requested - why for this release]: regressed in Aurora35.0a2 and Nightly36.0a1

Steps To reproduce:
1. Clear cache
2. Put mouse pointer to the bottom of the browser screen
3. Open http://ie.microsoft.com/testdrive/Performance/FlyingImages/Default.html
   --- observe images
4. Change Image size (Default IE Logo, Large IE Logo，Mix Photo, My Browser, All Browsers)
   --- observe images

Actual Results:
Images cut off at bottom

Expected Results:
Image should draw all its height

Regression window(m-c)
Good:
https://hg.mozilla.org/mozilla-central/rev/43ab820c7b68
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 ID:20140926183438
Bad:
https://hg.mozilla.org/mozilla-central/rev/818f353b7aa6
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 ID:20140926185339
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=43ab820c7b68&tochange=818f353b7aa6

Regressed by:
818f353b7aa6	Seth Fowler — Bug 1069369 - Remove all manual discarding during frame lookup. r=tn a=kwierso


Note:
there is other Bug 1098895 in Aurora35.0a2.

Comment 1

[Sylvestre Ledru [:Sylvestre]]

I can confirm the intermittent issue on 36 (GNU/Linux).

Comment 2

[Alice0775 White]

[Tracking Requested - why for this release]:

Comment 3

[Seth Fowler [:seth] [:s2h]]

Attachment: Call UpdateImageContainer whenever we send invalidations in RasterImage

Since debugging showed that the images involved here are getting decoded, notified about, etc. successfully, my next theory was that the layers::ImageContainer was falling out of sync. This proved to be correct, and the fix is extremely straightforward.

I do feel like we're starting to get too much logic about invalidations in FinishedSomeDecoding; a future refactoring should try to clean this stuff up a little. At least it's the *only* place in RasterImage we have logic about invalidations due to decoding now.

Comment 4

[Seth Fowler [:seth] [:s2h]]

Before that patch, I could reproduce the problem 75% of the time on a shift-reload. After the patch, I cannot reproduce it at all. That said, this bug is timing dependent, so I think we should ask QA for a verification of the fix.

This also needs an uplift to Aurora. (And a try job, but try is closed right now.)

Comment 5

[Seth Fowler [:seth] [:s2h]]

Attachment: Call UpdateImageContainer whenever we send invalidations in RasterImage

Thanks for the review! I made a minor tweak; now we only call UpdateImageContainer if the default decode flags are used. Doing it otherwise is pointless, since we use the default decode flags version of the image in UpdateImageContainer.

Comment 6

[Seth Fowler [:seth] [:s2h]]

Here's a try job:

https://tbpl.mozilla.org/?tree=Try&rev=bdbe1d8d2310

Comment 7

[Seth Fowler [:seth] [:s2h]]

Try looks good. Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5fcba591258

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 9

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8534720 [details] [diff] [review]
Call UpdateImageContainer whenever we send invalidations in RasterImage

This needs an uplift to Aurora. I am very sorry to say that because the dependencies here are *massive*, we cannot uplift to 35, even though it's affected. =( I'll look into the possibility of making a different version of the patch that could land on 35.

Approval Request Comment
[Feature/regressing bug #]: 1069369
[User impact if declined]: Sometimes part of an image will be cut off.
[Describe test coverage new/current, TBPL]: On m-c. Current tests should cover this; it's not clear to me right now why we're not getting intermittent oranges from this issue. (OTOH, there are several unexplained intermittent orange bugs in ImageLib right now, so maybe we are...)
[Risks and why]: Low risk. We just update the version of the image displayed onscreen more often.
[String/UUID change made/needed]: None.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1a0667f25ad2

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

wontfix for 35 based on comment 9