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

RESOLVED FIXED in Firefox 3

Status

()

Firefox
General
P1
blocker
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: myk, Assigned: Gavin)

Tracking

Trunk
Firefox 3
x86
Windows Server 2003
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

10 years ago
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 2

10 years ago
(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...
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.
OS: Windows 2000 → Windows Server 2003
Blocks: 380454
Severity: normal → blocker

Comment 4

10 years ago
Created attachment 306256 [details] [diff] [review]
setTimeout(..., 0) -> setTimeout(..., 20)

Comment 5

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

20ms might be enough.
Attachment #306256 - Flags: review?(gavin.sharp)
If 20ms is not, do we get intermittent orange again? How about polling?

/be

Comment 7

10 years ago
Created attachment 306392 [details] [diff] [review]
simple, ugly polling
Attachment #306256 - Attachment is obsolete: true
Attachment #306392 - Flags: review?(gavin.sharp)
Attachment #306256 - Flags: review?(gavin.sharp)
Won't this lead to infinite recursion if any of the test conditions fail?

Comment 9

10 years ago
No recursion but timer loop, which is IMHO ok for a test. Mochitest should
eventually stop that test if it fails.
(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

10 years ago
(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.
Created attachment 306632 [details] [diff] [review]
alternate solution

sayrer proposed this on IRC, it works well and avoids the need for polling.
Assignee: nobody → gavin.sharp
Attachment #306392 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #306632 - Flags: review?(Olli.Pettay)
Attachment #306392 - Flags: review?(gavin.sharp)

Updated

10 years ago
Attachment #306632 - Flags: review?(Olli.Pettay) → review+
Keywords: checkin-needed
Priority: -- → P1
Target Milestone: --- → Firefox 3
mozilla/browser/base/content/test/browser_autodiscovery.js 	1.4 
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.