1273455 - Flex rerendering causes images to blink (?)

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[tr.yura]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20160502172042

Steps to reproduce:

I cannot reproduce the bug in fiddles, but it can be reproduced on http://kudago.com/new-york/ website.

- So, go to http://kudago.com/new-york/
- Open any dropdown ('more' in menu or city choice)
- Maybe you'll have to open/close dropdown 2 or 3 times.




Actual results:

Images on the page blink.


Expected results:

Images are still.

Comment 1

[tr.yura]

'body' element on the page has 'display: flex' and dropdown opening appends 'div' to the body. So, I think that flex rerendering somehow causes the issue. If you remove 'flex' property from body, everything goes back to normal.

Unfortunately, I could not reproduce the bug in the wild. Probably it requires lots of images/elements on the page to be reproduced?

Thanks for any help.

Comment 2

[tr.yura]

Also reproduced on Ubuntu 14.04 FF 46.0

Comment 3

[tr.yura]

Note also that blinking images are 'div' elements with background images set. 'img' tag seems to render correctly.

Comment 4

[Loic]

Probably similar to bug 1272178.

Rag range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=99a4fb
4ba5c1ee96ac745c6ad11894bf0537f73a&tochange=2e19045ba652ca2a5a5fc0e20d6f95293acf
a32d

Comment 5

[Mike Taylor [:miketaylr]]

Comment #3 makes me think of https://bugzilla.mozilla.org/show_bug.cgi?id=1273546.

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking, recent regression. Seth this has a couple of other possible dupes and looks like it may be your regression.

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

We could still potentially take a fix for this in 47. We are heading into beta 7 later this week.

Comment 9

[Gerry Chang [:gchang]]

Hi Jet,
Do you think you can help to find someone for this issue?

Comment 10

[Jet Villegas (inactive)]

(In reply to Gerry Chang [:gchang] from comment #9)
> Hi Jet,
> Do you think you can help to find someone for this issue?

Eventually :)

Some notes on this bug: 

This bug goes away when I remove the lock decrement in RasterImage::UnlockImage(), indicating a potential refcount error with CSS background images. 

Keeping the needinfo open until I find an assignee.

Comment 11

[Jet Villegas (inactive)]

Attachment: Instead of discarding immediately, let the Image surfaces expire.

Comment 12

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

Comment on attachment 8772260 [details] [diff] [review]
Instead of discarding immediately, let the Image surfaces expire.

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

This will cause an AWSY regression. Discard() is meant to, well, discard the surfaces associated with the image. Rather than make this change, we should understand why an image which is being display on the screen is not locked. This is particularly odd for CSS background images, which are normally locked as long as they're in the document. Probably the style struct involved is being destroyed and recreated.

Comment 13

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

Daniel, is some style struct churn involved in this situation?

Comment 14

[Jet Villegas (inactive)]

Attachment: Let CSS Image surface caches expire, rather than immediate discarding.

Comment 15

[Jet Villegas (inactive)]

Attachment: Let CSS Image surface caches expire, rather than immediate discarding.

Since flags defaults to zero anyway, removed that 2nd optional parameter from the patch.

Comment 16

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

Comment on attachment 8772270 [details] [diff] [review]
Let CSS Image surface caches expire, rather than immediate discarding.

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

Looks good!

We talked about this on IRC, but it's worth stating on the bug as well:

This may cause an AWSY regression. We should accept it, because the alternative is that in situations like this where we are rapidly destroying and creating style structs referencing the same image, we're forcibly throwing away the decoded surfaces and then redecoding them on the next paint. This isn't just a flicker issue; it's bad for performance. If this change regresses AWSY, I think we should just consider the previous AWSY numbers as being enabled only via a bug.

Even with this change, the surfaces will still be discarded if nothing references them within 60 seconds, so in real world usage there should be no issue here.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e868621be2f
Let CSS Image surface caches expire, rather than immediately discarding. r=seth

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8e868621be2f

Comment 19

[David Bolter [:davidb] (NeedInfo me for attention)]

Jet do you think we should uplift this fix to 49?

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

Happy to try uplift if you think it is worth the risk. But I also think it's OK to ship 49 with this and let the fix ride with 50.

Comment 21

[Jet Villegas (inactive)]

I'm now leaning towards letting ship in 50 for more bake time. I'll also tag this [MemShrink] so we can get a review of the concerns in comment 16.

Comment 22

[Eric Rahm [:erahm]]

I'll take a look for any regressions.

Comment 24

[Eric Rahm [:erahm]]

(In reply to Eric Rahm [:erahm] from comment #22)
> I'll take a look for any regressions.

I didn't see any regressions.