Open Bug 477409 Opened 16 years ago Updated 2 years ago

make reftest check its own timeout invariants


(Testing :: Reftest, defect)



(Not tracked)



(Reporter: dbaron, Unassigned)




(2 files, 1 obsolete file)

Attached patch patchSplinter Review
There have been a lot of correlated load failures on tinderbox lately, so I'm beginning to wonder if there's something wrong with the reftest harness's handling of its load failure timeouts.

Here's a patch that makes it issue an unexpected failure report if it detects that something has gone wrong with its timeouts.
Attachment #361076 - Flags: review?(jruderman)
(Note that there could also be interaction with the slow script dialog involved.  sayrer says mochitest disables it through  Maybe we'll want the same once ted's patch in bug 468913 lands.)
Attachment #361076 - Flags: review?(jruderman) → review+
Other ideas:

* OnDocumentLoad could complain if there is not an outstanding timer.

* The "if (gClearingForAssertionCheck)" bit might need to be moved down before the "Ignore load events for previous documents" bit.

* The "Ignore load events for previous documents" bit could output some "NOISE:" explaining what it thinks is going on.

* When contentRootElement is null, it should probably do something sane.
I pushed the existing patch as .  I'll try to write a patch for some of those other ideas soon...
Blocks: 473680
Attached patch patch 2 (obsolete) — Splinter Review
This is equivalent to:

(In reply to comment #2)
> * The "if (gClearingForAssertionCheck)" bit might need to be moved down before
> the "Ignore load events for previous documents" bit.

but simpler, and I think it could be the fix for the cause of our double-timeouts lately.
Attachment #361591 - Flags: review?(jruderman)
"but about:blank is unlikely to take a really long time to load"

I'm not sure how to convince myself that this is the only way to get here with the URL being about:blank.  And we're skeptical enough that about:blank *could* be taking a long time to load.

How about using a unique URL for the clearing, like 
  data:text/html,<!-- CLEAR -->
Attached patch patch 2Splinter Review
Good idea.
Attachment #361591 - Attachment is obsolete: true
Attachment #361613 - Flags: review?(jruderman)
Attachment #361591 - Flags: review?(jruderman)
Attachment #361613 - Flags: review?(jruderman) → review+
Is there any reason not to land this on m-1.9.1?
Assignee: dbaron → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.