Closed Bug 659620 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Disability Access APIs, defect, critical)

x86
Windows 7
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla7
Tracking Status
firefox6 + fixed

People

(Reporter: MarcoZ, Assigned: tbsaunde)

References

Details

(Keywords: hang, regression, verified-aurora)

Attachments

(1 file, 11 obsolete files)

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.
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
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.
Firefox doesn't hung, UI works, I can operate on accessible tree, but NVDA keeps silent.
(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?
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?
Attached patch test patch (obsolete) — Splinter Review
make nsLinkableAccessible::BindToParent() never set mActionAcc = this;
Comment on attachment 535064 [details] [diff] [review]
test patch

This patch definitely fixes the hang! Verified locally by building with and without it.
Attachment #535064 - Flags: feedback+
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
Attachment #535064 - Flags: review?(bolterbugz)
Comment on attachment 535064 [details] [diff] [review]
test patch

r=me. Thanks for the explanation.
Attachment #535064 - Flags: review?(bolterbugz) → review+
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
Also, please add mochitests for this, we shouldn't hit this problem twice.
> 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?
(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 on attachment 535064 [details] [diff] [review]
test patch

this breaks the action number, be careful when you do not 1 to 1 rollback
Attachment #535064 - Flags: review-
(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
(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 on attachment 535064 [details] [diff] [review]
test patch

r-=me based on new data (nit, existing test failure, and Alex's catch).
Attachment #535064 - Flags: review+ → review-
for ome reason getElementById() doesn't seem to be working
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.
Blocks: 659826
Attached patch (next patch (obsolete) — Splinter Review
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
Duplicate of this bug: 659826
Attached patch patch (obsolete) — Splinter Review
correct tests html
Attachment #535064 - Attachment is obsolete: true
Attachment #535271 - Attachment is obsolete: true
Attachment #535282 - Attachment is obsolete: true
Attachment #535295 - Flags: review?(surkov.alexander)
(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 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>
Attachment #535295 - Flags: review?(surkov.alexander) → review+
Assignee: nobody → trev.saunders
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
address comments on tests
Attachment #535295 - Attachment is obsolete: true
Attached patch patch to landSplinter Review
correct patch actually includes changes addressing comments this time.
Attachment #535437 - Attachment is obsolete: true
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.
(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>?
(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.
Can we move on on this one? This is needed for Aurora since the hang is in there, too.
Please request approval to land on Aurora for 6 with a risk analysis. Thanks.
Attached patch patch (obsolete) — Splinter Review
test linkable accessibles more.  These seem to have passed atleast once for me, but seem to hang other times.
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();
Attached patch test cleanup (obsolete) — Splinter Review
Alex hopefully this is closer to what you were looking for the first time around.
Attachment #537703 - Flags: review?(surkov.alexander)
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 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
Attachment #537703 - Flags: review?(surkov.alexander) → review-
Attached patch updated patch address comments (obsolete) — Splinter Review
Attachment #537703 - Attachment is obsolete: true
Attachment #538167 - Flags: review?(surkov.alexander)
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.
Attachment #538167 - Flags: review?(surkov.alexander) → review+
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.
(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 :)
Attached patch patch (obsolete) — Splinter Review
should address nits.
(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.
Attached patch fix only patch (obsolete) — Splinter Review
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.
Attachment #538820 - Flags: approval-mozilla-aurora?
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 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?
(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.
(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.
Attachment #538820 - Attachment is obsolete: true
Attachment #538820 - Flags: approval-mozilla-aurora?
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.
Attachment #535493 - Flags: approval-mozilla-aurora?
Resolving fixed because the initial fix landed (see comment #39) and fixes the bug.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Attachment #536682 - Attachment is obsolete: true
Attachment #538167 - Attachment is obsolete: true
Attachment #538224 - Attachment is obsolete: true
Attachment #539441 - Attachment is obsolete: true
Flags: in-testsuite+
Depends on: 664443
Target Milestone: --- → mozilla7
Comment on attachment 535493 [details] [diff] [review]
patch to land

Approved for mozilla-aurora
Attachment #535493 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Pushed on Trevor's behalf to mozilla-aurora: http://hg.mozilla.org/releases/mozilla-aurora/rev/addb57fc93b5
Verified fixed in Aurora build Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a2) Gecko/20110616 Firefox/6.0a2
Keywords: verified-aurora
Set status to verified based on the above comment.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.