Closed Bug 1273455 Opened 4 years ago Closed 3 years ago

Flex rerendering causes images to blink (?)

Categories

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

45 Branch
All
Unspecified
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox46 --- wontfix
firefox47 + wontfix
firefox48 + wontfix
firefox49 + wontfix
firefox50 + fixed

People

(Reporter: tr.yura, Assigned: bugs)

References

Details

(Keywords: regression, Whiteboard: [MemShrink])

Attachments

(1 file, 2 obsolete files)

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.
'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.
Also reproduced on Ubuntu 14.04 FF 46.0
Note also that blinking images are 'div' elements with background images set. 'img' tag seems to render correctly.
Hardware: Unspecified → All
Probably similar to bug 1272178.

Rag range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=99a4fb
4ba5c1ee96ac745c6ad11894bf0537f73a&tochange=2e19045ba652ca2a5a5fc0e20d6f95293acf
a32d
Blocks: 1207355
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout: Images
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
Version: 46 Branch → 45 Branch
Duplicate of this bug: 1272178
Tracking, recent regression. Seth this has a couple of other possible dupes and looks like it may be your regression.
We could still potentially take a fix for this in 47. We are heading into beta 7 later this week.
Flags: needinfo?(seth) → needinfo?(bugs)
Hi Jet,
Do you think you can help to find someone for this issue?
(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 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.
Attachment #8772260 - Flags: feedback?(seth) → feedback-
Daniel, is some style struct churn involved in this situation?
Flags: needinfo?(dholbert)
Assignee: nobody → bugs
Attachment #8772260 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Flags: needinfo?(bugs)
Attachment #8772268 - Flags: review?(seth)
Since flags defaults to zero anyway, removed that 2nd optional parameter from the patch.
Attachment #8772268 - Attachment is obsolete: true
Attachment #8772268 - Flags: review?(seth)
Attachment #8772270 - Flags: review?(seth)
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.
Attachment #8772270 - Flags: review?(seth) → review+
Flags: needinfo?(dholbert)
Pushed by jvillegas@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e868621be2f
Let CSS Image surface caches expire, rather than immediately discarding. r=seth
https://hg.mozilla.org/mozilla-central/rev/8e868621be2f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Jet do you think we should uplift this fix to 49?
Flags: needinfo?(bugs)
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.
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.
Flags: needinfo?(bugs)
Whiteboard: [MemShrink]
I'll take a look for any regressions.
Flags: needinfo?(erahm)
No longer blocks: 1234276
Duplicate of this bug: 1234276
(In reply to Eric Rahm [:erahm] from comment #22)
> I'll take a look for any regressions.

I didn't see any regressions.
Flags: needinfo?(erahm)
Product: Core → Core Graveyard
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.