420055 - New shutdown crash

Status: RESOLVED FIXED

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

Description

[Aaron Leventhal]

I'm darn sure this was caused by bug 417249. I changed that line in that bug and how our doc caching works. It started crashing on Feb 27. 

http://crash-stats.mozilla.com/report/list?range_unit=days&query_search=stack&query_type=contains&version=Firefox%3A3.0b4pre%2CFirefox%3A3.0b5pre%2CSeaMonkey%3A2.0a1pre%2CSunbird%3A0.6a1%2CThunderbird%3A3.0a1pre&signature=PL_DHashTableEnumerate&query=Access&range_value=1

That fix actually improved our stability, and at least this new crash doesn't happen until shutdown, but we should figure this new one out ASAP.

Comment 1

[Aaron Leventhal]

The problem might be caused from nsDocAccessible::Shutdown() now removing that doc accessible from the cache.

During ShutdownXPAccessibility() at xpcom shutdown we use a cache enumerator to shutdown the remaining docs. It probably doesn't like that the current item being enumerate in the cache no longer exists.

Comment 2

[Aaron Leventhal]

Attachment: 1) Use ClearCache() again in ShutdownXPAccessiblity(), 2) Change nsDocAccessible::Shutdown() so it doesn't remove |this| from cache when we're in ShutdownXPAccessibility()

This uses a different approach to avoid double cache removal during XPCOM shutdown, when PL_DHASH_REMOVE and cache enumeration is used to remove docs from doc cache.

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)
> This uses a different approach to avoid double cache removal during XPCOM
> shutdown, when PL_DHASH_REMOVE and cache enumeration is used to remove docs
> from doc cache.
> 

Aaron, could you give more details on this because I can't see difference between approaches?

Comment 4

[Aaron Leventhal]

Hi, I am going back to the previous approach because that is when the enw crash showed up. It is hard to say, but my guess from looking at crash stats is that this is the channge that caused it. It is probably because the new code deleted the item it was curently on while iterating through the hash table. If you delete the item zou are currently on then you might lose your place. Fortunately the has iterator has the PL_DHASH_REMOVE feature to help remove items while iterating, and that worked in the past, so we should go back to it and see if it fixes the crash.

Comment 5

[alexander :surkov (:asurkov)]

Comment on attachment 306246 [details] [diff] [review]
1) Use ClearCache() again in ShutdownXPAccessiblity(), 2) Change nsDocAccessible::Shutdown() so it doesn't remove |this| from cache when we're in ShutdownXPAccessibility()

ok, I thought about this
r=me

Comment 6

[Marco Zehe (:MarcoZ)]

Comment on attachment 306246 [details] [diff] [review]
1) Use ClearCache() again in ShutdownXPAccessiblity(), 2) Change nsDocAccessible::Shutdown() so it doesn't remove |this| from cache when we're in ShutdownXPAccessibility()

Requesting approval on Aaron's behalf. He's on the plane and won't be available for several hours. Alexander, can you check this in when it is approved?

Comment 7

[Mike Connor [:mconnor]]

Comment on attachment 306246 [details] [diff] [review]
1) Use ClearCache() again in ShutdownXPAccessiblity(), 2) Change nsDocAccessible::Shutdown() so it doesn't remove |this| from cache when we're in ShutdownXPAccessibility()

rs/sr=mconnor

a=mconnor on behalf of drivers, someone please land this ASAP

Comment 8

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

Comment on attachment 306246 [details] [diff] [review]
1) Use ClearCache() again in ShutdownXPAccessiblity(), 2) Change nsDocAccessible::Shutdown() so it doesn't remove |this| from cache when we're in ShutdownXPAccessibility()

a1.9b4=beltzner

Comment 9

[alexander :surkov (:asurkov)]

checked in