The following tests (at least) are calling SimpleTest.finish() two or more times. This causes the tests following it to not run. /tests/content/canvas/test/test_mozGetAsFile.html (3 times) /tests/content/html/content/test/test_bug430392.html (3 times) /tests/content/media/test/test_audio1.html (3 times) /tests/toolkit/components/url-classifier/tests/mochitest/test_classifier_worker.html (2 times) chrome://mochitests/content/chrome/dom/workers/test/test_file.xul (4 times) chrome://mochitests/content/chrome/dom/workers/test/test_filePosting.xul (2 times) chrome://mochitests/content/chrome/dom/workers/test/test_fileReaderSync.xul (9 times) chrome://mochitests/content/browser/browser/base/content/test/browser_tab_dragdrop2_frame1.xul (2 times) These should be fixed, but finish() could be changed to warn for this situation.

Comment on attachment 556820 [details] [diff] [review] simple patch to do this Review of attachment 556820 [details] [diff] [review]: ----------------------------------------------------------------- This patch looks like we are just warning and not protecting against it being called twice. Is the intention of this patch to warn and then fix later?

The test will go orange if it calls finish() twice. Is there something more that you mean? Note that we don't want to use this patch until the tests themselves are fixed.

I just assumed these tests were not going orange since they exist and are not perma orange tests. I recall fixing browser_tab_dragdrop2_frame1.xul in my patch queue.

This patch is old and I believe we have changed the way we count finish in the last year.

My patch in bug 792462 is largely the same as this (except it returns to try to avoid the test breaking other things). In running it through the try server, there were only 2 tests that broke, and Ehsan and I have fixes for both -- I'll update soon.

We'd want a combination of both patches: Vlad's has it returning early, and mine includes the document location in the error message to more clearly show which test has the problem.

Updated patch, combining mine and Enn's patches (note that the test name would have been printed as part of the ok fail, but the docoument.location is handy for debugging too). Also two test fixes that were the only two that tripped this on a full tryserver run.

Comment on attachment 665469 [details] [diff] [review] updated patch Review of attachment 665469 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the below comment addressed. ::: testing/mochitest/tests/SimpleTest/SimpleTest.js @@ +664,5 @@ > **/ > SimpleTest.finish = function () { > + if (SimpleTest._alreadyFinished) { > + SimpleTest.ok(false, "[SimpleTest.finish()] test " + document.location + " already called finish!"); > + return; The return here seems like a mistake -- we should still let finish() make progress.

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #8) > note that the test name would have been printed as part of the ok fail If I remember correctly, I think that displays the currently running test name, not the test that called SimpleTest.finish(), which can be different when asynchronous callbacks fire after the test finishes. It might be worth fixing or logging that problem as well.

Comment on attachment 665469 [details] [diff] [review] updated patch Review of attachment 665469 [details] [diff] [review]: ----------------------------------------------------------------- I agree with ehsan about the early return, otherwise this is nice and simple.