Closed Bug 651001 Opened 10 years ago Closed 10 years ago

Test for bug 583948 uses flaky timeouts

Categories

(Core :: XUL, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla6

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Attached patch Patch (v1) (obsolete) — Splinter Review
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.
Attached patch Patch (v2)Splinter Review
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.
http://hg.mozilla.org/mozilla-central/rev/2440b3f2c339
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
You need to log in before you can comment on or make changes to this bug.