455606 - hang/asserts after watching webcam

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P2)

Description

[Joe Drew (not getting mail)]

This is fallout from bug 430061, but other things might be related.

If you watch a webcam (such as the one linked) for a short amount of time, the image cache slowly fills up (for a currently unknown reason). Also, we seemingly get lines like the ones below, one for each frame:

++DOMWINDOW == 62 (0x1138e0fc) [serial = 65] [outer = 0x155f5d80]
++DOMWINDOW == 63 (0x1138b18c) [serial = 66] [outer = 0x155f5d80]
++DOMWINDOW == 64 (0x11393b2c) [serial = 67] [outer = 0x155f5d80]

The real problem is that if you then proceed to try to load a new image, you get 

###!!! ASSERTION: imgLoader::CheckCacheLimits -- NULL entry pointer: 'entry', file /Users/joe/mozilla-central/modules/libpr0n/src/imgLoader.cpp, line 687

Comment 1

[Boris Zbarsky [:bzbarsky]]

Why does this block bug 373701?

Comment 2

[Joe Drew (not getting mail)]

Because I can't review it until this is fixed.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Why not, if I might ask?  If it's a matter of not being able to test, that patch should also apply to Firefox 3 branch (and needs to land on there, preferably before the next dot release).

Comment 4

[Joe Drew (not getting mail)]

Yeah, it's strictly a testing issue. I didn't think of 1.9.0 - I will try there.

Comment 5

[Joe Drew (not getting mail)]

Attachment: remove old cache elements when pushing new ones

For reasons I don't understand, when you load a webcam like the one linked above, imagelib gets many, many calls to LoadImageWithChannel. Due to a bug in imagelib, we didn't remove the old entries in the cache when we got a new element that couldn't be verified over the network. This patch fixes that by removing pre-existing elements if they're already in the cache before putting a new element into the cache.

Comment 6

[Joe Drew (not getting mail)]

Attachment: Slight fix to the previous patch

The previous patch had a small nit - we called RemoveFromCache(entry) when |entry| is the cache entry we're trying to add. It didn't actually matter, because of the way RemoveFromCache is implemented, but we shouldn't do that.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> imagelib gets many, many calls to LoadImageWithChannel.

This is a full-page image (going through nsImageDocument), which means that you get a LoadImageWithChannel for each channel.  The multipart converter creates one channel per part, so you're getting a LoadImageWithChannel per frame, basically.

Comment 8

[Joe Drew (not getting mail)]

Pushed with http://hg.mozilla.org/mozilla-central/rev/82cd7332e973

Comment 9

[Joe Drew (not getting mail)]

Couple of comments I should have made earlier:

What was happening was that |imgCacheEntry|s were allocated, but PutIntoCache would return false (which wasn't checked) because an entry with that key already existed in the cache. So the caller would continue on, assuming that the element was in the cache.Further, when the request received callbacks as data was received, it'd increase the size of the cache.

Eventually, the base size of the cache would be exceeded with these "phantom entries," leaving the cache-clearing code in an infinite loop as it tried to free elements that didn't exist to reduce the cache below its maximum size.