419747 - qm-win2k3-01 goes orange intermittently on browser_autodiscovery.js tests

Status: RESOLVED FIXED

(Firefox :: General, defect, P1)

Description

[Myk Melez [:myk] [@mykmelez]]

According to smaug on IRC, the following browser_autodiscovery.js test failures have been happening randomly on qm-win2k3-01 lately:

	FAIL - rel search discovered - chrome://mochikit/content/browser/browser/base/content/test/browser_autodiscovery.js

	FAIL - rel is case insensitive - chrome://mochikit/content/browser/browser/base/content/test/browser_autodiscovery.js

They happened again a moment ago.  Here's the brief log:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1204069318.1204071563.7208.gz

cc:ing folks who have touched that file.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

The first time I've seen the failure was after http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1204020590&maxdate=1204022788

Comment 2

[Robert Sayre]

(In reply to comment #1)
> The first time I've seen the failure was after
> http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1204020590&maxdate=1204022788
> 

No tests for Bug 419549...

Comment 3

[:Gavin Sharp [email: [e-mail]]]

The patch for bug 380454 made DOMLinkAdded fire asynchronously, and changed the test to use a setTimeout(test, 0). Seems likely that in the failure cases, 10ms isn't long enough for the DOMLinkAdded event to have fired, especially given the unit test boxes' tendency to be resource constrained.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: setTimeout(..., 0) -> setTimeout(..., 20)

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 306256 [details] [diff] [review]
setTimeout(..., 0) -> setTimeout(..., 20) 

20ms might be enough.

Comment 6

[Brendan Eich [:brendan]]

If 20ms is not, do we get intermittent orange again? How about polling?

/be

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: simple, ugly polling

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Won't this lead to infinite recursion if any of the test conditions fail?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

No recursion but timer loop, which is IMHO ok for a test. Mochitest should
eventually stop that test if it fails.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #9)
> No recursion but timer loop

That's what I meant.

> which is IMHO ok for a test. Mochitest should eventually stop that test if it
> fails.

Have you tested that? I don't really like relying on mochitest runaway-test handling, and turning any failure into a "test timed out" error hurts debugging should the test actually fail on tinderbox.

Perhaps we could change modify browser.js code to allow defining arbitrary functions to be called after the browser's DOMLinkAdded listener has done it's work, and change the test to use that? Somewhat of a test-specific hack, but I imagine it may be useful to extensions as well.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #10)
> Have you tested that?
Yes. It does give the timeout failure quite fast and whoever sees the
error can look at which one was the previous succeeding test.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Attachment: alternate solution

sayrer proposed this on IRC, it works well and avoids the need for polling.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

mozilla/browser/base/content/test/browser_autodiscovery.js 	1.4