659620 - Hang when trying to edit a page on WikiMo with NVDA running

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

This is a very recent regression:
1. Start NVDA and a current nightly.
2. Go to any page on https://wiki.mozilla.org
3. Click on "Edit this page".
4. The new page loads and should put NVDA into focus mode automatically, focused on the textarea that contains the page text.

Expected: NVDA should allow you to edit the page.
Actual: A hang. Firefox does not crash, but NVDA and Firefox become unresponsive. The only solution is to run Task manager and kill the Firefox.exe process. That will restore NVDA to normal operation.

Comment 1

[Marco Zehe (:MarcoZ)]

Regression window:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=21c304c5f351&tochange=52f72d71dc59

There were 4 bugs in ourmodule that got checked in on that day:
bug 657296
bug 654999
bug 653584
and
bug 658737

Comment 2

[Marco Zehe (:MarcoZ)]

BTW a link that reproduces this for me is
https://wiki.mozilla.org/index.php?title=Accessibility/Meetings/2011-05-25&action=edit

That page currently doesn't exist, and it throws me into that hang immediately.

Comment 3

[alexander :surkov (:asurkov)]

Firefox doesn't hung, UI works, I can operate on accessible tree, but NVDA keeps silent.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #1)
> Regression window:
> http://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=21c304c5f351&tochange=52f72d71dc59
> 
> There were 4 bugs in ourmodule that got checked in on that day:
> bug 657296
> bug 654999
> bug 653584
> and
> bug 658737

if one of these is guilty then it's bug 653584. Marco, can you try to backout it locally and try?

Comment 5

[alexander :surkov (:asurkov)]

btw, bug 653584 can hang, for example, TakeFocus(), since mAction can be this and infinite recursion happens. Iirc mAction accessible was never 'this' prior this patch. Trevor, can you fix that?

Comment 6

[Trevor Saunders (:tbsaunde)]

Attachment: test patch

make nsLinkableAccessible::BindToParent() never set mActionAcc = this;

Comment 7

[Marco Zehe (:MarcoZ)]

Comment on attachment 535064 [details] [diff] [review]
test patch

This patch definitely fixes the hang! Verified locally by building with and without it.

Comment 8

[Trevor Saunders (:tbsaunde)]

Comment on attachment 535064 [details] [diff] [review]
test patch

BEFORE BUG 653584 WE STORED THE ACTION CONTENT AND GOT THE ACCESSIBLE FOR IT WHEN WE NEEDED IT, AND IF THE lINKABLEaCCESSIBLE HAD AN ONCLICK LISTENER ITSELF WE'D SET MaCTIONcONTENT = McONTENT, BUT gETaCTIONaCCESSIBLE() WOULD RETURN NULL IF MaCTIONcONTENT == McONTENT.  sO SINCE WE NOLONGER USE gETaCTIONaCCESSIBLE() WE SHOULD MAKE MaCTIONaCC BE NULL IF THE ACTIONABLE ACCESSIBLE IS THIS SO THAT THE TESTS IF WE SHOULD REDIRECT THE METHOD CALL TO A DIFFERENT ACCESSIBLE STILL DO THE CORRECT THING, NOT INFINETELY RECURSE

Comment 9

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

Comment on attachment 535064 [details] [diff] [review]
test patch

r=me. Thanks for the explanation.

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 535064 [details] [diff] [review]
test patch


>   // Cache action content.
>   mActionAcc = nsnull;
>   mIsLink = PR_FALSE;
>   mIsOnclick = PR_FALSE;
> 
>   if (nsCoreUtils::HasClickListener(mContent)) {
>-    mActionAcc = this;
>+    mActionAcc = nsnull;

this is crazy, do not land while it's not fixed please

Comment 11

[alexander :surkov (:asurkov)]

Also, please add mochitests for this, we shouldn't hit this problem twice.

Comment 12

[Trevor Saunders (:tbsaunde)]

> this is crazy, do not land while it's not fixed please

ok, your in time, but I'm confused about what you meant by 
 
(In reply to comment #5)
> btw, bug 653584 can hang, for example, TakeFocus(), since mAction can be
> this and infinite recursion happens. Iirc mAction accessible was never
> 'this' prior this patch. Trevor, can you fix that?

I would agree GetActionAccessible() would never return  this, so what do you want mActionAcc to be if not nsnull?

Comment 13

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

(In reply to comment #10)
> Comment on attachment 535064 [details] [diff] [review] [review]
> test patch
> 
> 
> >   // Cache action content.
> >   mActionAcc = nsnull;
> >   mIsLink = PR_FALSE;
> >   mIsOnclick = PR_FALSE;
> > 
> >   if (nsCoreUtils::HasClickListener(mContent)) {
> >-    mActionAcc = this;
> >+    mActionAcc = nsnull;
> 
> this is crazy, do not land while it's not fixed please

Do you mean Trevor might as well remove the assignment since mActionAcc is already nsnull? Or is it crazy in some other way?

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 535064 [details] [diff] [review]
test patch

this breaks the action number, be careful when you do not 1 to 1 rollback

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #13)
> (In reply to comment #10)
> > Comment on attachment 535064 [details] [diff] [review] [review] [review]
> > test patch
> > 
> > 
> > >   // Cache action content.
> > >   mActionAcc = nsnull;
> > >   mIsLink = PR_FALSE;
> > >   mIsOnclick = PR_FALSE;
> > > 
> > >   if (nsCoreUtils::HasClickListener(mContent)) {
> > >-    mActionAcc = this;
> > >+    mActionAcc = nsnull;
> > 
> > this is crazy, do not land while it's not fixed please
> 
> Do you mean Trevor might as well remove the assignment since mActionAcc is
> already nsnull? Or is it crazy in some other way?

right, that was a point

Comment 16

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

(In reply to comment #14)
> Comment on attachment 535064 [details] [diff] [review] [review]
> test patch
> 
> this breaks the action number, be careful when you do not 1 to 1 rollback

Oof! Definitely a good reason to ensure test coverage here.

Comment 17

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

Comment on attachment 535064 [details] [diff] [review]
test patch

r-=me based on new data (nit, existing test failure, and Alex's catch).

Comment 18

[Trevor Saunders (:tbsaunde)]

Attachment: (wip) better fix tests are broken

for ome reason getElementById() doesn't seem to be working

Comment 19

[Marco Zehe (:MarcoZ)]

Comment on attachment 535271 [details] [diff] [review]
(wip) better fix tests are broken

>+var acc = document.getElementById("someclickable");

This doesn't give you an accessible, just a dom eelement. Just do 

var acc = getAccessible("someclickable");

This will also do all handling for you.

You can also add this to the array above these new lines, and do regular action testing. Therefore, you would only get this acc for the takeFocus(); test.

>+      is(acc, "image with onclick handler should have 1 action but found " + acc.numActions);

You're missing the expected value here, is() takes 3 parameters. AND it must be acc.numActions. But again, you can ommit this by simply adding this element to the array above this new stuff and let the already existing action testing logic do its magic.

Comment 20

[Trevor Saunders (:tbsaunde)]

Attachment: (next patch

use the actions table for everything but takeFocus() the tests mostly pass, but one dies with a syntax error even though I can't find anything that looks close to a syntax error in the file

Comment 22

[Trevor Saunders (:tbsaunde)]

Attachment: patch

correct tests html

Comment 23

[alexander :surkov (:asurkov)]

(In reply to comment #22)
> Created attachment 535295 [details] [diff] [review] [review]
> patch
> 
> correct tests html

GetNumActions() should be broken for links still. Please make sure you have mochitest for this case too.

Comment 24

[alexander :surkov (:asurkov)]

Comment on attachment 535295 [details] [diff] [review]
patch

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

r=me, thanks!

::: accessible/tests/mochitest/actions/test_general.html
@@ +47,3 @@
>        testActions(actionsArray);
> +
> +      //getAccessible("onclick_img").takeFocus();

artifact?

@@ +67,5 @@
>      Mozilla Bug 423409
>    </a>
> +  <a target="_blank"
> +     href="https://bugzilla.mozilla.org/show_bug.cgi?id=659620"
> +     title="hand when trying to edit a page on wikimo withnvda running">

hand -> Hang, withnvda -> with NVDA

@@ +81,5 @@
>      <li id="li_clickable2" onmousedown="">Clickable list item</li>
>      <li id="li_clickable3" onmouseup="">Clickable list item</li>
>    </ul>
> +
> +  <img id="onclick_img" onclick="" src="../moz.png">

maybe comment like <!-- linkable accessibles -->

it makes sense to add tests for
<a href="www">linkable textleaf accessible</a>
<div onclick="">linkable textleaf accessible</div>

Comment 25

[Trevor Saunders (:tbsaunde)]

Attachment: patch

address comments on tests

Comment 26

[Trevor Saunders (:tbsaunde)]

Attachment: patch to land

correct patch actually includes changes addressing comments this time.

Comment 27

[alexander :surkov (:asurkov)]

Comment on attachment 535493 [details] [diff] [review]
patch to land


>+
>+      getAccessible("onclick_img").takeFocus();

why do you need that?

>+is(getAccessible("link1").numActions, 1, "links should have one action");
>+is(getAccessible("link2").numActions, 1, "link with onclick handler should have 1 action");

wrong identation. Why you don't use action testing functions like above and prefer to test numActions only?

also I asked to add tests for linkable accessible (text leaf inside accessible with actions), you didn't.

Comment 28

[Trevor Saunders (:tbsaunde)]

(In reply to comment #27)
> Comment on attachment 535493 [details] [diff] [review] [review]
> patch to land
> 
> 
> >+
> >+      getAccessible("onclick_img").takeFocus();
> 
> why do you need that?

I thought we want to exercise that there wasn't infinite recursion in takeFocus()? I'm not sure we need it, but I don't see how it hurts.  I gues I could remove it if you like, I thought you were asking if the commenting out  should go away not the whole line.

> >+is(getAccessible("link1").numActions, 1, "links should have one action");
> >+is(getAccessible("link2").numActions, 1, "link with onclick handler should have 1 action");
> 
> wrong identation. Why you don't use action testing functions like above and

ok

> prefer to test numActions only?

well, I don't see how tests for name and  the actionactually hapening can hurt.  

> also I asked to add tests for linkable accessible (text leaf inside
> accessible with actions), you didn't.

ok,  I saw 

> it makes sense to add tests for
> <a href="www">linkable textleaf accessible</a>
> <div onclick="">linkable textleaf accessible</div>

did youask somewhere else then?  do you mean something like <a href="www"><p>test</p></a>?

Comment 29

[alexander :surkov (:asurkov)]

(In reply to comment #28)

> > >+      getAccessible("onclick_img").takeFocus();
> > 
> > why do you need that?
> 
> I thought we want to exercise that there wasn't infinite recursion in
> takeFocus()? I'm not sure we need it, but I don't see how it hurts.  I gues
> I could remove it if you like, I thought you were asking if the commenting
> out  should go away not the whole line.

then put this test into test_takeFocus.html, because it doesn't look as a test.

> > >+is(getAccessible("link1").numActions, 1, "links should have one action");
> > >+is(getAccessible("link2").numActions, 1, "link with onclick handler should have 1 action");

> > prefer to test numActions only?
> 
> well, I don't see how tests for name and  the actionactually hapening can
> hurt.  

I prefer complex tests in this case. Event if these elements have something common then it makes sense to double test it since these elements have a difference. We don't know whether the ratio of common to difference is permanent.

> > also I asked to add tests for linkable accessible (text leaf inside
> > accessible with actions), you didn't.
> 
> ok,  I saw 
> 
> > it makes sense to add tests for
> > <a href="www">linkable textleaf accessible</a>
> > <div onclick="">linkable textleaf accessible</div>
> 
> did youask somewhere else then?  do you mean something like <a
> href="www"><p>test</p></a>?

the hierarchy is:
  link
    text leaf (linkable accessible)

it's not necessary to put paragraph inside.

Comment 30

[Marco Zehe (:MarcoZ)]

Can we move on on this one? This is needed for Aurora since the hang is in there, too.

Comment 31

[Asa Dotzler [:asa]]

Please request approval to land on Aurora for 6 with a risk analysis. Thanks.

Comment 32

[Trevor Saunders (:tbsaunde)]

Attachment: patch

test linkable accessibles more.  These seem to have passed atleast once for me, but seem to hang other times.

Comment 33

[Marco Zehe (:MarcoZ)]

Comment on attachment 536682 [details] [diff] [review]
patch

>+      getAccessible("onclick_img").takeFocus();
>+

Hm, this will not work because the image itself is not focusable, not even through an onclick handler. It has to have tabindex="0" set.

And then, you should simply do:
       gQueue.push(new takeFocusInvoker("onclick_img"));

before the call to gQueue.invoke();

Comment 34

[Trevor Saunders (:tbsaunde)]

Attachment: test cleanup

Alex hopefully this is closer to what you were looking for the first time around.

Comment 35

[Trevor Saunders (:tbsaunde)]

http://hg.mozilla.org/mozilla-central/rev/3b17ef9c3cd8
the acutal fix, and some initial tests landed in  but it seems I never acutally posted the link :(

Comment 36

[alexander :surkov (:asurkov)]

Comment on attachment 537703 [details] [diff] [review]
test cleanup

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

::: accessible/tests/mochitest/actions/test_general.html
@@ +43,3 @@
>            actionName: "click",
> +          events: CLICK_EVENTS,
> +          numActions: 1

you didn't add a support for numActions property, right?

@@ +67,5 @@
> +is(textChild1.getActionName(0), "jump", "link should have jump action");
> +
> +      var textChild2 = getAccessible("link2").getChildAt(0);
> +      is(textChild2.numActions, 1, "text child accessible of accessible with onclick handler should have 1 action");
> +      is(textChild2.getActionName(0), "click", "text child accessible of an object with an onclick handler should have an onclick handler");

instead all of these, I would allow the usage of objects as "ID" property.

@@ +104,5 @@
>      <li id="li_clickable3" onmouseup="">Clickable list item</li>
>    </ul>
>  
>    <!-- linkable accessibles -->
> +  <img id="onclickImg" onclick="" src="..moz.png">

what's about src change?

btw, I don't get the point of id change.

::: accessible/tests/mochitest/test_takeFocus.html
@@ +52,5 @@
>  
>        gQueue.push(new takeFocusInvoker("aria-link"));
>        gQueue.push(new takeFocusInvoker("aria-link2"));
>        gQueue.push(new takeFocusInvoker("link"));
> +gQueue.push(new takeFocusInvoker("onclick_img"));

fix identation please

@@ +75,2 @@
>    <pre id="test">
>    </pre>

add bug link please

Comment 37

[Trevor Saunders (:tbsaunde)]

Attachment: updated patch address comments

Comment 38

[alexander :surkov (:asurkov)]

Comment on attachment 538167 [details] [diff] [review]
updated patch address comments

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

r=me with nits fixed

::: accessible/tests/mochitest/actions.js
@@ +55,5 @@
>      var actionObj = aArray[idx];
>      var accOrElmOrID = actionObj.ID;
>      var actionIndex = actionObj.actionIndex;
>      var actionName = actionObj.actionName;
> +    var actions = "numActions" in actionObj ? actionObj.numActions : -1;

name them actionCount, i.e.

var actionCount = "actionCount" in actionObj ? actionObj.numActions : -1;

since I think eventually nsIAccessible::numAction will be renamed to actionCount

@@ +93,5 @@
>  
>  var gActionsQueue = null;
>  
> +function actionInvoker(aAccOrElmOrId, aActionIndex, aActionName, aActions,
> +                       aEventSeq)

please pass actionCount as a second argument

@@ +109,5 @@
> +      var isThereActions = acc.numActions > 0;
> +      ok(isThereActions,
> +         "No actions on the accessible for " + prettyName(aAccOrElmOrId));
> +      if (!isThereActions)
> +        return INVOKER_ACTION_FAILED;

add an empty line between ok() and if statement

@@ +143,5 @@
>    this.phase = false;
>  
>    this.getID = function getID()
>    {
> +    return aType + " event handling on " + aTarget.id;

ok, fine with me, though probably it should be handled in event.js, i.e. getID() from checker and invoker should be combined.

Comment 39

[Trevor Saunders (:tbsaunde)]

http://hg.mozilla.org/mozilla-central/rev/3b17ef9c3cd8
the acutal fix, and some initial tests landed in  but it seems I never acutally posted the link :((In reply to comment #38)
> Comment on attachment 538167 [details] [diff] [review] [review]

> 
> var actionCount = "actionCount" in actionObj ? actionObj.numActions : -1;
> 
> since I think eventually nsIAccessible::numAction will be renamed to
> actionCount

mind if I do that and dexpcom it as a low priority thing?

> 
> @@ +93,5 @@
> >  
> >  var gActionsQueue = null;
> >  
> > +function actionInvoker(aAccOrElmOrId, aActionIndex, aActionName, aActions,
> > +                       aEventSeq)
> 
> please pass actionCount as a second argument

you mean the second argument right?

> >    this.getID = function getID()
> >    {
> > +    return aType + " event handling on " + aTarget.id;
> 
> ok, fine with me, though probably it should be handled in event.js, i.e.
> getID() from checker and invoker should be combined.

That seems like a reasonable clean up to me, but my interest in cleaning up js right now is pretty limited, and I'd rather not block this bug on any more refactoring.

Comment 40

[alexander :surkov (:asurkov)]

(In reply to comment #39)

> > since I think eventually nsIAccessible::numAction will be renamed to
> > actionCount
> 
> mind if I do that and dexpcom it as a low priority thing?

sure I don't mind, though honestly I don't understand why you ask this if you meant interface changes.

> > please pass actionCount as a second argument

> you mean the second argument right?

right.

> > ok, fine with me, though probably it should be handled in event.js, i.e.
> > getID() from checker and invoker should be combined.
> 
> That seems like a reasonable clean up to me, but my interest in cleaning up
> js right now is pretty limited, and I'd rather not block this bug on any
> more refactoring.

btw, I didn't ask to do refactoring right now :)

Comment 41

[Trevor Saunders (:tbsaunde)]

Attachment: patch

should address nits.

Comment 42

[Trevor Saunders (:tbsaunde)]

(In reply to comment #41)
> Created attachment 538224 [details] [diff] [review] [review]
> patch
> 
> should address nits.

This still causes the tried to run a compile and go script on a cleared scope test failures.     From https://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/10ff69b04b88e06f/87f89aaec17c1aed?lnk=raot it looks like what's happening is that there are event handlers that are still installed when the test is "finished".  I noticed that aAtkObject apparently supported a "targetID" as well as the id attribute so I tried setting that to be getAccessible("link1") in the theory that  maybe events were getting delivered to one but not the other, but that didn't seem to help.

Comment 43

[Trevor Saunders (:tbsaunde)]

Attachment: fix only patch

this is only the fix without the first set of test changes.  Since per my last comment I'm a little worried about what might be going on and not sure how long it will take to sort out.  So, I think it may be a good idea to get this on aurora.  If you guys think its worth getting fixed tests on aurora feel free to cancel the approval request.

Comment 44

[Marco Zehe (:MarcoZ)]

Comment on attachment 538224 [details] [diff] [review]
patch

Hm, from the patch, I don#t see what could be wrong with those tests except that there are differences in the actionCount name in the array versus the function prototype. Are you sure you have all the parameters from the array to the function call etc. right?

Comment 45

[Marco Zehe (:MarcoZ)]

Comment on attachment 538224 [details] [diff] [review]
patch

>   <a id="link" href="">link</span>

Please put a proper closing </a> instead of </span>

 
>+  <img id="onclick_img" onclick="" src="../moz.png" tabindex="0">

Please end with a proper /> instead of just a > so the tab is properly closed.

And above in the actionsArray, you have:

>+        {
>+          ID: getAccessible("link1").firstChild,
>+          actionName: "jump",
>+          events: CLICK_EVENTS,
>+          actionsCount: 1
>+        },
>+        {
>+          ID: getAccessible("link2").firstChild,
>+          actionName: "click",
>+          events: CLICK_EVENTS,
>+          actionsCount: 1
>         }

Is getting the ID like this really supposed to work?

Comment 46

[Marco Zehe (:MarcoZ)]

Attachment: Further test adjustment and fixes, but still not working

Comment 47

[Trevor Saunders (:tbsaunde)]

(In reply to comment #46)
> Created attachment 539441 [details] [diff] [review] [review]
> Further test adjustment and fixes, but still not working
Marco  said on irc that the problem was when we try to focus link2.  when I broke on DoAction(0 in gdb I found that nsAccessible::DoAction() was being called not anything in nsLinkableAccessible.  nothing appears immediately wrong with nsAccessible::DoAction() however I notice that  nsAccessible::DoCommand() just calls NS_DISPATCH_RUNNABLE_METHOD_ARG2() with a function that sends mouse up / down events, but I'm not seeing why a click event will get generated.

Comment 48

[Marco Zehe (:MarcoZ)]

(In reply to comment #43)
> Created attachment 538820 [details] [diff] [review] [review]
> fix only patch
> 
> this is only the fix without the first set of test changes.  Since per my
> last comment I'm a little worried about what might be going on and not sure
> how long it will take to sort out.  So, I think it may be a good idea to get
> this on aurora.  If you guys think its worth getting fixed tests on aurora
> feel free to cancel the approval request.

I second this request to take the code-only fix on Aurora and fix the hang that will affect a lot of users using WordPress, Wikipedia editing and others. We've discussed this internally and also Alex Surkov as the module owner agrees it's worth taking the code-only fix on Aurora now.

Comment 49

[Marco Zehe (:MarcoZ)]

Comment on attachment 535493 [details] [diff] [review]
patch to land

This is the proper patch with test coverage that has been on Mozilla-Central for a while. It fixes the hang, is low risk, and it has test coverage. I will move the additional test coverage to a separate bug where we can also work on what those additional tests apparently uncover as a separate bug, but this one here fixes this particular bug and should go to Aurora ASAP so the hang gets off users' annoyance.

Comment 50

[Marco Zehe (:MarcoZ)]

Resolving fixed because the initial fix landed (see comment #39) and fixes the bug.

Comment 51

[LegNeato]

Comment on attachment 535493 [details] [diff] [review]
patch to land

Approved for mozilla-aurora

Comment 52

[Marco Zehe (:MarcoZ)]

Pushed on Trevor's behalf to mozilla-aurora: http://hg.mozilla.org/releases/mozilla-aurora/rev/addb57fc93b5

Comment 53

[Marco Zehe (:MarcoZ)]

Verified fixed in Aurora build Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a2) Gecko/20110616 Firefox/6.0a2

Comment 54

[Virgil Dicu [:virgil] [QA]]

Set status to verified based on the above comment.