1137058 - image.mem.discardable broken in firefox 36

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[her34her34]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20150122214805

Steps to reproduce:

about:config set
image.mem.discardable;false
image.mem.decodeondraw;false

with this setting, firefox 35 and earlier do not have image flashing when switching between fully loaded tabs. after upgrading to firefox 36, images flash/redraw when switching tabs





Actual results:

switching between fully loaded tabs shows image flash, images pop in late. image place holders are visible before images fill place holder.


Expected results:

images should show instantly without delay

Comment 1

[MarkRH]

Is this similar to this bug I submitted about sites flickering that have background images when going back to their tabs: https://bugzilla.mozilla.org/show_bug.cgi?id=1137082

Comment 2

[JustOff]

I'm not shure about the cause but can confirm that images redraw behaviour is annoying in Firefox 36 as it always flicker after 2 minutes idle when switching between tabs.

Comment 3

[johnl15278]

I can also confirm this issue which is really annoying and only started happening in Firefox 36

Some people call it image flashing or image flickering or pop in.

To be specific for what I see, I have multiple tabs that are all finished loading, so the issue isn't images downloading. When I switch tabs, the text for the tab appears first without any images and then a half second later I see the images appear. It really ruins browser experience.

If the issue is that firefox 36 is throwing away decoded images background tabs, then there should be an option to tell firefox don't throw away decoded images unless tab is closed.

Comment 4

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

(In reply to her34her34 from comment #0)
> image.mem.discardable;false
> with this setting, firefox 35 and earlier do not have image flashing when
> switching between fully loaded tabs. after upgrading to firefox 36, images
> flash/redraw when switching tabs

That's definitely a bug. This should still work.

> image.mem.decodeondraw;false

This pref is unrelated to this bug. Its name is actually pretty misleading; it will get renamed in an upcoming release.

Comment 5

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

(In reply to johnl15278 from comment #3)
> If the issue is that firefox 36 is throwing away decoded images background
> tabs, then there should be an option to tell firefox don't throw away
> decoded images unless tab is closed.

That is exactly what's happening. Actually, Firefox always did this, but we've become more aggressive about it recently. While this has significant benefits in terms of memory usage, it can lead to image flashing depending on how you tend to use the browser.

We're already aware of this, and we consider fixing it to be a high priority. (I had hoped to get it fixed in time for 36, but unfortunately there were some more serious issues that had to be solved first.) The work is being tracked in bug 1113855.

image.mem.discardable *should* be the option you want, but it sounds like it's broken in 36. We'll fix that in this bug.

Comment 6

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

Actually, the severe image flashing that some users are experiencing is probably related to bug 1122643. If you download Firefox 37 (on Beta right now) and no longer see the problem, then bug 1122643 is the culprit.

Comment 7

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

her34her34, can you reproduce this on Firefox Beta? image.mem.discardable seems to work just fine here. It may be that this has nothing to do with image discarding, and is really just a symptom of bug 1122643.

Comment 8

[JustOff]

I've checked 37b2 with same profile as 36 and can confirm that images flickering when redraw inactive tab issue isn't occuring @win7x64.

Comment 9

[her34her34]

The problem still happens in Firefox 37.0b2. 

Observations:
*The problem is not as severe as happens in firefox 36. If you switch to a tab with 10 images, in firefox 36 you will see image 2-10 pop in which is very noticeable. In firefox 37.0b2 you will see image 9-10 pop in which some people might not notice.  
*the other bug 1122643 sounds unrelated to this issue. bug 1122643 mentions that images not yet loaded are black, in this issue the missing area shows the background of the website, meaning you see what is behind the image. bug 1122643 mentions that a reload of the tab is necessary to fix the problem, in this issue the image will always appear after a second or two. bug 1122643 mentions they see it on android version of firefox, I am using windows version
*In firefox 37.0b2 you will see image pop in while scrolling down page. I manually scrolled down each tab after first load to make sure pop in wasn't caused by lazy loading script from page, waited few minutes, then tested tabs. So pop in happens from switching tabs and from scrolling
*There is a slight delay sometimes when switching tabs in firefox 37.0b2. I wonder if this delay is masking image decoding, which makes the problem seem not as severe as firefox 36.
*Based on memory consumption, decoded images are being discarded. With firefox 35 set to image.mem.discardable;false, I loaded enough tabs to reach 2.0 gb ram after tabs finished loading and firefox cpu settle to 0%. Loading same tabs with firefox 35 set to image.mem.discardable;true and ram reaches 1.3gb. In firefox 37.0b2 ram reaches 1.3gb regardless of setting true or false.

*My conclusion from testing is that firefox 37.0b2 is still discarding decoded images but does a better job of masking it. image.mem.discardable;false doesn't work as it has in previous firefox versions <36.

Comment 10

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

(In reply to her34her34 from comment #9)
> *the other bug 1122643 sounds unrelated to this issue. bug 1122643 mentions
> that images not yet loaded are black, in this issue the missing area shows
> the background of the website, meaning you see what is behind the image. bug
> 1122643 mentions that a reload of the tab is necessary to fix the problem,
> in this issue the image will always appear after a second or two. bug
> 1122643 mentions they see it on android version of firefox, I am using
> windows version

We're pretty sure that bug does happen on Windows as well, and it may not necessarily require a reload for the images to reappear. (We added code that should make the images reappear automatically.) But if you see the background of the website and not a black area, then you're definitely not seeing bug 1122643. So it sounds like we can remove that from the list of possibilities.

> *In firefox 37.0b2 you will see image pop in while scrolling down page. I
> manually scrolled down each tab after first load to make sure pop in wasn't
> caused by lazy loading script from page, waited few minutes, then tested
> tabs. So pop in happens from switching tabs and from scrolling

That is very strange behavior! When you first load the page, we should decode all the images on it. They're decoded asynchronously, so it's possible to scroll the page faster than the images on it can be decoded, but that's nothing new, so if you're seeing a change that's definitely interesting.

We do throw away images that aren't visible after a while. We should still redecode them before you scroll them back onscreen, though. (And of course, image.mem.discardable should stop us from doing that.)

> *There is a slight delay sometimes when switching tabs in firefox 37.0b2. I
> wonder if this delay is masking image decoding, which makes the problem seem
> not as severe as firefox 36.

It's possible.

> *Based on memory consumption, decoded images are being discarded. With
> firefox 35 set to image.mem.discardable;false, I loaded enough tabs to reach
> 2.0 gb ram after tabs finished loading and firefox cpu settle to 0%. Loading
> same tabs with firefox 35 set to image.mem.discardable;true and ram reaches
> 1.3gb. In firefox 37.0b2 ram reaches 1.3gb regardless of setting true or
> false.

Yes, that is really strong evidence that the images are being discarded!

I'm not sure why you're seeing different behavior than I am here. I'll try again to reproduce the issue.

Comment 11

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

(In reply to Seth Fowler [:seth] from comment #10)
> I'm not sure why you're seeing different behavior than I am here. I'll try
> again to reproduce the issue.

OK, I did try again, and I was able to reproduce the issue. I must not have waited long enough for the images to be discarded last time. Sorry about that.

I've also found the problem, so we'll get this fixed ASAP.

Comment 12

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

Attachment: Increment RasterImage::mLockCount to ensure that non-discardable images don't eventually become unlocked

Here's the fix. (Note that we can't just call RasterImage::LockImage(), because
RasterImage::Init() runs off-main-thread sometimes.)

Comment 13

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8575727 [details] [diff] [review]
Increment RasterImage::mLockCount to ensure that non-discardable images don't eventually become unlocked

Refresh my memory, how did this work before?

Comment 14

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

(In reply to Timothy Nikkel (:tn) from comment #13)
> Comment on attachment 8575727 [details] [diff] [review]
> Increment RasterImage::mLockCount to ensure that non-discardable images
> don't eventually become unlocked
> 
> Refresh my memory, how did this work before?

In the past we wouldn't add ourselves to the list of discardable images that the DiscardTracker tracked if mDiscardable was false. Since the SurfaceCache owns all surfaces now, we can't use a similar approach, but it's enough to call SurfaceCache::LockImage() to get the same effect. The only problem is that the locking state of an image in the SurfaceCache is a boolean, not a counter, so if we end up calling SurfaceCache::UnlockImage() later (say because the image isn't visible anymore), the image will get unlocked anyway. Incrementing the lock count in RasterImage::Init() prevents us from ever doing that.

(In the long term it would be better to make the SurfaceCache's image locking state use a counter and not a boolean, but it wouldn't be upliftable and there are some other issues blocking that change.)

Comment 15

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

Comment on attachment 8575727 [details] [diff] [review]
Increment RasterImage::mLockCount to ensure that non-discardable images don't eventually become unlocked

Approval Request Comment
[Feature/regressing bug #]: Let's call it bug 1104622.
[User impact if declined]: The image.mem.discardable pref won't work.
[Describe test coverage new/current, TreeHerder]: None.
[Risks and why]: There is virtually zero risk here - this is a trivial one line patch that only affects a non-default configuration. It's a non-default configuration that is important to some users, though.
[String/UUID change made/needed]: None.

Comment 16

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

Pushed to central:

https://hg.mozilla.org/integration/mozilla-inbound/rev/66a2384dde3c

Comment 17

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

(s/central/inbound, of course)

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/66a2384dde3c

Comment 19

[JustOff]

Can we expect that this fix will be included in version 36.0.x or at least 37?

Comment 20

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

(In reply to JustOff from comment #19)
> Can we expect that this fix will be included in version 36.0.x

Definitely not, no.

> or at least 37?

I requested it in comment 15, but the feedback I'm getting so far leads me to think that it won't make it into 37, as we only have one beta left to go and the bar for entry is pretty high.

Based on what I know at this point, I'd expect the fix to end up in 38.

Comment 21

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8575727 [details] [diff] [review]
Increment RasterImage::mLockCount to ensure that non-discardable images don't eventually become unlocked

I reviewed this change with Seth. There is very little risk of regression and the regression should be very visible. This fix also has potential benefit for the default case. I'm OK taking this in Beta 7. Beta+ Aurora+

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fdf7355134e1

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/52b55d9c1d61

Comment 24

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

(In reply to JustOff from comment #19)
> Can we expect that this fix will be included in version 36.0.x or at least
> 37?

So an update here - thanks to the kind approval from Lawrence in comment 21, this will make it into 37.

Comment 25

[JustOff]

Thank you so much, Seth Fowler!

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/52b55d9c1d61