599628 - [SeaMonkey] mochitests-1/5: "TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug561636.html | Test timed out."

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Bruno 'Aqualon' Escherl]

Probably the result from bug 599258. We don't even get to the first check in that test, so the hang occurs somewhere else.

Comment 1

[Justin Wood (:Callek)]

Does this fail locally, and if so can you check if there are any existing observers for invalidformsubmit registered. [I don't see how]

But this test shouldn't call |waitForExplicitFinish| if there are no observers and thus shouldn't time out.

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Comment 3

[Mounir Lamouri (:mounir)]

The test is run when onload is called. waitForExplicitFinish should have been called before. I don't see how that might break SeaMonkey but that was a mistake. So, now finish() is explicitly call if there is no observer.

Comment 4

[Justin Wood (:Callek)]

Comment on attachment 478592 [details] [diff] [review]
Patch v1

Unable to test locally right now, so feedback+ If you want an official review before being willing to push this, re-request and I'll be sure to do just that [with local testing]. code-inspection-wise this looks good though.

Comment 5

[Justin Wood (:Callek)]

Interesting... I didn't fail this locally.... :/

Comment 6

[Bruno 'Aqualon' Escherl]

The test hang for me and passes with the patch. So looks good.

Comment 7

[Mounir Lamouri (:mounir)]

Justin, according to comment 5, you were able to test this. May I have you r+? :)

Comment 8

[Justin Wood (:Callek)]

(In reply to comment #7)
> Justin, according to comment 5, you were able to test this. May I have you r+?
> :)

c#5 was about with_out_ this patch, I didn't fail locally....  but r+ anyway based on Aqualon's c#6

Comment 9

[Mounir Lamouri (:mounir)]

http://hg.mozilla.org/mozilla-central/rev/d0d3ac2ce08c

Comment 10

[Serge Gautherie (:sgautherie)]

(In reply to comment #9)
> http://hg.mozilla.org/mozilla-central/rev/d0d3ac2ce08c

    1.32 +  if (!observers.hasMoreElements()) {

Nit: adding an explicit todo(false, ...) or ok(true, ...) would be perfect.

    1.33 +    SimpleTest.finish();
    1.34 +    return;
    1.35 +  }

Comment 11

[Mounir Lamouri (:mounir)]

The current mochitest behavior is to add a todo if there is no test. Isn't that enough?

Comment 12

[Justin Wood (:Callek)]

(In reply to comment #11)
> The current mochitest behavior is to add a todo if there is no test. Isn't that
> enough?

Yes it is. And I have not seen this on any runs of the test for SeaMonkey machines since this landed--> Verified