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

RESOLVED FIXED

Status

()

defect
P1
critical
RESOLVED FIXED
11 years ago
8 years ago

People

(Reporter: vlad, Assigned: vlad)

Tracking

({crash})

Trunk
x86
All
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature, URL)

Attachments

(1 attachment)

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.
Flags: blocking1.9+
Posted patch fixSplinter Review
Protect against recursive calls to ReleaseCallback -- only ever release once.
Assignee: nobody → vladimir
Status: NEW → ASSIGNED
Attachment #302938 - Flags: review?(pavlov)

Updated

11 years ago
Priority: -- → P1

Comment 2

11 years ago
Comment on attachment 302938 [details] [diff] [review]
fix

add a comment explaining the recursion here
Attachment #302938 - Flags: review?(pavlov) → review+
Duplicate of this bug: 417101

Updated

11 years ago
Severity: normal → critical
Keywords: crash
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

11 years ago
(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?
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.
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).

Updated

11 years ago
Summary: animated image causes recursion during shutdown → animated image causes recursion during shutdown [@ imgContainer::Release][@ nsTimerImpl::ReleaseCallback][@ nsTimerImpl::Cancel][@ imgContainer::~imgContainer][@ nsTimerImpl::ReleaseCallback][@ nsTimerImpl::Cancel]
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.
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?
Blocks: robots
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Crash Signature: [@ imgContainer::Release] [@ nsTimerImpl::ReleaseCallback] [@ nsTimerImpl::Cancel] [@ imgContainer::~imgContainer] [@ nsTimerImpl::ReleaseCallback] [@ nsTimerImpl::Cancel]
You need to log in before you can comment on or make changes to this bug.