1168985 - images inserted at the top of a document can fail to decode even though visible

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[David Flanagan [:djf]]

Attachment: reduced test case

If I'm dynamically adding images to a document and add them at the bottom of the document without the user doing any scrolling, then most of those images will never even be decoded. Memory use remains low and everything is good.

But if I add each image at the top of the document, then each new image starts off visible and is decoded.  As new images are decoded, the old ones gets pushed off the bottom of the viewport and stop being visible.  In this case memory use is much higher, as expected.

But there is also bug: once image memory is exhausted, the images inserted at the top of the document are not decoded, even though they are visible. Gecko apparently does not realize that the images that have been pushed off the bottom of the viewport are not visible anymore and can have their image memory freed.

If the user scrolls, even just a tiny bit, then the images at the top of the document get decoded and all is well again. I've found that I can work around this bug with a manual scroll when inserting images at the top of the document.

This affects MacOS and I believe that it is also the bug underlying FirefoxOS Gallery bug 1164164 and bug 1163225.

Comment 1

[David Flanagan [:djf]]

Seth: I suspect this is related to the thumbnail failure in 2.2+ bug 1163225. If I modify the gallery app so that it always adds thumbnails at the end, then we never get the blank black thumbnails.  I have not yet tried to extend this reduce test case to cover image decoding to draw into an off-screen canvas, so I don't know if the scrollBy() workaround fixes that or not.

Comment 2

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

Attachment: Always update image visibility after reflow

This patch eliminates the problem totally. It's so simple that I don't
understand why we weren't already doing this. Timothy, is some other code
expected to take care of this, and failing to do so?

Comment 3

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0295df6119f3

Comment 4

[David Flanagan [:djf]]

Attachment: test case variant more like the thumbnail creation code in the gallery app

Here's a variant of the test case where we decode the image, use drawImage() to copy it to a canvas, then encode the canvas. Its like the thumbnail creation code in the Gallery app, except we keep the images at full 4megapixel size so we can exhaust memory on desktop.  The results are basically the same with this version of the test case as they are with the other attachment, but in Firefox 38, we get an exception when the drawImage() fails. And in Firefox 40 and 41, we just get a broken image instead.  (In FirefoxOS we get a black image, but I couldn't reproduce that on desktop)

Comment 5

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

(In reply to David Flanagan [:djf] from comment #4)
> The results are
> basically the same with this version of the test case as they are with the
> other attachment, but in Firefox 38, we get an exception when the
> drawImage() fails.

Yep, the previous behavior was a violation of the spec - see bug 1162282.

We can look into way to mitigate this - including detecting the OOM case specifically and throwing a special exception in that case - but none of those changes would be upliftable to 2.2.

Comment 6

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

(But we shouldn't need to do any mitigating with the patch in this bug.)

Comment 7

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8611448 [details] [diff] [review]
Always update image visibility after reflow

A full image visibility update, and the "after first reflow" for image frames are intended to get most images, but they can never mark all images visible that need to be. And even if we do it after every reflow we will still miss for example visibility:hidden changing to visibility:visible. And that is okay because any change which can make an image visible should cause invalidation, and the following paint should call Draw() on the image, which should call UnlockedDraw, which will in turn add the images to the visible list.

So why aren't we calling Draw on this image?

Comment 8

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

Timothy and I discussed this on IRC, but for the benefit of others reading this bug: actually, what we're trying to do here is remove the image from the visible list, not add it.

As long as Reflow() gets called, the approach in this patch works, and it does indeed fix the testcase posted in this bug. However, after writing this patch I spent a lot of time debugging the Gallery app's image visibility issues, and one thing that became clear to me is that we can't rely on Reflow() being called just because we're reflowing frames elsewhere in the frame tree.

We need a better way to ensure that images get expired from the visible list.

Comment 9

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

We now have that better way; see bug 1169880 for the patch. That patch fixes the testcase in this bug as well.

Comment 10

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

This should be fixed by bug 1169880, which landed today. Please reopen if that's not the case.