417115 - animated image causes recursion during shutdown [@ imgContainer::Release][@ nsTimerImpl::ReleaseCallback][@ nsTimerImpl::Cancel][@ imgContainer::~imgContainer][@ nsTimerImpl::ReleaseCallback][@ nsTimerImpl::Cancel]

Status: RESOLVED FIXED

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

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

The problem in bug 417101 is this:

0|103516|xul.dll|imgContainer::`vector deleting destructor'(unsigned int)
0|103517|xul.dll|imgContainer::Release()
0|103518|xul.dll|nsTimerImpl::ReleaseCallback()
0|103519|xul.dll|nsTimerImpl::Cancel()
0|103521|xul.dll|imgContainer::~imgContainer()
0|103522|xul.dll|imgContainer::`vector deleting destructor'(unsigned int)
0|103523|xul.dll|imgContainer::Release()
0|103524|xul.dll|nsTimerImpl::ReleaseCallback()
0|103525|xul.dll|nsTimerImpl::Cancel()
0|103526|xul.dll|imgContainer::Anim::`scalar deleting destructor'(unsigned int)
0|103527|xul.dll|imgContainer::~imgContainer()
0|103528|xul.dll|imgContainer::`vector deleting destructor'(unsigned int)
0|103529|xul.dll|imgContainer::Release()
0|103530|xul.dll|nsTimerImpl::ReleaseCallback()
0|103531|xul.dll|TimerThread::Shutdown()
0|103532|xul.dll|NS_ShutdownXPCOM_P

Basically, we're trying to shut down the timer which is the last thing that apparently owns the imgContainer, by virtue of the release callback.  The imgContainer makes sure to Cancel the timer, which in turn calls ReleaseCallback again.

One fix is to make ReleaseCallback recursion safe.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: fix

Protect against recursive calls to ReleaseCallback -- only ever release once.

Comment 2

[Stuart Parmenter]

Comment on attachment 302938 [details] [diff] [review]
fix

add a comment explaining the recursion here

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Wow, this is totally a bug in the NS_RELEASE macro, nsCOMPtr is dealing with this by setting the pointer to null before calling Release. Can't believe we don't run into this more.

Comment 5

[Mike Schroepfer]

(In reply to comment #4)
> Wow, this is totally a bug in the NS_RELEASE macro, nsCOMPtr is dealing with
> this by setting the pointer to null before calling Release. Can't believe we
> don't run into this more.
> 

Is there something else we need to fix for NS_RELEASE?

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The currently attached patch doesn't touch NS_RELEASE at all.

It would be interesting to try to fix this the right way by fixing NS_RELEASE. It would increase the binary size, but I have no idea how much. It would definitely be a scary change though since it's such a central function, but it seems really unlikely that it would break anything.

That said, this will be a non-issue in mozilla2 since we're getting rid of refcounting.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yeah, I don't want to touch NS_RELEASE -- especially since it's a macro, who knows what people are doing (e.g. calling NS_RELEASE on the address of a reference or something).

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

That shouldn't be a problem. Something like this should work in all cases:

template <class T>
inline
void
ns_release( T& expr )
{
    T old = expr;
    expr = 0;
    old->Release();
}

#define NS_RELEASE(_expr) ns_release(_expr)


We'd need similar code for NS_RELEASE2.

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Let's spin a new bug off for NS_RELEASE, as it's a larger issue and if it causes regressions we'll probably want to back it out; in the meantime, can we land this fix for animated images?