hang/asserts after watching webcam

RESOLVED FIXED

Status

()

Core
ImageLib
P2
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Joe Drew (not getting mail), Assigned: Joe Drew (not getting mail))

Tracking

({fixed1.9.1, regression})

Trunk
x86
Mac OS X
fixed1.9.1, regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

9 years ago
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
Why does this block bug 373701?
Blocks: 430061
Flags: blocking1.9.1?
Keywords: regression
(Assignee)

Comment 2

9 years ago
Because I can't review it until this is fixed.
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).
(Assignee)

Comment 4

9 years ago
Yeah, it's strictly a testing issue. I didn't think of 1.9.0 - I will try there.
(Assignee)

Comment 5

9 years ago
Created attachment 339300 [details] [diff] [review]
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.
Attachment #339300 - Flags: review?(pavlov)
(Assignee)

Comment 6

9 years ago
Created attachment 339301 [details] [diff] [review]
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.
Attachment #339300 - Attachment is obsolete: true
Attachment #339301 - Flags: review?(pavlov)
Attachment #339300 - Flags: review?(pavlov)
> 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.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2

Updated

9 years ago
Attachment #339301 - Flags: review?(pavlov) → review+
(Assignee)

Comment 8

9 years ago
Pushed with http://hg.mozilla.org/mozilla-central/rev/82cd7332e973
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 9

9 years ago
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.
Keywords: fixed1.9.1
You need to log in before you can comment on or make changes to this bug.