Test for bug 583948 uses flaky timeouts

RESOLVED FIXED in mozilla6

Status

()

Core
XUL
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

Trunk
mozilla6
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
Created attachment 526882 [details] [diff] [review]
Patch (v1)
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #526882 - Flags: review?(jonas)
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.
http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1303190802.1303194471.9767.gz
Created attachment 527016 [details] [diff] [review]
Patch (v2)

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.
Attachment #526882 - Attachment is obsolete: true
Attachment #526882 - Flags: review?(jonas)
Attachment #527016 - Flags: review?(jonas)
(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.
Attachment #527016 - Flags: review?(jonas) → review+
http://hg.mozilla.org/mozilla-central/rev/2440b3f2c339
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
You need to log in before you can comment on or make changes to this bug.