1137906 - [Accessibility] crash during FxA login with screen reader on - mozilla::a11y::xpcAccessible::GetState(unsigned int*, unsigned int*)

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Yeojin Chung [:YeojinC]]

Attachment: logcat_20150227_CrashWithScreenReader.txt

This bug was filed from the Socorro interface and is 
report bp-9bd0c138-b20b-4e2e-aea3-10e632150227.
=============================================================
When the user tries to enter password for FFOX account with screen reader on.

Repro Steps:
1) Update a Flame to 20150227010229.
2) Enable Screen Reader on Settings > Accessibility.
3) Go to Settings > Firefox Accounts > Create Account or Sign In.
4) Enter an email address, and select Next.
5) Tap on the password input field to enter password.

Actual:
Keyboard never shows up, and the crash happens.

Expected:
The keyboard pops up, so the user can enter the password.

Environmental Variables:
Device: Flame 3.0 (KK, 319mb, full flash)
Build ID: 20150227010229
Gaia: 7512026a377271a0cade12d70846557f0bc7781c
Gecko: c7968255c1ea
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 39.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

Repro frequency: 5/5
See attached: logcat

Comment 1

[Yeojin Chung [:YeojinC]]

This issue also reproduces on Flame 2.2.

Result: The crash occurs. 

Device: Flame 2.2 (KK, 319mb, full flash)
Build ID: 20150227002521
Gaia: eb6a5ac9081d3962198e0f4520b0743d716d7a27
Gecko: c8a38dcfbebc
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0 (2.2)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
==========================================

This issue is unable to reproduce on Flame 2.1. The user is unable to type in the email field at all when trying to log in with FxA.

Device: Flame 2.1  (KK, 319mb, full flash)
Build ID: 20150227002636
Gaia: 5d3479fdd438412adee4452720856b6b771fe5cd
Gecko: 0390c73a827b
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 34.0 (2.1)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.

Comment 2

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Crash stack shows:

Frame 	Module 	Signature 	Source
0 	libxul.so 	mozilla::a11y::xpcAccessible::GetState(unsigned int*, unsigned int*) 	accessible/xpcom/xpcAccessible.cpp
1 	libxul.so 	NS_InvokeByIndex 	xpcom/reflect/xptcall/md/unix/xptcinvoke_arm.cpp
2 	libxul.so 	XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) 	js/xpconnect/src/XPCWrappedNative.cpp
3 	libxul.so 	XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) 	js/xpconnect/src/XPCWrappedNativeJSOps.cpp
4 	libxul.so 	js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) 	js/src/jscntxtinlines.h
5 	libxul.so 	js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) 	js/src/vm/Interpreter.cpp
6 	libxul.so 	js::jit::DoCallFallback 	js/src/jit/BaselineIC.cpp
7 		@0xb29f57e2

Comment 3

[Yeojin Chung [:YeojinC]]

Here's a video for the crash: http://youtu.be/QyhbPECMkDg

Comment 4

[alexander :surkov (:asurkov)]

Yura, can you take a look please?

Comment 5

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: crash. let's block.

Comment 6

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Ting-Yu, you may want to take a look at this.

Comment 7

[Ting-Yu Chou [:ting] (away)]

I guess you're looking for :TYLin?

Comment 8

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Sorry that I am unfamiliar with the Accessibility module.

Comment 9

[alexander :surkov (:asurkov)]

see another str from bug 1096930 comment #4

Comment 10

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Spoke with :ting and he'll take a look first.

Comment 11

[howie [:howie]]

ni :ting for notice, thanks.

Comment 12

[Ting-Yu Chou [:ting] (away)]

I see only "Color Filter" in Accessibility menu, are there anything else I should set?

Comment 13

[Ting-Yu Chou [:ting] (away)]

(In reply to Ting-Yu Chou [:ting] from comment #12)
> I see only "Color Filter" in Accessibility menu, are there anything else I
> should set?

Just learnt from Arthur I need to enable that from developer menu. But I don't know how to control the device then...

Comment 14

[Ting-Yu Chou [:ting] (away)]

(In reply to Ting-Yu Chou [:ting] from comment #13)
> Just learnt from Arthur I need to enable that from developer menu. But I
> don't know how to control the device then...

Learnt how to control it, will try to reproduce tomorrow.

Comment 15

[Ting-Yu Chou [:ting] (away)]

GetState() is called on a xpcAccessibleHyperText belongs to the xpcAccessibleDocument of keyboard.html for entering account, which has been shutdown already.

I tried to get JS stack, but calling DumpJSStack() prints nothings and cx->currentScript(0x0, 1)->filename() makes the process exited.

Comment 16

[Ting-Yu Chou [:ting] (away)]

(In reply to Ting-Yu Chou [:ting] from comment #15)
> GetState() is called on a xpcAccessibleHyperText belongs to the
> xpcAccessibleDocument of keyboard.html for entering account, which has been
> shutdown already.

Correction: index.html of keyboard app

Comment 17

[Ting-Yu Chou [:ting] (away)]

(In reply to Ting-Yu Chou [:ting] from comment #15)
> I tried to get JS stack, but calling DumpJSStack() prints nothings and
> cx->currentScript(0x0, 1)->filename() makes the process exited.

The JS stack:

03-06 04:13:37.510 31020 31020 I Gecko   : Utils.jsm | getState getState@resource://gre/modules/accessibility/Utils.jsm:272:15
03-06 04:13:37.510 31020 31020 I Gecko   : isAliveAndVisible@resource://gre/modules/accessibility/Utils.jsm:400:19
03-06 04:13:37.510 31020 31020 I Gecko   : VisualPresenter_viewportChanged@resource://gre/modules/accessibility/Presentation.jsm:168:9
03-06 04:13:37.510 31020 31020 I Gecko   : Presentation_viewportChanged@resource://gre/modules/accessibility/Presentation.jsm:725:13
03-06 04:13:37.510 31020 31020 I Gecko   : handleEvent@resource://gre/modules/accessibility/EventManager.jsm:122:22
03-06 04:13:37.510 31020 31020 D x       : 0xab607428 xpcAccessible::GetState 0xafe633c8

Comment 18

[Ting-Yu Chou [:ting] (away)]

The issue can be reproduced when you activate the keyboard at 2nd time, you don't need to go through the Fx account settings.

Comment 19

[Ting-Yu Chou [:ting] (away)]

A more accurate STR:

1. Enable screen reader
2. Activate keyboard
3. Press any key to make it focused (border wrapped)
4. Deactivate keyboard
5. Activate keyboard

My current understanding is: When deactivate keyboard, the corresponding xpcAccessibleyDocument is shutdown and its cached accessibles are cleared, but VisualPresenter still keeps the accessible of the last pressed key in _displayedAccessibles. Next time when keyboard is activated, a resize event comes and VisualPresenter_viewportChanged() will try to get the state of the dead accessible.

Comment 20

[Ting-Yu Chou [:ting] (away)]

Attachment: patch v1

Comment 21

[alexander :surkov (:asurkov)]

Comment on attachment 8574542 [details] [diff] [review]
patch v1

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

thanks for fixing this, r=me

::: accessible/xpcom/xpcAccessibleDocument.cpp
@@ +199,5 @@
> +ShutdownEnumerator(const Accessible* aKey, xpcAccessibleGeneric* aValue,
> +                   void* aUnused)
> +{
> +  aValue->Shutdown();
> +  return PL_DHASH_NEXT;

not sure which way is more perfromant: iterate all and then remove all or iterate and then remove by one but please make sure to consider this

Comment 22

[Trevor Saunders (:tbsaunde)]

> ::: accessible/xpcom/xpcAccessibleDocument.cpp
> @@ +199,5 @@
> > +ShutdownEnumerator(const Accessible* aKey, xpcAccessibleGeneric* aValue,
> > +                   void* aUnused)
> > +{
> > +  aValue->Shutdown();
> > +  return PL_DHASH_NEXT;
> 
> not sure which way is more perfromant: iterate all and then remove all or
> iterate and then remove by one but please make sure to consider this

I'd be suprised if it wasn't faster to do the removals in the same pass.

Comment 23

[Ting-Yu Chou [:ting] (away)]

Attachment: patch v2

You're right, clear() actually walks through the entries. Change to shutdown and remove in one pass.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=be79084d8473

Comment 24

[Ting-Yu Chou [:ting] (away)]

Another try with bug 1123762 backout: https://treeherder.mozilla.org/#/jobs?repo=try&revision=950c5ed59ae1

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/048ac1585bb6

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/048ac1585bb6

Comment 27

[Ting-Yu Chou [:ting] (away)]

Comment on attachment 8575053 [details] [diff] [review]
patch v2

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): seems it's there since the code was created
User impact if declined: crash when activate keyboard 2nd time
Testing completed: crashtest/mochitest on windows, linux and b2g emulator
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: n/a

Comment 28

[Peter Bylenga [:PBylenga]]

Adding qawanted to verify this on nightly mozilla-central.

Comment 29

[Yeojin Chung [:YeojinC]]

This crash no longer occurs on Flame Master. However, I had another crash while following the STR for this crash, which is filed as bug 1142747, and a graphic issue after logging in, bug 1142764.

Device: Flame 3.0 (KK, 319mb, full flash)
Build ID: 20150312010235
Gaia: 0c4e8b0b330757e261b031b7e7f326ef419c9808
Gecko: 5334d2bead3e
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
Version: 39.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0
=================================

Leaving verifyme for 2.2 uplift & verification.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/57c432ab9172