Comment on attachment 526882 [details] [diff] [review] Patch (v1) This seems to significantly change what the test does. Before this change the test would do the following: for (i=0; i < 6; ++i) { otherWindow.document.commandDispatcher.updateCommands(''); <short asynchronous wait> otherWindow.location.reload(); <long asynchronous wait> } With this change we would for (i=0; i < 6; ++i) { otherWindow.location.reload(); otherWindow.document.commandDispatcher.updateCommands(''); <long asynchronous wait> } Is that change not significant? In particular, was it important that the reload happend shortly after the updateCommands call? Presumably while updateCommands were doing things. Would be great to have someone that knows this test better review this change.

Not sure whether this helps, but the flaky failures I saw twice on TraceMonkey were like http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1303092448.1303096155.24199.gz - looking to me like it was somehow finding that otherWindow.location was non-null, and proceeding with calling reload() on it, long after otherWindow was closed and the whole test had finished.

This patch modifies the test to wait between calling updateCommands and reload. I think this should make the new test behave the same way as the old test. Also, feel free to forward the review request to somebody else if you know of a better reviewer for this change.

(In reply to comment #3) > Not sure whether this helps, but the flaky failures I saw twice on TraceMonkey > were like > http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1303092448.1303096155.24199.gz > - looking to me like it was somehow finding that otherWindow.location was > non-null, and proceeding with calling reload() on it, long after otherWindow > was closed and the whole test had finished. Yeah, the non-patched version of the patch races the closing of the window against the window.location.reload call, and I can see it failing in that way, definitely.