topcrash regression: null dereference in nsRefPtr<T>::assign_assuming_AddRef(AddStyleSheetTxn*) | nsAccEventQueue::Shutdown() [@nsRefPtr<AddStyleSheetTxn>::assign_assuming_AddRef(AddStyleSheetTxn*) | nsAccEventQueue::Shutdown()]

VERIFIED FIXED

Status

()

Core
Disability Access APIs
P1
critical
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: dbaron, Assigned: surkov)

Tracking

({crash, topcrash})

Trunk
x86
Windows XP
crash, topcrash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(1 attachment)

At first glance, this would implicate bug 515141, however, that landed on January 27, 2010 already.

Moreover, I haven't seen this crash myself, I've been running the Feb 16 build all day yesterday without problems.
(Assignee)

Comment 2

8 years ago
I looks like the crash happens when document accessible object is destroyed by garbage collector and we try to release the reference on the document object. However I'm not sure why this is not allowed.
(Assignee)

Comment 3

8 years ago
(In reply to comment #1)
> At first glance, this would implicate bug 515141, however, that landed on
> January 27, 2010 already.

this must be 515141 since before it the evens queue logic was inside of document accessible. Possibly something else starts to fail this new code.
Possibly related bugs that landed:
bug 545391
bug 543961

These and bug 534527 were the only ones that landed in that timeframe on a11y side.
(Assignee)

Comment 5

8 years ago
According to stack nsRefPtr<nsDocAccessible>::assign_assuming_AddRef(nsnull) should crash http://hg.mozilla.org/mozilla-central/annotate/46484930f4f3/xpcom/base/nsAutoPtr.h#l954 when nsDocAccessible instance is destroyed by cycle collector.

Could we cc someone who knows the cycle collector work?
(Assignee)

Comment 6

8 years ago
It looks like we release nsDocAccessible instance when it is destroyed by cycle collection what leads the object destabilization (i.e. mRefCnt == 0 again) - http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsImpl.h#422. We could do not release the nsDocAccessible instance while it's destroyed but it would be really nice to understand what really happen there.
(Assignee)

Comment 7

8 years ago
(In reply to comment #6)

Actually we delete the object twice, i.e. NS_DELETEXPCOM (http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsUtils.h#80) is called twice. The first time it's called by cycle collector (Release() method) which stabilize mRefCnt (mRefCnt = 1), then nsAccEventQueue calls Release() second time what calls NS_DELETEXPCOM and then NS_DELETEXPOM is called from first Release() method (by cycle collector).
(Assignee)

Comment 8

8 years ago
Neil, David could the NS_IMPL_CYCLE_COLLECTING_RELEASE_FULL and NS_IMPL_RELEASE_WITH_DESTROY be changed to be protected from recurring calls of the destroy() function? I think it should be right way to fix this bug.
Severity: normal → critical
Priority: -- → P1
(Assignee)

Comment 9

8 years ago
Created attachment 428535 [details] [diff] [review]
patch

Please ignore my few last comments - I must misread the stack and made wrong assumptions.

It looks like mEventQueue is null when we try to Shutdown it. It might be null because of unsuccessful initialization but I'm not sure what the actual reason is in this case.

Thanks David Baron for the bug discussion.
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #428535 - Flags: review?(bolterbugz)
Comment on attachment 428535 [details] [diff] [review]
patch

r=me. Thanks!
Attachment #428535 - Flags: review?(bolterbugz) → review+
Alexander can you confirm if this fixes the bug? We should land a fix asap since this is a topcrash.
(Assignee)

Comment 12

8 years ago
landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/19efcff85e78

(In reply to comment #11)
> Alexander can you confirm if this fixes the bug? We should land a fix asap
> since this is a topcrash.

I haven't steps to reproduce. But the patch should fix the bug.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Reporter)

Comment 13

8 years ago
Verified that this is no longer appearing in crash-stats for Saturday's builds.
Status: RESOLVED → VERIFIED
Crash Signature: [@nsRefPtr<AddStyleSheetTxn>::assign_assuming_AddRef(AddStyleSheetTxn*) | nsAccEventQueue::Shutdown()]
You need to log in before you can comment on or make changes to this bug.