Last Comment Bug 753093 - crash in nsAccUtils::HasDefinedARIAToken
: crash in nsAccUtils::HasDefinedARIAToken
Status: RESOLVED FIXED
[native-crash]
: crash, reproducible
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: ARM Android
: -- critical (vote)
: mozilla15
Assigned To: Eitan Isaacson [:eeejay]
:
:
Mentors:
http://jqueryui.com/demos/dialog/defa...
: 753095 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-08 13:51 PDT by Scoobidiver (away)
Modified: 2012-05-15 06:45 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
aContent passed in is null, safeguard (815 bytes, patch)
2012-05-08 21:21 PDT, Marco Zehe (:MarcoZ)
no flags Details | Diff | Splinter Review
Fix crasher when virtual cursor's position becomes defunct and a move method is called. (8.66 KB, patch)
2012-05-09 20:10 PDT, Eitan Isaacson [:eeejay]
no flags Details | Diff | Splinter Review
Fix crasher when virtual cursor's position becomes defunct and a move method is called. (6.62 KB, patch)
2012-05-10 12:29 PDT, Eitan Isaacson [:eeejay]
no flags Details | Diff | Splinter Review
Fix crasher when virtual cursor's position becomes defunct and a move method is called. (11.19 KB, patch)
2012-05-11 11:59 PDT, Eitan Isaacson [:eeejay]
surkov.alexander: review+
Details | Diff | Splinter Review

Description Scoobidiver (away) 2012-05-08 13:51:53 PDT
There are three crashes from the same user.

Signature 	nsAccUtils::HasDefinedARIAToken More Reports Search
UUID	707eeb50-36cc-4931-942d-1a3212120508
Date Processed	2012-05-08 18:15:56
Uptime	238
Last Crash	4.2 minutes before submission
Install Age	30.1 minutes since version was first installed.
Install Time	2012-05-08 17:45:20
Product	FennecAndroid
Version	15.0a1
Build ID	20120508055912
Release Channel	nightly
OS	Linux
OS Version	0.0.0 Linux 2.6.36.4-cyanogenmod+ #1 SMP PREEMPT Fri Apr 6 20:26:41 EDT 2012 armv7l
Build Architecture	arm
Build Architecture Info	
Crash Reason	SIGSEGV
Crash Address	0x0
App Notes 	
AdapterVendorID: p3, AdapterDeviceID: GT-P7500.
AdapterDescription: 'Model: 'GT-P7500', Product: 'GT-P7500', Manufacturer: 'samsung', Hardware: 'p3''.
samsung GT-P7500
samsung/GT-P7500/GT-P7500:3.2/HTJ85B/XWKK4:user/release-keys
EMCheckCompatibility	True

Frame 	Module 	Signature 	Source
0 	libxul.so 	nsAccUtils::HasDefinedARIAToken 	accessible/src/base/nsAccUtils.cpp:221
1 	libxul.so 	nsAccessible::ARIATransformRole 	accessible/src/base/nsAccessible.cpp:1805
2 	libxul.so 	RuleCache::ApplyFilter 	accessible/src/generic/Accessible-inl.h:19
3 	libxul.so 	nsAccessiblePivot::SearchForward 	accessible/src/base/nsAccessiblePivot.cpp:431
4 	libxul.so 	nsAccessiblePivot::MoveNext 	accessible/src/base/nsAccessiblePivot.cpp:221
5 	libxul.so 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/unix/xptcinvoke_arm.cpp:194
6 	libxul.so 	XPCWrappedNative::CallMethod 	js/xpconnect/src/XPCWrappedNative.cpp:3102
7 	libxul.so 	XPC_WN_CallMethod 	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1541
8 	libxul.so 	js::Interpret 	js/src/jscntxtinlines.h:426
9 	libxul.so 	js::RunScript 	js/src/jsinterp.cpp:480
10 	libxul.so 	js::Invoke 	js/src/jsinterp.cpp:540
11 	libxul.so 	js::ProxyHandler::call 	js/src/jsproxy.cpp:340
12 	libxul.so 	js::Wrapper::call 	js/src/jswrapper.cpp:276
13 	libxul.so 	js::CrossCompartmentWrapper::call 	js/src/jswrapper.cpp:758
14 	libxul.so 	proxy_Call 	js/src/jsproxy.cpp:911
15 	libxul.so 	js::Invoke 	js/src/jscntxtinlines.h:426
16 	libxul.so 	JS_CallFunctionValue 	js/src/jsapi.cpp:5429
17 	libxul.so 	nsXPCWrappedJSClass::CallMethod 	js/xpconnect/src/XPCWrappedJSClass.cpp:1509
18 	libxul.so 	nsXPCWrappedJS::CallMethod 	js/xpconnect/src/XPCWrappedJS.cpp:616
19 	libxul.so 	PrepareAndDispatch 	xpcom/reflect/xptcall/src/md/unix/xptcstubs_arm.cpp:138
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=nsAccUtils%3A%3AHasDefinedARIAToken
Comment 1 Marco Zehe (:MarcoZ) 2012-05-08 21:13:05 PDT
Looks like we assert that we're being passed a null argument, but we should also safeguard against it so we don't crash. Patch coming.
Comment 2 Marco Zehe (:MarcoZ) 2012-05-08 21:21:27 PDT
Created attachment 622282 [details] [diff] [review]
aContent passed in is null, safeguard

Looks like aContent that is passed in as mContent from nsAccessible::ARIATransformRole is null. This patch does a safeguard in addition to the already present assertion.
Comment 3 Marco Zehe (:MarcoZ) 2012-05-08 21:27:43 PDT
Comment on attachment 622282 [details] [diff] [review]
aContent passed in is null, safeguard

Obsoleting, since it seems Trevor and Eitan have an idea on how to properly fix this and bug 753095. Should have read all bugmail first. ;)
Comment 4 Eitan Isaacson [:eeejay] 2012-05-09 16:26:53 PDT
*** Bug 753095 has been marked as a duplicate of this bug. ***
Comment 5 Eitan Isaacson [:eeejay] 2012-05-09 16:36:38 PDT
I think I am able to replicate this.

1. Go to: http://jqueryui.com/demos/dialog/default.html
2. Navigate in AccessFu to the close button ("button close")
3. Activate it
4. navigate with AccessFu (forward or back).
5. CRASH! With identical stack trace as above.
Comment 6 Eitan Isaacson [:eeejay] 2012-05-09 20:10:15 PDT
Created attachment 622613 [details] [diff] [review]
Fix crasher when virtual cursor's position becomes defunct and a move method is called.

This fixes the crasher, but does not remedy the larger problem of the vc's position getting lost in the document when the accessible it was on goes away.
Comment 7 Marco Zehe (:MarcoZ) 2012-05-09 20:22:00 PDT
FWIW, this is a problem *all* screen readers face when the web dev didn't provide a new focus position themselves. Normally, best practice is to set focus to a defined position after closing such a modal dialog as in your example. However, most don't do this, so focus is left in limbo land. However, this warrants a separate bug.
Comment 8 alexander :surkov 2012-05-09 23:17:21 PDT
Comment on attachment 622613 [details] [diff] [review]
Fix crasher when virtual cursor's position becomes defunct and a move method is called.

you don't notify observers when pivot is changed that's inconsistency the patch introduces. So since you don't control all pivot instances then I'd suggest to check mPosition->IsDefunct() and if it is true then return failure (if there's no existing suitable failure code then it makes sense to add it). Also you should keep in mind that accessible is defunct after it was destroyed but when accessible is unattached from the tree then pivot movements don't make huge sense. That means it makes sense to replace IsDefunct check on IsInDocument. Also since pivot is bounded by root accessible then you should that check for it. It's also is not covered by your patch and added mochitests.
Comment 9 Eitan Isaacson [:eeejay] 2012-05-09 23:30:32 PDT
(In reply to alexander :surkov from comment #8)
> Comment on attachment 622613 [details] [diff] [review]
> Fix crasher when virtual cursor's position becomes defunct and a move method
> is called.
> 
> you don't notify observers when pivot is changed that's inconsistency the
> patch introduces. So since you don't control all pivot instances then I'd
> suggest to check mPosition->IsDefunct() and if it is true then return
> failure (if there's no existing suitable failure code then it makes sense to
> add it). Also you should keep in mind that accessible is defunct after it
> was destroyed but when accessible is unattached from the tree then pivot
> movements don't make huge sense. That means it makes sense to replace
> IsDefunct check on IsInDocument. Also since pivot is bounded by root
> accessible then you should that check for it. It's also is not covered by
> your patch and added mochitests.

That makes a lot of sense. I'll have a new patch tomorrow.
Comment 10 Eitan Isaacson [:eeejay] 2012-05-09 23:36:58 PDT
(In reply to Marco Zehe (:MarcoZ) from comment #7)
> FWIW, this is a problem *all* screen readers face when the web dev didn't
> provide a new focus position themselves. Normally, best practice is to set
> focus to a defined position after closing such a modal dialog as in your
> example. However, most don't do this, so focus is left in limbo land.
> However, this warrants a separate bug.

The problem with our vc is that it is completely independent of focus and caret position, so even if a web page does it right, we are broken. I think focus/caret should drive the vc and the other way around. So if focus or caret changes, the vc should follow it. If the vc changes focus/caret should follow it.

This will help with this issue, and also potentially help with the different kinds of "skip to content" links.
Comment 11 Eitan Isaacson [:eeejay] 2012-05-10 12:19:23 PDT
(In reply to alexander :surkov from comment #8)
> Comment on attachment 622613 [details] [diff] [review]
> Also since pivot is bounded by root
> accessible then you should that check for it. It's also is not covered by
> your patch and added mochitests.

This is the only bit I don't understand. We already have checks when setting the position, and the move functions can't move the pivot past the root. I agree we should test for this, but how is it related to this bug?
Comment 12 Eitan Isaacson [:eeejay] 2012-05-10 12:29:57 PDT
Created attachment 622849 [details] [diff] [review]
Fix crasher when virtual cursor's position becomes defunct and a move method is called.

This throws NS_ERROR_FAILURE, which seems to me to be fine for now. Where were you proposing a new error constant? nsError.h?
Comment 13 alexander :surkov 2012-05-10 20:04:55 PDT
(In reply to Eitan Isaacson [:eeejay] from comment #11)
> (In reply to alexander :surkov from comment #8)
> > Comment on attachment 622613 [details] [diff] [review]
> > Also since pivot is bounded by root
> > accessible then you should that check for it. It's also is not covered by
> > your patch and added mochitests.
> 
> This is the only bit I don't understand. We already have checks when setting
> the position, and the move functions can't move the pivot past the root. 

so you get pivot and then root accessible goes away (or virtual cursor and the document it's attached to goes away). You do MoveFirst() and it operates on defunct accessible as well.

> I
> agree we should test for this, but how is it related to this bug?

it's part of the same problem.
Comment 14 alexander :surkov 2012-05-10 20:08:30 PDT
(In reply to Eitan Isaacson [:eeejay] from comment #12)

> This throws NS_ERROR_FAILURE, which seems to me to be fine for now. Where
> were you proposing a new error constant? nsError.h?

you could introduce something like NS_ERROR_NOT_IN_TREE similar to what we do http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/a11yGeneric.h#56. You can keep it in nsAccessiblePivot.h I think.
Comment 15 alexander :surkov 2012-05-10 20:16:56 PDT
Comment on attachment 622849 [details] [diff] [review]
Fix crasher when virtual cursor's position becomes defunct and a move method is called.

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

::: accessible/src/base/nsAccessiblePivot.cpp
@@ +222,5 @@
> +    nsRefPtr<nsDocAccessible> document = mPosition->Document();
> +    if (!document || !document->IsInDocument(mPosition))
> +      // Can't move pivot in relation to an invalid point.
> +      return NS_ERROR_FAILURE;
> +  }

I think you can do

if (mPosition && (mPosition->IsDefunct() || !mPosition->Document()->IsInDocument(mPosition))
  return NS_ERROR_NOT_IN_TREE;

IsDefunct is very light since it's inline flag check and false guarantees there's a document

::: accessible/tests/mochitest/pivot.js
@@ +269,5 @@
> + *   set to it.
> + */
> +function removeVirtualCursorPositionInvoker(aDocAcc, aNodeToHide)
> +{
> +  var accessible = getAccessible(aNodeToHide);

nit: usually we keep it as a member since it's used in member functions
Comment 16 Eitan Isaacson [:eeejay] 2012-05-11 11:59:00 PDT
Created attachment 623242 [details] [diff] [review]
Fix crasher when virtual cursor's position becomes defunct and a move method is called.

This adds all of your suggestions plus tests. I only check if the root is valid in move first/last.
Comment 17 alexander :surkov 2012-05-12 07:16:53 PDT
Comment on attachment 623242 [details] [diff] [review]
Fix crasher when virtual cursor's position becomes defunct and a move method is called.

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

::: accessible/src/base/nsAccessiblePivot.cpp
@@ +244,5 @@
> +  if (mPosition &&
> +      (mPosition->IsDefunct() ||
> +       !mPosition->Document()->IsInDocument(mPosition)))
> +    // Can't move pivot in relation to an invalid point.
> +    return NS_ERROR_NOT_IN_TREE;

I don't think we really need a comment here. And actually I'd use the style I suggested
if (mPosition && (mPosition->IsDefunct() ||
    !mPosition->Document()->IsInDocument(mPosition)))
  return NS_ERROR_NOT_IN_TREE;

@@ +265,5 @@
>    NS_ENSURE_ARG(aRule);
> +
> +  if (!mRoot || mRoot->IsDefunct())
> +    // Can't move pivot in relation to invalid root.
> +    return NS_ERROR_NO_ROOT;

it's overkill, NS_ERROR_NOT_IN_TREE is enough. When you can't move the first it can mean only one thing mRoot is not in tree any more. It makes sense to check IsInDocument also I think

::: accessible/tests/mochitest/pivot.js
@@ +248,5 @@
>  
>  /**
> + * A checker for removing an accessible while the virtual cursor is on it.
> + */
> +function removeVirtualCursorPositionChecker(aDocAcc, aHiddenParentAcc)

nit: they are so long, maybe you could use VC instead VirtualCursor?

@@ +261,5 @@
> +      errorResult = x.result;
> +    }
> +    SimpleTest.is(
> +      errorResult, NS_ERROR_NOT_IN_TREE,
> +      "Expecting NOT_IN_TREE error when moving pivot from invalid position.");

iirc we talked about that but I don't recall why this mochitest is special so it requires SimpleTest.is

@@ +270,5 @@
> + * Put the virtual cursor's position on an object, and then remove it.
> + *
> + * @param aDocAcc document that manages the virtual cursor
> + * @param aNodeToHide DOM node to hide after virtual cursor's position is
> + *   set to it.

in mochitests we use the following style
@param aDocAcc     [in] the document that manages the virtual cursor
@param aNodeToHide [in]

btw, I'd name it aPositionAcc, anyway 'remove' is more correct than 'hide'

@@ +283,5 @@
> +  };
> +
> +  this.getID = function removeVirtualCursorPositionInvoker_getID()
> +  {
> +    return "Bring virtual cursor to accessible, and remove its DOM node.";

maybe "Bring virtual cursor to the accessible and then make it go away" or destroy

@@ +292,5 @@
> +  ];
> +}
> +
> +/**
> + * A checker for removing the pivot root and then calling moveFirst.

and then 'checking' perhaps?

@@ +313,5 @@
> +}
> +
> +/**
> + * Create a pivot, remove its root, and perform an operation where the root is
> + *   needed.

you don't need extra indentation for second line

@@ +316,5 @@
> + * Create a pivot, remove its root, and perform an operation where the root is
> + *   needed.
> + *
> + * @param aRootNode node of which accessible will be the root of the pivot.
> + *   Should have more than one children.

maybe: the pivot root accessible having children.

::: accessible/tests/mochitest/pivot/doc_virtualcursor.html
@@ +17,5 @@
>    <iframe
>       src="data:text/html,<html><body>An <i>embedded</i> document.</body></html>">
>    </iframe>
> +  <div id="hide-me">Hide me</div>
> +<p id="links">

please fix indentation
Comment 18 Eitan Isaacson [:eeejay] 2012-05-14 12:30:22 PDT
(In reply to alexander :surkov from comment #17)
> Comment on attachment 623242 [details] [diff] [review]
> Fix crasher when virtual cursor's position becomes defunct and a move method
> is called.
> 
> Review of attachment 623242 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/base/nsAccessiblePivot.cpp
> @@ +265,5 @@
> >    NS_ENSURE_ARG(aRule);
> > +
> > +  if (!mRoot || mRoot->IsDefunct())
> > +    // Can't move pivot in relation to invalid root.
> > +    return NS_ERROR_NO_ROOT;
> 
> it's overkill, NS_ERROR_NOT_IN_TREE is enough. When you can't move the first
> it can mean only one thing mRoot is not in tree any more. It makes sense to
> check IsInDocument also I think
> 

I don't know if it is a bug or not, but IsInDocument returns false on the document accessible itself. I guess you could get the parent doc, and check against it. But for some reason I can't get that to work either.

Anyway, if the root node is not defunct, but not in a document - the subtree should still be intact, and traversing it should not be bad (maybe useless, but not bad).

I'm going to leave out IsInDocument for now, and land this fix since it is critical. If you think this is a really bad idea, please open a new bug and assign it to me.
Comment 20 alexander :surkov 2012-05-15 00:48:54 PDT
(In reply to Eitan Isaacson [:eeejay] from comment #18)

> I don't know if it is a bug or not, but IsInDocument returns false on the
> document accessible itself. I guess you could get the parent doc, and check
> against it. But for some reason I can't get that to work either.

either you should have those check or do not have. I don't think there's anything bad to navigate through unattached tree, it's just doesn't make sense and could be confusing. But you should keep in mind since pivot can cross document boundaries then you fall into the same case with MoveNext and MovePrevious (you just don't have a proper mochitest for that).

Another issue is when the subtree (the pivot is inside of) goes away then all you can do is to start from the start or from the end. This behavior might be confusing for the user. A simple example showing that, say you navigate through a tree and you're on the tree item twister (an image), you click on it, they change the image of the twister, the image accessible goes away and new one is created and the virtual cursor position is undefined (all you can do is to move it on top or bottom of the document). I think you should spot real examples in the wild.
Comment 21 Ed Morley [:emorley] 2012-05-15 06:45:52 PDT
https://hg.mozilla.org/mozilla-central/rev/58991e7a4aaf

Note You need to log in before you can comment on or make changes to this bug.