818643 - Animated background gif stopped working in beta 18

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[j.j.]

Mozilla/5.0 (Windows NT 5.1; rv:18.0) Gecko/20100101 Firefox/18.0

see
http://bedienungsanleitung.elektronotdienst-nuernberg.de/impedanz.html

or testcase from bug 773203
https://bug773203.bugzilla.mozilla.org/attachment.cgi?id=641667

Background animation works in Fx 17 but not in beta 18

Comment 1

[Timothy Nikkel (:tnikkel)]

Is it broken or working on trunk?

Comment 2

[j.j.]

Mozilla/5.0 (Windows NT 5.1; rv:20.0) Gecko/20121205 Firefox/20.0

also broken on latest Nightly

Comment 3

[Alex Keybl [:akeybl]]

WFM on a Mac running Firefox 18. Will need a regression range on Windows to hand this off to the correct engineer.

j.j. - if you have the time to help us speed up testing process, please run http://harthur.github.com/mozregression/. Thanks!

Comment 4

[Daniel Holbert [:dholbert]]

(actually, mozregression now lives here:
  http://mozilla.github.com/mozregression/
though harthur's clone probably still works just as well)

Comment 5

[Daniel Holbert [:dholbert]]

I just ended up doing mozregression, since it's really fast from the MoCo MV office.  (I can reproduce this in my linux nightly -- candles don't blink in comment 0 links)

Regression range:
Last good nightly: 2012-11-16
First bad nightly: 2012-11-17

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2012-11-16&enddate=2012-11-17

That included a number of image-related changes, but the commits for bug 810470 look most relevant to background images in particular.  Tentatively flagging as a regression from that bug, & cc'ing roc & mattwoodrow.

Comment 6

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking this regression for 18 and hoping roc can look into this or assign someone else to investigate.

Comment 7

[Daniel Holbert [:dholbert]]

Confirmed that bug 810470's csets caused this, with targeted builds, using attachment 641667 [details] as a testcase.

A build from https://hg.mozilla.org/mozilla-central/rev/5444aff38dfb animates (no bug).
A build from https://hg.mozilla.org/mozilla-central/rev/e8c599817e97 doesn't animate (buggy).

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Comment 9

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 689541 [details] [diff] [review]
fix

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

::: layout/base/nsDisplayList.cpp
@@ +2100,2 @@
>    }
>  }

I think this could still fail if the nsDisplayBackground changed layers and changed size during the same paint. Or alternatively we stop creating the display item, change the size, and add it again.

Both of those would change rendering without calling ComputeInvalidationRegion().

We might need a specific 'something changed' callback to call from InvalidateForLayerChange (which isn't a great name anymore now that it does a lot more than that).

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is difficult to test properly, unfortunately.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #9)
> I think this could still fail if the nsDisplayBackground changed layers and
> changed size during the same paint. Or alternatively we stop creating the
> display item, change the size, and add it again.
> 
> Both of those would change rendering without calling
> ComputeInvalidationRegion().
> 
> We might need a specific 'something changed' callback to call from
> InvalidateForLayerChange (which isn't a great name anymore now that it does
> a lot more than that).

This is tricky.

I think the simplest approach is simply to have nsCanvasFrame::Reflow invalidate the background cache if the frame size changes.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: invalidate background image on canvas size change

I think this covers it. We invalidate on:
a) style changes (via InvalidateFrameInternal)
b) canvasframe size changes
c) image changes
Did I miss anything? :-)

I've taken out the InvalidateCachedBackgroundImage() calls from nsDisplayBackgroundImage::ComputeInvalidationRegion. They should not be needed anymore.

Comment 13

[Matt Woodrow (:mattwoodrow)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #9)
> > I think this could still fail if the nsDisplayBackground changed layers and
> > changed size during the same paint. Or alternatively we stop creating the
> > display item, change the size, and add it again.
> > 
> > Both of those would change rendering without calling
> > ComputeInvalidationRegion().
> > 
> > We might need a specific 'something changed' callback to call from
> > InvalidateForLayerChange (which isn't a great name anymore now that it does
> > a lot more than that).
> 
> This is tricky.

What's tricky?

I think the 'if (!combined.IsEmpty()) branch at the bottom of InvalidateForLayerChange covers all the cases we care about, doesn't it?

we could just add a call to nsDisplayItem::RenderingChanged/InvalidateCachedData/SomeUsefulNameHere from there and it would avoid having to worry about potentially missing a callsite.

The only other case we might care about is if we stop painting the background, and want to destroy the cache to save memory. ProcessRemovedDisplayItems would work, though maybe just having an expiration timer would be better. Possibly work for another bug.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: Issue a NotifyRenderingChanged callback on nsDisplayItems when we invalidate anything to do with the display item

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Combined fix

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It might be easy to write a testcase for this. Just need an image sprite in an active transform.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #17)
> It might be easy to write a testcase for this. Just need an image sprite in
> an active transform.

I put this comment in the wrong bug. This one is quite difficult to test.

Comment 20

[bhavana bajaj [:bajaj]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b30a1fff2d07

:roc, considering the testing we have had on central, can you please nominate branch patches with risk analysis ?

Comment 21

[juan becerra [:juanb]]

I can no longer reproduce this on Nightly according to comment #0 and comment #5. The animation works.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 690266 [details] [diff] [review]
Combined fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 810470
User impact if declined: animated background-attachment:fixed image is not animated
Testing completed: a week on central
Risk to taking this patch (and alternatives if risky): This patch shuffles around some invalidation code. The likely impact of any regression bug would be to fail to invalidate background-attachment:fixed images in a different way.
String or UUID changes made by this patch: None

Comment 23

[Simona Badau, Desktop QA]

This is no longer reproducible on the latest Nightly on Windows XP, Windows 7, Ubuntu 12.10 and Mac OS X 10.7.5:
Mozilla/5.0 (Windows NT 5.1; rv:20.0) Gecko/20121217 Firefox/20.0 Build ID:20121217030850
Mozilla/5.0 (Windows NT 6.1; rv:20.0) Gecko/20121217 Firefox/20.0 Build ID:20121217030850
Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20121217 Firefox/20.0 Build ID:20121217030850
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:20.0) Gecko/20121217 Firefox/20.0	Build Id:20121217030850

Comment 24

[Ryan VanderMeulen [:RyanVM]]

FWIW, anything that lands on beta will be merged to b2g18 as well, so there's no need to request separate approval to land there (nor do you need to land it on both trees individually).

Comment 25

[Alex Keybl [:akeybl]]

Comment on attachment 690266 [details] [diff] [review]
Combined fix

We think that the risk (only reported a week ago, unlikely to get new feedback before release, touches layout code) versus reward (no major sites are impacted, user impact is pretty minor) means that this can wait till FF19 to fix.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/63a5fb585e84