1145724 - Crash in mozilla::a11y::TreeWalker::TreeWalker

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Nicholas Troast [:ntroast]]

The following crash signature was observed. cafbot will upload the decoded minidump and extra file

[@ mozilla::a11y::TreeWalker::TreeWalker | nsAccessibilityService::ContentRemoved | mozilla::ElementRestyler::SendAccessibilityNotifications | mozilla::ElementRestyler::RestyleChildren ]

Related Crash: https://crash-stats.mozilla.com/report/index/dd9a84b3-488d-4957-b33c-5f6c22150315

Comment 1

[cafbot (PoC: ggrisco)]

Attachment: EXTRA file attachment -

Comment 2

[cafbot (PoC: ggrisco)]

Attachment: decoded minidump -

Comment 3

[Mike Lee [:mlee]]

Hi David,

Please assign someone on your team to help to root-cause this crash or route this to the appropriate team.

Thanks!
Mike

Comment 4

[Mike Lee [:mlee]]

Additional info from CAF (Code Aurora Foundation) regarding urgency:


On Mar 20, 2015, at 2:53 PM, Kumar, Indrajeet <[e-mail]> wrote:

Hi Mike,
   Can you please find someone to take a look at https://bugzilla.mozilla.org/show_bug.cgi?id=1145724
As per the description Mozilla has also seen it earlier and the fix could be as simple as a null check.

This crash has happened 20 times already in our stability testing in just one run so we need to fix it asap.
Current MTBF is 4 and needs to reach 30+ in a week!

Thanks,
-Inder

Comment 5

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

(In reply to Nicholas Troast [:ntroast] from comment #0)

> Related Crash:
> https://crash-stats.mozilla.com/report/index/dd9a84b3-488d-4957-b33c-
> 5f6c22150315

Hmmm, this looks like a Firefox 23 crash on Windows NT and I couldn't find any similar reports. There is a PluginTimerCallBack::Notify call in the stack which I'm not familiar with...

Alex any ideas?

Comment 6

[Michael Vines [:m1] [:evilmachines]]

My drive-by guess is we're getting a nullptr for aContext at https://dxr.mozilla.org/mozilla-central/source/accessible/base/TreeWalker.cpp#24

The segfault in attachment 8580822 [details] is quite close to 0 and the constructor's initialization list dereferences aContext->Document() without a NULL check.

nsAccessibilityService::ContentRemoved() isn't checking for NULL before creating a TreeWalker?

TreeWalker will assert a non-NULL aContext at https://dxr.mozilla.org/mozilla-central/source/accessible/base/TreeWalker.cpp#27, even though it'll never get a chance to make that assertion due to the potential nullptr dereference on line 24, which we have managed to hit?

Comment 7

[Trevor Saunders (:tbsaunde)]

(In reply to Michael Vines [:m1] [:evilmachines] (PTO 3/21-4/4) from comment #6)
> My drive-by guess is we're getting a nullptr for aContext at
> https://dxr.mozilla.org/mozilla-central/source/accessible/base/TreeWalker.
> cpp#24
> 
> The segfault in attachment 8580822 [details] is quite close to 0 and the
> constructor's initialization list dereferences aContext->Document() without
> a NULL check.
> 
> nsAccessibilityService::ContentRemoved() isn't checking for NULL before
> creating a TreeWalker?

I believe this is correct, though I'm not sure if it should be possible for doc->GetAccessibleOrContainer() to return nullptr in this case.

I wish someone would debug why GetAccessibleOrContainer returns null, but I doubt I'm going to get to it so if you want to take a patch for release branches adding a null check I can live with that.

Comment 8

[Michael Vines [:m1] [:evilmachines]]

(In reply to Trevor Saunders (:tbsaunde) from comment #7)
> 
> I wish someone would debug why GetAccessibleOrContainer returns null, but I
> doubt I'm going to get to it so if you want to take a patch for release
> branches adding a null check I can live with that.

We've only been able to reproduce this in monkey testing I believe, so no better STR than run monkey for hours until it crashes.  However we can easily bring in debug patches that add more logging to gather additional data around the event.  Integrating bug 1043112 into our builds could really help here too, but we're not there yet.  So logcat tracing it is for now :(

Yes, a patch with moar null checking would be a big help for the b2g v2.2 release!  Then we can resume the hunt for other crashes that this one may be masking.

Comment 9

[cafbot (PoC: ggrisco)]

Observed on: 

Device: msm8909
Gonk Version: AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.106
Moz BuildID: 20150317002504
Manifest: https://www.codeaurora.org/cgit/quic/lf/b2g/manifest/tree/caf_AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.106.xml?h=release
Gecko Version: 37.0
Gaia:  http://git.mozilla.org/?p=releases/gaia.git;a=commit;h=d0e09d5e6367e558824f9cbf691da99cedf63037
Gecko: http://git.mozilla.org/?p=releases/gecko.git;a=commit;h=bf84ffe0f7b44cc5248a84d1c04ec61d38547df9
Patches: bug 1133147, bug 1139469, bug 1142770, bug 1053634

Comment 10

[cafbot (PoC: ggrisco)]

Attachment: EXTRA file attachment - AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.106

Comment 11

[cafbot (PoC: ggrisco)]

Attachment: decoded minidump - AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.106

Comment 12

[alexander :surkov (:asurkov)]

(In reply to Michael Vines [:m1] [:evilmachines] (PTO 3/21-4/4) from comment #8)
> (In reply to Trevor Saunders (:tbsaunde) from comment #7)
> > 
> > I wish someone would debug why GetAccessibleOrContainer returns null, but I
> > doubt I'm going to get to it so if you want to take a patch for release
> > branches adding a null check I can live with that.
> 
> We've only been able to reproduce this in monkey testing I believe, so no
> better STR than run monkey for hours until it crashes.  However we can
> easily bring in debug patches that add more logging to gather additional
> data around the event. 

can you enable "tree" logging pls? see http://asurkov.blogspot.ca/2012/10/debugging-firefox-accessibility.html

Comment 13

[Greg Grisco]

> can you enable "tree" logging pls? see
> http://asurkov.blogspot.ca/2012/10/debugging-firefox-accessibility.html

Hi Alex, how much extra logging will this generate?  I want to know if it's safe to turn this on in more than just an engineering build.

Comment 14

[alexander :surkov (:asurkov)]

a lot of logging, every content insertion/deletion/some style changes will generate into it, so it'd be good to restrict that output as much as possible

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Why is a11y enabled at all?

Comment 16

[Trevor Saunders (:tbsaunde)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #15)
> Why is a11y enabled at all?

I'm not sure what "monkey testing" involves, but maybe the screen reader got enabled?  but I'm not really sure and it is a good question.

Comment 17

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

I'm reverting this back to ARM/gonk and I think the stack in comment 0 is unrelated.

Comment 18

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

Note I meant unrelated as in not the actual crash instances we're talking about here (since that crash was on windows). The fact that contentremoval is in (all?) the stack(s) may indeed be related.

Comment 19

[Mike Lee [:mlee]]

Nicholas,

Please respond, or have someone at CAF who's able to, respond to Kyle's comment #15:

> Why is a11y enabled at all?


and Trevor's comment #16:

> I'm not sure what "monkey testing" involves, but maybe the screen reader got
> enabled?  but I'm not really sure and it is a good question.


Our Disability Access APIs team (:dbolter) believes a11y should only be enabled by users' specific action via the settings view; cc'ing :eeejay + :yzen for confirmation.

Thanks,
Mike

Comment 20

[Greg Grisco]

If a11y can be turned on through settings, then I think it can turned on by monkey, too.  Monkey is just pressing different areas of the screen at random.

However, I think this particular crash was found by stability test team (through scripting or manual test).  Whether they turned a11y on or not, I'm not sure.  I'll try to find out more.  Hopefully you can carry on with the analysis while I find out, thanks.

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Another interesting question is whether this code works in the presence of web components.

Comment 22

[shawn ku [:sku]]

add ni for myself for tracking.

Comment 23

[Trevor Saunders (:tbsaunde)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #21)
> Another interesting question is whether this code works in the presence of
> web components.

blaim says it comes from bug 1040735 so it should work.  Though clearly there exists some case in which it doesn't.

Comment 24

[Greg Grisco]

The tester that found the issue told me the following:

The accessibility option in settings shows only one menu “color Filter” which is disabled. Still we are able to reproduce the crash.
Below are the steps to reproduce this-
1. Dial a number in dial pad.
2. Long press the green icon used to make a call.
3. You will find phone just crashed pop-up.

Comment 25

[cafbot (PoC: ggrisco)]

Observed on: 

Device: msm8909
Gonk Version: AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.110
Moz BuildID: 20150322002503
Manifest: https://www.codeaurora.org/cgit/quic/lf/b2g/manifest/tree/caf_AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.110.xml?h=release
Gecko Version: 37.0
Gaia:  http://git.mozilla.org/?p=releases/gaia.git;a=commit;h=44c62060581fde8de1e12e94cf55e9673b401a47
Gecko: http://git.mozilla.org/?p=releases/gecko.git;a=commit;h=c36e800ae5e5e7e790129e18edaef202d8d3dbff
Patches: bug 1133147, bug 1139469, bug 1053634, bug 1142770

Comment 26

[cafbot (PoC: ggrisco)]

Attachment: EXTRA file attachment - AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.110

Comment 27

[cafbot (PoC: ggrisco)]

Attachment: decoded minidump - AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.110

Comment 28

[shawn ku [:sku]]

we can not repo. this issue on both ZTE open L and Nexus5-L per comment 24.

Comment 29

[shawn ku [:sku]]

Hi David:
 Could you please also check this issue to see any light to shed?

Comment 30

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

What are the hg changesets (or git revisions) for the source corresponding to the minidumps in comment 2 and comment 27?  It's hard to analyze them with confidence without knowing that.

(Also, I'm in meetings all day today.)

Comment 31

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

Er, I guess comment 25 answers that question for comment 27's minidump (which I didn't look at as closely as comment 2's).

Comment 32

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Greg Grisco from comment #24)
> The tester that found the issue told me the following:
> 
> The accessibility option in settings shows only one menu “color Filter”
> which is disabled. 

AFAIK, if the menu for screen reader is not visible, it has never been activated by the user. The screen reader menu item would remain visible forever if it was enabled at least once, or enabled via the Developer Settings.

> Still we are able to reproduce the crash.
> Below are the steps to reproduce this-
> 1. Dial a number in dial pad.
> 2. Long press the green icon used to make a call.
> 3. You will find phone just crashed pop-up.

Comment 33

[Greg Grisco]

(In reply to Yura Zenevich [:yzen] from comment #32)
> (In reply to Greg Grisco from comment #24)
> 
> AFAIK, if the menu for screen reader is not visible, it has never been
> activated by the user. The screen reader menu item would remain visible
> forever if it was enabled at least once, or enabled via the Developer
> Settings.

So you're saying that it seems inconsistent that with screen reader never being enabled that we're landing in a11y code?  Is there any explanation for this?

Comment 34

[Greg Grisco]

This one crash has been seen so many times that it could block our FC, we need to have some solution for this very soon.

Comment 35

[Mike Lee [:mlee]]

Yura, please respond to Greg's comment #33:

> So you're saying that it seems inconsistent that with screen reader never
> being enabled that we're landing in a11y code?  Is there any explanation for
> this?

As Greg mentions in comment 34, this is a critical crash that we need to resolve and understanding what's happening with a11y seems to be key to achieving that.
 
Thanks,
Mike

Comment 36

[Greg Grisco]

Revisiting comment 6, 7 and 8, should null checking be done?

Comment 37

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Greg Grisco from comment #33)
> (In reply to Yura Zenevich [:yzen] from comment #32)
> > (In reply to Greg Grisco from comment #24)
> > 
> > AFAIK, if the menu for screen reader is not visible, it has never been
> > activated by the user. The screen reader menu item would remain visible
> > forever if it was enabled at least once, or enabled via the Developer
> > Settings.
> 
> So you're saying that it seems inconsistent that with screen reader never
> being enabled that we're landing in a11y code?  Is there any explanation for
> this?

So far the only explanation I have is based on comment 20:

> However, I think this particular crash was found by stability test team
> (through scripting or manual test).  Whether they turned a11y on or not, I'm
> not sure.  I'll try to find out more.  Hopefully you can carry on with the
> analysis while I find out, thanks.

If at any point an integration test was run, accessibility will be turned on.

Comment 38

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1145724 null check patch

Adding a null check patch in the meanwhile.

Comment 39

[Greg Grisco]

bug 1147646 is another a11y crash that was recently found by stress test.  Although, that crash has only been seen once so far.  Referencing it here in case it helps the analysis.

I'm going to apply the null-check patch, thanks.

Comment 40

[Bobby Chien]

Yura, could you help on this bug?

Comment 41

[alexander :surkov (:asurkov)]

Comment on attachment 8583346 [details] [diff] [review]
1145724 null check patch

Review of attachment 8583346 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comment fixed

::: accessible/base/nsAccessibilityService.cpp
@@ +535,5 @@
>      if (!child) {
> +      Accessible* container = document->GetContainerAccessible(aChildNode);
> +      if (container) {
> +        a11y::TreeWalker walker(container, aChildNode,
> +                              a11y::TreeWalker::eWalkCache);

container is not really used when you don't create the tree so you can use 'document' if container is null
nit: please make sure you lined up arguments

Comment 42

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1145724 null check patch v2

Addressed Alex's comments, carrying over r+

Comment 43

[Yura Zenevich [:yzen] (please use "needinfo")]

Ill land the null check patch once the tree is open.

Comment 44

[Yura Zenevich [:yzen] (please use "needinfo")]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f5c7e485c60

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3f5c7e485c60

Comment 46

[bhavana bajaj [:bajaj]]

Comment on attachment 8582242 [details]
decoded minidump - AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.110

[Triage Comment]

Approving the low risk null check

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/4d973ff565c7