Nicholas Troast [:ntroast] Reporter
	Description • 9 years ago

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

	Mike Lee [:mlee]
	Comment 3 • 9 years ago

Hi David,

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

Thanks!
Mike

	Mike Lee [:mlee]
	Comment 4 • 9 years ago

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


On Mar 20, 2015, at 2:53 PM, Kumar, Indrajeet <ikumar@codeaurora.org> 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

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 5 • 9 years ago

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

	Michael Vines [:m1] [:evilmachines]
	Comment 6 • 9 years ago

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?

	Trevor Saunders (:tbsaunde)
	Comment 7 • 9 years ago

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

	Michael Vines [:m1] [:evilmachines]
	Comment 8 • 9 years ago

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

	cafbot (PoC: ggrisco)
	Comment 9 • 9 years ago

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

	alexander :surkov (:asurkov)
	Comment 12 • 9 years ago

(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

	Greg Grisco
	Comment 13 • 9 years ago

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

	alexander :surkov (:asurkov)
	Comment 14 • 9 years ago

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

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 15 • 9 years ago

Why is a11y enabled at all?

	Trevor Saunders (:tbsaunde)
	Comment 16 • 9 years ago

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 17 • 9 years ago

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

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 18 • 9 years ago

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.

	Mike Lee [:mlee]
	Comment 19 • 9 years ago

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

	Greg Grisco
	Comment 20 • 9 years ago

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.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 21 • 9 years ago

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

	shawn ku [:sku]
	Comment 22 • 9 years ago

add ni for myself for tracking.

	Trevor Saunders (:tbsaunde)
	Comment 23 • 9 years ago

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.

	Greg Grisco
	Comment 24 • 9 years ago

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.

	cafbot (PoC: ggrisco)
	Comment 25 • 9 years ago

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

	shawn ku [:sku]
	Comment 28 • 9 years ago

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

	shawn ku [:sku]
	Comment 29 • 9 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 30 • 9 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 31 • 9 years ago

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

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 32 • 9 years ago

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

	Greg Grisco
	Comment 33 • 9 years ago

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

	Greg Grisco
	Comment 34 • 9 years ago

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.

	Mike Lee [:mlee]
	Comment 35 • 9 years ago

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

	Greg Grisco
	Comment 36 • 9 years ago

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

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 37 • 9 years ago

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

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 38 • 9 years ago

Adding a null check patch in the meanwhile.

	Greg Grisco
	Comment 39 • 9 years ago

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.

	Bobby Chien
	Comment 40 • 9 years ago

Yura, could you help on this bug?

	alexander :surkov (:asurkov)
	Comment 41 • 9 years ago

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

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 42 • 9 years ago

Addressed Alex's comments, carrying over r+

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 43 • 9 years ago

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

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 44 • 9 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 45 • 9 years ago

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

	bhavana bajaj [:bajaj]
	Comment 46 • 9 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 47 • 9 years ago

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