Last Comment Bug 651001 - Test for bug 583948 uses flaky timeouts
: Test for bug 583948 uses flaky timeouts
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla6
Assigned To: :Ehsan Akhgari
:
Mentors:
Depends on:
Blocks: FlakyTimeout
  Show dependency treegraph
 
Reported: 2011-04-18 17:34 PDT by :Ehsan Akhgari
Modified: 2011-04-26 13:40 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (1.81 KB, patch)
2011-04-18 17:38 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Patch (v2) (1.86 KB, patch)
2011-04-19 09:25 PDT, :Ehsan Akhgari
jonas: review+
Details | Diff | Splinter Review

Description :Ehsan Akhgari 2011-04-18 17:34:17 PDT

    
Comment 1 :Ehsan Akhgari 2011-04-18 17:38:18 PDT
Created attachment 526882 [details] [diff] [review]
Patch (v1)
Comment 2 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-04-18 18:10:05 PDT
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.
Comment 3 Phil Ringnalda (:philor) 2011-04-18 19:02:48 PDT
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.
Comment 5 :Ehsan Akhgari 2011-04-19 09:25:45 PDT
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.
Comment 6 :Ehsan Akhgari 2011-04-19 09:26:57 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.