369214 - ASSERTION: The observer list changed while being iterated over!: 'count == mObservers.Count()'

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Mats Palmgren (inactive)]

STEPS TO REPRODUCE
1. load the URL
2. Reload

ACTUAL RESULT
###!!! ASSERTION: The observer list changed while being iterated over!: 'count == mObservers.Count()', file imgRequest.cpp, line 608
###!!! ASSERTION: nsVoidArray::ElementAt: index out of range: '0 <= aIndex && aIndex < Count()', file ../../../dist/include/xpcom/nsVoidArray.h, line 81
###!!! ASSERTION: The observer list changed while being iterated over!: 'count == mObservers.Count()', file imgRequest.cpp, line 608


PLATFORMS AND BUILDS TESTED
Firefox debug build on Linux (~2 hours old)
SeaMonkey debug build on Linux (a day or so old)
Camino debug build on MacOSX 10.4.8 (~1 hour old)

Comment 1

[Mats Palmgren (inactive)]

Attachment: stack

Comment 2

[Boris Zbarsky [:bzbarsky]]

So this is odd.  I can't reproduce over here with seamonkey or Firefox on Linux...

I'd be curious to find out what the stack is to the observer removing itself..

That said, I see nothing in the imagelib APIs that says observers can't remove themselves at any point.  So I think the right approach is to put nsTObserverArray into XPCOM and use it here...

Comment 3

[Mats Palmgren (inactive)]

Attachment: stack #2

Comment 4

[Boris Zbarsky [:bzbarsky]]

Um.  But we cancel mCurrentRequest, and only when being notified by mPendingRequest.

Are both proxies for the same underlying request?  I suppose we can get into that state if someone does a force-load for the same URI we're already loading... but there's really no way that we can detect that situation outside imagelib, nor should we have to, imo.

Comment 5

[Mats Palmgren (inactive)]

Attachment: stack #3

Not sure what you're asking for, does this help?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yeah, those are both proxies for the same request.  So I think we should do what comment 2 paragraph 3 says (and do it in imagelib).

Comment 7

[Nickolay_Ponomarev]

Attachment: switch to nsTObserverArray + minor cleanup

Comment 8

[Nickolay_Ponomarev]

http://www.mozilla.org/hacking/portable-cpp.html#mutable says not to use |mutable|, is it still the rule or can we try if it builds these days?

Comment 9

[Boris Zbarsky [:bzbarsky]]

How's compiler support for it?

Note also that this patch is a step away from fixing bug 304870.  I'm not really sure what we can do about that...

Comment 10

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

If you think it'll work you can try it -- you just need to be prepared to back out or fix.

Comment 11

[Nickolay_Ponomarev]

(In reply to comment #9)
I don't know about compiler support, but given that we're trying to use more of standard C++ features these days, I think it makes sense to try it.

> Note also that this patch is a step away from fixing bug 304870.  I'm not
> really sure what we can do about that...
> 
Do you mean this patch makes 304870's perf worse / makes it harder to fix?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Just harder to fix, if it's going to not regress this bug.

Comment 13

[Stuart Parmenter]

Comment on attachment 278895 [details] [diff] [review]
switch to nsTObserverArray + minor cleanup

this patch looks OK to me.  We might want to make nsTObserverArray handle large lists differently to solve the perf problem that bz mentioned.

Comment 14

[Nickolay_Ponomarev]

Comment on attachment 278895 [details] [diff] [review]
switch to nsTObserverArray + minor cleanup

Didn't realize this "red" threat level was a real code freeze, this can certainly wait until after M8.

Comment 15

[Nickolay_Ponomarev]

mozilla/modules/libpr0n/src/imgRequest.cpp       1.97
mozilla/modules/libpr0n/src/imgRequest.h         1.38
mozilla/modules/libpr0n/src/imgRequestProxy.cpp  1.58
mozilla/modules/libpr0n/src/imgRequestProxy.h    1.28
mozilla/xpcom/glue/nsTObserverArray.h            1.3

Comment 16

[Nickolay_Ponomarev]

Everything is still green. If no problems arise in the near future, we probably should update the portability guide to remove the restriction on |mutable|.

Comment 17

[Nickolay_Ponomarev]

Two years later nobody complained, so I updated https://developer.mozilla.org/index.php?title=en/C%2B%2B_Portability_Guide#The_mutable_keyword