In preparation for bug 649012, I'm removing all of the flaky setTimeout's from the accessibility tests.

Comment on attachment 526555 [details] [diff] [review] Patch (v1) > menu1.open = true; > >- window.setTimeout(function() { >+ menu1.addEventListener("popupshown", function() { > var menu2 = document.getElementById("menu_item2"); > menu2.open = true; > >- window.setTimeout(function() { >+ menu2.addEventListener("popupshown", function() { > testGroupAttrs("menu_item1.1", 1, 1); > testGroupAttrs("menu_item1.2", 1, 3); > testGroupAttrs("menu_item1.4", 2, 3); > testGroupAttrs("menu_item2", 3, 3); > testGroupAttrs("menu_item2.1", 1, 2, 1); > testGroupAttrs("menu_item2.2", 2, 2, 1); > >+ menu1.open = false; >+ menu2.open = false; >+ just curious, why you need to close them? >+++ b/accessible/tests/mochitest/common.js > if (state.value & STATE_BUSY) > return waitForDocLoad(); > >- window.setTimeout(aFunc, 150); >+ window.setTimeout(aFunc, 0); nice, I wanted to do that for times :) >+++ b/accessible/tests/mochitest/events.js >@@ -299,17 +299,17 @@ function eventQueue(aEventType) > if (!aUncondProcess && this.areAllEventsExpected()) { > // We need delay to avoid events coalesce from different invokers. > var queue = this; > SimpleTest.executeSoon(function() { queue.processNextInvoker(); }); > return; > } > > // Check in timeout invoker didn't fire registered events. >- window.setTimeout(function(aQueue) { aQueue.processNextInvoker(); }, 500, >+ window.setTimeout(function(aQueue) { aQueue.processNextInvoker(); }, 0, > this); nope, we need non zero timeout to ensure there's no unexpected events, zero timeout doesn't always work. >- // SimpleTest.executeSoon() doesn't help here: use setTimeout() with a little delay. >- setTimeout(test_txc7, 25); >+ setTimeout(test_txc7, 0); iirc, 0 timeout resulted in random failures, ideally this test should be changed to use accessible events instead timeouts. r=me with events.js timeout fixed.

(In reply to comment #2) > Comment on attachment 526555 [details] [diff] [review] > Patch (v1) > > > > menu1.open = true; > > > >- window.setTimeout(function() { > >+ menu1.addEventListener("popupshown", function() { > > var menu2 = document.getElementById("menu_item2"); > > menu2.open = true; > > > >- window.setTimeout(function() { > >+ menu2.addEventListener("popupshown", function() { > > testGroupAttrs("menu_item1.1", 1, 1); > > testGroupAttrs("menu_item1.2", 1, 3); > > testGroupAttrs("menu_item1.4", 2, 3); > > testGroupAttrs("menu_item2", 3, 3); > > testGroupAttrs("menu_item2.1", 1, 2, 1); > > testGroupAttrs("menu_item2.2", 2, 2, 1); > > > >+ menu1.open = false; > >+ menu2.open = false; > >+ > > just curious, why you need to close them? No special reason, seemed cleaner this way. > >+++ b/accessible/tests/mochitest/events.js > >@@ -299,17 +299,17 @@ function eventQueue(aEventType) > > if (!aUncondProcess && this.areAllEventsExpected()) { > > // We need delay to avoid events coalesce from different invokers. > > var queue = this; > > SimpleTest.executeSoon(function() { queue.processNextInvoker(); }); > > return; > > } > > > > // Check in timeout invoker didn't fire registered events. > >- window.setTimeout(function(aQueue) { aQueue.processNextInvoker(); }, 500, > >+ window.setTimeout(function(aQueue) { aQueue.processNextInvoker(); }, 0, > > this); > > nope, we need non zero timeout to ensure there's no unexpected events, zero > timeout doesn't always work. How do accessibility events work? Are they handled off of the usual event queue? If yes, then we should just be able to hit the event queue a few times to make sure that such unexpected events do not happen. In general, waiting a magical number of milliseconds to see that something does not occur in the event queue is a bad idea. > >- // SimpleTest.executeSoon() doesn't help here: use setTimeout() with a little delay. > >- setTimeout(test_txc7, 25); > >+ setTimeout(test_txc7, 0); > > iirc, 0 timeout resulted in random failures, ideally this test should be > changed to use accessible events instead timeouts. If using a timeout used to result in random failures, using a larger timeout will also result in a similar random failure, just less frequently. If this is a concern, the test should be rewritten as you suggest, but I don't know how it's supposed to work, so I don't think that I can do that myself.

(In reply to comment #3) > How do accessibility events work? Are they handled off of the usual event > queue? usually a11y events are fired when refresh driver triggers us. So it may happen after zero timeout. > If yes, then we should just be able to hit the event queue a few times > to make sure that such unexpected events do not happen. I don't get technical side. > In general, waiting a magical number of milliseconds to see that something does > not occur in the event queue is a bad idea. not nice, and doesn't us guarantee that it works 100%. Anything in replacement? > If this is a concern, the test should be rewritten as you suggest, but I don't > know how it's supposed to work, so I don't think that I can do that myself. I'll try to come with something.

The refresh driver is run off of the OS event queue, which basically the main point of entry to Firefox from the OS once there is a window open. setTimeout(f, 0) will post an event to this queue, which is triggered at least 4ms after the call, so I think if accessibility events are raised off of the refresh driver, then setTimeout(f, 0) should be enough to make sure that they're fired (since the refresh driver gets a chance to run *at least* once before "f" is executed. To support this idea, I've pushed this patch to the try server, and the mochitest-a11y suite has been green...

1) I fixed test_txtctrl.xul to not use setTimeout 2) Get back 500 ms timeout for waiting to make sure there's no event. I'm not sure how to change it. Also I wouldn't go with 0 timeout since I think it may not cover some cases we need to care about. The case I try to handle here is the same a11y event can be fired on different notifications from gecko (for example, a11y focus event can be fired on DOM focus event or RadioStateChange event). Gecko can call into a11y from various places, depending on accessible tree state, these notifications may be deferred (or processed immediately), after processing a11y events may be deferred or fired immediately. The logic is not clear is some parts are really messy. Therefore I don't want to change to 0 timeout.

(In reply to comment #6) > 2) Get back 500 ms timeout for waiting to make sure there's no event. > > I'm not sure how to change it. Also I wouldn't go with 0 timeout since I > think it may not cover some cases we need to care about. The case I try to > handle here is the same a11y event can be fired on different notifications > from gecko (for example, a11y focus event can be fired on DOM focus event or > RadioStateChange event). Gecko can call into a11y from various places, > depending on accessible tree state, these notifications may be deferred (or > processed immediately), after processing a11y events may be deferred or > fired immediately. The logic is not clear is some parts are really messy. > Therefore I don't want to change to 0 timeout. What if we hit the event loop a number of times, let's say, 100 times? Does that sound like a way to deal with this problem?

(In reply to comment #7) > What if we hit the event loop a number of times, let's say, 100 times? Does > that sound like a way to deal with this problem? It's a problem, theoretically, but that doesn't happen currently, no helper function that can add some unexpected events unexplicitly. If you feel like that's quite necessary then the check can be added to make sure the number of unexpected events doesn't exceed the certain limit.

I landed what we have - http://hg.mozilla.org/mozilla-central/rev/bbceb00866f0 let's figure out what we can do with 500ms timeout for unexpected events check.

(In reply to comment #8) > (In reply to comment #7) > > > What if we hit the event loop a number of times, let's say, 100 times? Does > > that sound like a way to deal with this problem? > > It's a problem, theoretically, but that doesn't happen currently, no helper > function that can add some unexpected events unexplicitly. If you feel like > that's quite necessary then the check can be added to make sure the number > of unexpected events doesn't exceed the certain limit. Oh, no, that's not what I meant. You have an event and you want to make sure that it doesn't happen, right? So, what if you install an event handler, and do something like |ok(false, "Should not have happened");| inside it. Then, in order to give the event a chance to be fired (if something is broken for example), you spin the event loop 100 times. If there's a bug which causes the event to be fired, the event handler you've installed would get called, and the test would fail. Otherwise, you can be relatively sure that the event doesn't get fired, and everything is OK. We use this technique in a bunch of places in our tests for making sure an event does not happen. See <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/text/tests/test_bug527935.html?force=1#64> for an example.

(missed your answer, thanks davidb for pointing out, needinfo usually works better than summary change:) ) What do you mean by event loop 100 times? I can see setTimeout in test you gave as example, basically that's what we do, no?

Sorry, I've paged this bug out of my memory, and won't be updating the patch myself... I'll be happy to try to page this stuff back in once someone starts owning bug 649012. Clearing the ni? until then.

No assignee, updating the status.

No assignee, updating the status.