468659 - Crash [@ nsAccessNode::GetDocAccessibleFor(nsIDocShellTreeItem*, int) ]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

This is the top crash in Firefox 3.0.4 on Windows within the last week.

http://crash-stats.mozilla.com/report/list?product=Firefox&branch=1.9&version=Firefox%3A3.0.4&platform=windows&query_search=signature&query_type=contains&query=Accessible&date=&range_value=7&range_unit=days&do_query=1&signature=nsAccessNode%3A%3AGetDocAccessibleFor%28nsIDocShellTreeItem*%2C%20int%29
16 total.

Comment 1

[Marco Zehe (:MarcoZ)]

Also the second top crasher on 1.9.1: http://crash-stats.mozilla.com/report/list?product=Firefox&branch=1.9.1&version=Firefox%3A3.1b2&version=Firefox%3A3.1b2pre&version=Firefox%3A3.1b3pre&platform=windows&query_search=signature&query_type=contains&query=Access&date=&range_value=9&range_unit=days&do_query=1&signature=nsAccessNode%3A%3AGetDocAccessibleFor%28nsIDocShellTreeItem%2A%2C%20int%29

Comment 3

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

Less frequent but still appears in 1.9.2:
http://crash-stats.mozilla.com/report/index/79d35b1a-89c9-48cc-9a16-d28e92090309
http://crash-stats.mozilla.com/report/index/82eade61-3bb8-4748-87a7-f3dad2090306
(Windows and Linux)

Comment 4

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

Attachment: patch 1

Thoughts?

Comment 5

[Marco Zehe (:MarcoZ)]

Comment on attachment 366844 [details] [diff] [review]
patch 1

Looks correct, but I'll let surkov have the final say. :-) Do we have this kind of construct lying around elsewhere too with the same potential of failure?

Comment 6

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

(In reply to comment #5)
> (From update of attachment 366844 [details] [diff] [review])
> Looks correct, but I'll let surkov have the final say. :-) Do we have this kind
> of construct lying around elsewhere too with the same potential of failure?

Quite possible.

I need to develop a better understanding of when our code fails to deliver things like the accessibility service. I suspect I might need to understand more about the presshell and nsframe life cycles. It is probably time to watch another dbaron video.

Comment 7

[alexander :surkov (:asurkov)]

I don't understand the problem and therefore I don't know if this patch will fix (I assume this problem may appear in another place when we use GetAccService()). It's very strange the call goes from accessible service but GetAccService returns null. I'll try to reorganize GetAccService to make it more safer.

Comment 8

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

Still happening in Firefox 3.5b4:
http://crash-stats.mozilla.com/report/index/a10c2569-991b-46a8-b9a5-0e5d62090625

By the way, why isn't CreateRootAccessible defined in a header file?

Comment 9

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

Ah found it in nsIAccessibilityService.idl (grep fail)

Comment 10

[Marco Zehe (:MarcoZ)]

Can we have some more progress on this please? it's the top crash in Firefox 3.5, with 44 occurrences over the past week alone.

Comment 11

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

surkov: we're seeing a lot of these crashes on the 3.5 stream.  what do you say to taking this wallpaper on the branch, while you reorganize for 1.9.2? would really appreciate your help here!

Comment 12

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

When you have a reviewed patch, please ask for approval.

Comment 13

[alexander :surkov (:asurkov)]

(In reply to comment #11)
> surkov: we're seeing a lot of these crashes on the 3.5 stream.  what do you say
> to taking this wallpaper on the branch, while you reorganize for 1.9.2? would
> really appreciate your help here!

Mike, there is something wrong how we use accessibility service internally. I'll try to come with portion of clean up for these code in few days to put it in and see how it will affect.

Comment 14

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Does the wallpaper here make that harder?  A more involved clean-up sounds like something that will require a fair amount of baking and test, so I'm looking for some short-term mitigations for our topcrash.

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #14)
> Does the wallpaper here make that harder?  A more involved clean-up sounds like
> something that will require a fair amount of baking and test, so I'm looking
> for some short-term mitigations for our topcrash.

I'm close to put patch which might be a complex for a branch. However I think I will be able to put small patch as well (more or less equivalent to the big one).

I can' say I like a lot current approach because even it fixes this bug but it doesn't guarantee the same problem won't appear in another place.

Comment 16

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Sure, but this specific place is happening to a lot of people, and we can fix it for them at basically zero risk.  Does putting this 3-liner in somehow make it _harder_ to do your fix?  I'm not proposing that we only use the mitigation: just that we put it in place while you work on, test, bake on trunk, and then test on branch the more systematic fix.

Comment 17

[alexander :surkov (:asurkov)]

(In reply to comment #16)
> Sure, but this specific place is happening to a lot of people, and we can fix
> it for them at basically zero risk.  Does putting this 3-liner in somehow make
> it _harder_ to do your fix?

No, it's just not necessary.

>  I'm not proposing that we only use the mitigation:
> just that we put it in place while you work on, test, bake on trunk, and then
> test on branch the more systematic fix.

I don't insist actually.

Comment 18

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 19

[alexander :surkov (:asurkov)]

Comment on attachment 366844 [details] [diff] [review]
patch 1

If we don't want to wait when my patch will be reviewed, tested and etc then I don't mind if we'll take this one. It's really safe, will fix this bug however doesn't address a problem in general.

Comment 20

[alexander :surkov (:asurkov)]

This stack trace describes what happens - http://crash-stats.mozilla.com/report/index/82eade61-3bb8-4748-87a7-f3dad2090306

XPCOM is shutdown (NS_ShutdownXPCOM_P)
We get notified and shutdown accessibility - http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccessibilityService.cpp#162
Then rest events are processed (after our notification) - NS_ProcessNextEvent_P
And accessibility service (it is alive yet) is requested to get accessible for document on focus event (a bit strange though)
We trying to get accessibility service but gAccessibilityActive fails this operation (http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccessNode.cpp#96) because we shutdown accessibility already

Comment 21

[Marco Zehe (:MarcoZ)]

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

One nit I found:
>+   * Points if accessibility service was shutdown.

Better wording:
+   * Indicates whether accessibility service was shutdown.

Am going to try the functionality.

Comment 22

[Marco Zehe (:MarcoZ)]

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

This looks correct, and it behaves itself in my test build. However, for 1.9.1.x, I suggest to take the smaller patch instead for safety.

Comment 23

[alexander :surkov (:asurkov)]

Bug 504108 might be duplicate of this one.

Comment 24

[Ginn Chen]

I think you do not need to ifdef DEBUG for NS_ASSERTION.

Do we still need gIsShuttingDownApp then?

Comment 25

[alexander :surkov (:asurkov)]

(In reply to comment #24)
> I think you do not need to ifdef DEBUG for NS_ASSERTION.

I wrapped this because to move gIsAccessibilityActive to debug-only version. I think we don't need it any more but I decided to save it for the first time in debug.

> Do we still need gIsShuttingDownApp then?

Good catch. I'll replace it by new a11y service's variable.

Comment 26

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 27

[Ginn Chen]

surkov, I know gIsAccessibilityActive is debug-only.
But you still do not need ifdef DEBUG for NS_ASSERTION because NS_ASSERTION itself is debug only.

Comment 28

[alexander :surkov (:asurkov)]

(In reply to comment #27)
> surkov, I know gIsAccessibilityActive is debug-only.
> But you still do not need ifdef DEBUG for NS_ASSERTION because NS_ASSERTION
> itself is debug only.

Ah, I see, we won't have gIsAccessibilityActive placed inside of ns_assertion in non-debug build. Thanks.

Comment 29

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 30

[alexander :surkov (:asurkov)]

landed on 1.9.2 - http://hg.mozilla.org/mozilla-central/rev/70226f6e7c98

Comment 31

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Can someone ask for approval for whatever patch we want to take on 1.9.1?  Don't want to let this sit until it's too late to hit FF 3.5.2.

Comment 32

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

I don't think we want to take this so late in the game for Firefox 3.5.2 if it's not a topcrasher, I'd suggest we wait for 3.5.3.

(currently showing 9 crashes on 3.5.x over the past 5 days)

Comment 33

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Yes, that's why I thought we were going to take the tiny null-check fix above -- why did we ask for approval for the larger patch, rather than as recommended in comment 22?

Comment 34

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

I put an approval request on what I thought you were looking for, sorry. Patches named "patch 1" and "patch 3" are not communicative beyond "the later patch is probably the one you want". I'll let whomever ask for whatever approvals they want whereever and stop making assumptions.

Comment 35

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 366844 [details] [diff] [review]
patch 1

yeah, I should just have done it; sorry for the pointless pingpong

Comment 36

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

Comment on attachment 366844 [details] [diff] [review]
patch 1

My apologies, too, as if I knew this was just a null check I probably would have been fine with it, but we're now holding for a green cycle. If there's an orange on mozilla-1.9.1 and we need to check in a fix, I'm fine for this to go along with it, though I really don't think it's a super high priority for 1.9.1.2.

Comment 37

[Samuel Sidler (old account; do not CC)]

Comment on attachment 366844 [details] [diff] [review]
patch 1

There's orange on mozilla1.9.1 and we're going to need to checkin a fix. If you want this to land, land it asap.

Comment 38

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Did this land?  Was there any notification other than bugmail that there was a tiny window in which to land a fix?  Did drivers ask someone to land on behalf of the patch owner here?  I wasn't reading bugmail for several hours today, so I didn't see the approval happen, and I'm hoping that doesn't just mean that the opportunity was missed... :-/

Comment 39

[alexander :surkov (:asurkov)]

patch1 is landed on mozilla 1.9.1 -
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/017b8cd48bed

Comment 40

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

It didn't make the cutoff for 3.5.2, from what I can tell, no:

2009-07-30 20:43 +0800	David Bolter - Bug 468659 - Crash [@ nsAccessNode::GetDocAccessibleFor(nsIDocShellTreeItem*, int) ], r=surkov, marcoz
...
2009-07-29 21:07 -0700	ffxbld - Added tag FIREFOX_3_5_2_RELEASE for changeset b8dbd5ab1647. CLOSED TREE GECKO1912_20090729_RELBRANCH
2009-07-29 21:07 -0700	ffxbld - Added tag FIREFOX_3_5_2_BUILD1 for changeset b8dbd5ab1647. CLOSED TREE GECKO1912_20090729_RELBRANCH 

Shaver: no notification other than the bugmail, and the window was pretty narrow and the desire to get builds done overnight so QA could start testing in the morning was pretty high. I don't think there was anyone around who could take responsibility for landing this patch. We missed it, but in a variety of ways and places, IMO. It should have been nominated more clearly and sooner in the process, we'll know for next time.

This will be part of 3.5.3, adjusting approval flag and fixed flags to match.