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

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
Disability Access APIs
--
critical
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: MarcoZ, Assigned: surkov)

Tracking

({access, crash})

Trunk
mozilla1.9.2a1
access, crash
Points:
---

Firefox Tracking Flags

(blocking1.9.1 -, status1.9.1 .3-fixed)

Details

(crash signature)

Attachments

(2 attachments, 2 obsolete attachments)

Duplicate of this bug: 429644
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)
Assignee: nobody → david.bolter
Status: NEW → ASSIGNED
Created attachment 366844 [details] [diff] [review]
patch 1

Thoughts?
Attachment #366844 - Flags: review?(surkov.alexander)
Attachment #366844 - Flags: review?(marco.zehe)
(Reporter)

Updated

10 years ago
Attachment #366844 - Flags: review?(marco.zehe) → review+
(Reporter)

Comment 5

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

Comment 7

10 years ago
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.
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?
Ah found it in nsIAccessibilityService.idl (grep fail)
(Reporter)

Comment 10

9 years ago
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.
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!
Version: 1.9.0 Branch → unspecified
When you have a reviewed patch, please ask for approval.
blocking1.9.1: ? → -
status1.9.1: --- → wanted
(Assignee)

Comment 13

9 years ago
(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.
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.
(Assignee)

Comment 15

9 years ago
(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.
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.
(Assignee)

Comment 17

9 years ago
(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.
(Assignee)

Comment 18

9 years ago
Created attachment 390208 [details] [diff] [review]
patch
Assignee: bolterbugz → surkov.alexander
Attachment #390208 - Flags: review?(marco.zehe)
Attachment #390208 - Flags: review?(bolterbugz)
(Assignee)

Comment 19

9 years ago
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.
Attachment #366844 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 20

9 years ago
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
(Reporter)

Comment 21

9 years ago
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.
(Reporter)

Comment 22

9 years ago
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.
Attachment #390208 - Flags: review?(marco.zehe) → review+
(Assignee)

Comment 23

9 years ago
Bug 504108 might be duplicate of this one.
(Assignee)

Updated

9 years ago
Attachment #390208 - Flags: review?(ginn.chen)

Comment 24

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

Do we still need gIsShuttingDownApp then?
(Assignee)

Comment 25

9 years ago
(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.
(Assignee)

Comment 26

9 years ago
Created attachment 391010 [details] [diff] [review]
patch2
Attachment #390208 - Attachment is obsolete: true
Attachment #391010 - Flags: review?(ginn.chen)
Attachment #390208 - Flags: review?(ginn.chen)
Attachment #390208 - Flags: review?(bolterbugz)

Comment 27

9 years ago
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.
(Assignee)

Comment 28

9 years ago
(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.
(Assignee)

Comment 29

9 years ago
Created attachment 391050 [details] [diff] [review]
patch3
Attachment #391010 - Attachment is obsolete: true
Attachment #391050 - Flags: review?(ginn.chen)
Attachment #391010 - Flags: review?(ginn.chen)

Updated

9 years ago
Attachment #391050 - Flags: review?(ginn.chen) → review+
(Assignee)

Comment 30

9 years ago
landed on 1.9.2 - http://hg.mozilla.org/mozilla-central/rev/70226f6e7c98
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
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.
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)
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?
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 on attachment 366844 [details] [diff] [review]
patch 1

yeah, I should just have done it; sorry for the pointless pingpong
Attachment #366844 - Flags: approval1.9.1.2?
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 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.
Attachment #366844 - Flags: approval1.9.1.2? → approval1.9.1.2+
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... :-/
(Assignee)

Comment 39

9 years ago
patch1 is landed on mozilla 1.9.1 -
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/017b8cd48bed
(Assignee)

Updated

9 years ago
status1.9.1: wanted → .2-fixed
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.
status1.9.1: .2-fixed → .3-fixed
Attachment #366844 - Flags: approval1.9.1.2+ → approval1.9.1.3+
OS: Windows Vista → All
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.2a1
Version: unspecified → Trunk
Crash Signature: [@ nsAccessNode::GetDocAccessibleFor(nsIDocShellTreeItem*, int) ]
You need to log in before you can comment on or make changes to this bug.