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

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect, P1)

Description

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

We had no Windows builds between Feb. 11 and Feb. 16; a new accessibility topcrash appeared in the Feb. 16 builds.

Unfortunately, due to PGO, etc., the topcrash's signature changes every day.  The ones for yesterday are:
http://crash-stats.mozilla.com/report/list?product=Firefox&branch=1.9.3&platform=windows&query_search=signature&query_type=exact&query=&date=&range_value=31&range_unit=days&process_type=all&plugin_field=&plugin_query_type=&plugin_query=&do_query=1&signature=nsRefPtr%3CAddStyleSheetTxn%3E%3A%3Aassign_assuming_AddRef%28AddStyleSheetTxn*%29%20|%20nsAccEventQueue%3A%3AShutdown%28%29
and the ones for today are:
http://crash-stats.mozilla.com/report/list?product=Firefox&branch=1.9.3&platform=windows&query_search=signature&query_type=exact&query=&date=&range_value=1&range_unit=weeks&process_type=all&plugin_field=&plugin_query_type=&plugin_query=&do_query=1&signature=nsRefPtr%3CnsGenericElement%3E%3A%3Aassign_assuming_AddRef%28nsGenericElement*%29%20|%20nsAccEventQueue%3A%3AShutdown%28%29

Comment 1

[Marco Zehe (:MarcoZ)]

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.

Comment 2

[alexander :surkov (:asurkov)]

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.

Comment 3

[alexander :surkov (:asurkov)]

(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.

Comment 4

[Marco Zehe (:MarcoZ)]

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.

Comment 5

[alexander :surkov (:asurkov)]

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?

Comment 6

[alexander :surkov (:asurkov)]

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.

Comment 7

[alexander :surkov (:asurkov)]

(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).

Comment 8

[alexander :surkov (:asurkov)]

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.

Comment 9

[alexander :surkov (:asurkov)]

Attachment: 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.

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 428535 [details] [diff] [review]
patch

r=me. Thanks!

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

Alexander can you confirm if this fixes the bug? We should land a fix asap since this is a topcrash.

Comment 12

[alexander :surkov (:asurkov)]

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.

Comment 13

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

Verified that this is no longer appearing in crash-stats for Saturday's builds.