catch the case where .finish() gets called before waitForExplicitFinish()

NEW
Assigned to

Status

Testing
Mochitest
7 years ago
9 months ago

People

(Reporter: mdas, Assigned: Aman Dwivedi, Mentored)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=js][good first bug])

Attachments

(2 attachments, 10 obsolete attachments)

Enforce that waitForExplicitFinish() gets called before an explicit SimpleTest.finish() call, otherwise we will lose test data. We've seen this before in instances where waitForExplicitFinish gets called in some callback, but .finish() gets called shortly after loading the page.
Assignee: mdas → nobody

Comment 1

4 years ago
Relevant code: http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js
Relevant methods: finish and waitForExplicitFinish
Whiteboard: [mentor=jdm][lang=js]

Updated

4 years ago
Whiteboard: [mentor=jdm][lang=js] → [mentor=jdm][lang=js][good first bug]

Comment 2

4 years ago
Created attachment 8387166 [details] [diff] [review]
2.patch

Tests performed locally-
Ran local test(having both methods) with no change......passed
Ran local test(having both methods with mistake of finish() called before waitForExplicitFinish()...............failed
Ran local test(having none of the two methods - ./mach mochitest-plain content/base/test/test_bug420700.html) with no change........passed
Ran local test(having none of the two methods - content/base/test/test_bug420700.html) with the above mistake  .....1 pass 1 fail
Ran local test(having none of the two methods - content/base/test/test_bug420700.html) with the mistake of having only finish().....1 pass 2 fails

Fails in the last case are-

[SimpleTest.finish()] waitForExplicitFinish() has not been invoked
[SimpleTest.finish()] this test already called finish!
Attachment #8387166 - Flags: review?(josh)

Comment 3

4 years ago
Comment on attachment 8387166 [details] [diff] [review]
2.patch

Review of attachment 8387166 [details] [diff] [review]:
-----------------------------------------------------------------

Good work! Please replace the [PATCH] in the commit message with "Bug 682901 - ", and get rid of the blank line before the commit message as well. Make sure to run the tests you've been using after making the changes I've requested!

::: testing/mochitest/tests/SimpleTest/SimpleTest.js
@@ +766,4 @@
>      SimpleTest._cleanupFunctions.push(aFunc);
>  };
>  
> +SimpleTest._automatic = false;   // set when finish() is called automatically and not when called explicitly

I believe we don't actually need a separate variable for this. _stopOnLoad seems to give us what we need already.

@@ +773,5 @@
>  **/
>  SimpleTest.finish = function () {
> +    /* don't allow tests to call explicit finish before |SimpletestwaitForExplicitFinish()|
> +       or leave explicit call after |SimpleTest.waitForEFinish()|
> +    */

I don't really understand this comment right now; I think it might be better phrased as "disallow calling finish before waitForExplicitFinish".

@@ +774,5 @@
>  SimpleTest.finish = function () {
> +    /* don't allow tests to call explicit finish before |SimpletestwaitForExplicitFinish()|
> +       or leave explicit call after |SimpleTest.waitForEFinish()|
> +    */
> +    if((SimpleTest._stopOnLoad && !SimpleTest._automatic) || (SimpleTest._automatic && !SimpleTest._stopOnLoad))    

I think we can just check for _stopOnLoad, since calling waitForExplicitFinish makes it false.
Attachment #8387166 - Flags: review?(josh) → feedback+

Comment 4

4 years ago
Created attachment 8389025 [details] [diff] [review]
second.patch

I ran all the tests mentioned in comment 2. They gave same results.
Hope this one is fine.
Attachment #8387166 - Attachment is obsolete: true

Comment 5

4 years ago
Comment on attachment 8389025 [details] [diff] [review]
second.patch

Good stuff!
Attachment #8389025 - Flags: review+

Comment 6

4 years ago
This deserves a try run before landing; I'll push it for now, and I recommend you apply for an account as well: http://www.mozilla.org/hacking/committer/

Comment 7

4 years ago
pushed to try
https://tbpl.mozilla.org/?tree=Try&rev=d5ec55eb40c9
Mentor: josh@joshmatthews.net
Whiteboard: [mentor=jdm][lang=js][good first bug] → [lang=js][good first bug]

Comment 9

4 years ago
Jaspreet, do you remember what the status was here? If the patch worked and tests pass, we should really merge it :)
Flags: needinfo?(jaspreetsingh112)

Comment 10

4 years ago
hi! I'm new here , Please assign this bug to me!

Updated

4 years ago
Assignee: nobody → abhirav.kariya

Comment 11

4 years ago
Please help me get started as this is my first time!Thank you for assigning this bug to me.
Well, it seems like the second.patch is alrady enough to fix this bug.
I don't know what the result of the try server was, but probably wise to pull it through try server once again.
Abhirav, you could write a mochitest that would trigger this case. It would have to be skipped for now, because bug 1048446 is not fixed, but it would still be good to have a mochitest.

Updated

3 years ago
Assignee: abhirav.kariya → nobody
Flags: needinfo?(jaspreetsingh112)
jdm, can we get this patch landed and merged?
Flags: needinfo?(josh)
it seems we lost this a bit, I believe this is probably outdated functionality.  Do we need test cases?  Can we validate if this is still useful?
Outdated in what way? I would be surprised if anything has changed from Malini's original description.
Flags: needinfo?(josh)
the simpletest code has changed a lot, we have different code paths for handling multiple finish() calls which didn't exist back in 2011.  

We have a patch on here, with r+, but it never landed, so if there is anything we can do to either land it, or document what else is required it would be helpful.
This patch requires an automated test demonstrating that it works as expected. Maybe a new test added to testing/mochitest/Harness_sanity/ that calls SimpleTest.expectUncaughtException() and then SimpleTest.finish() would do the trick; I'm not sure. The try job didn't show any unexpected failures, but I'm no longer confident that this patch works as expected.
(Assignee)

Comment 20

a year ago
Hi Josh! I would like to take this up if not already fixed. :) Please assign it to me if anything needs to be done in this.
Flags: needinfo?(josh)
It's yours! My last comment still stands - a test demonstrating that the change works as expected is important. Please ask questions if anything is unclear!
Assignee: nobody → dwivedi.aman96
Flags: needinfo?(josh)
(Assignee)

Comment 22

a year ago
Created attachment 8824773 [details]
test_bug682901.html

I applied the patch and then ran this test. The test passed. Please see and comment if something's wrong. :)
Flags: needinfo?(josh)
Attachment #8824773 - Flags: review?(josh)
(Assignee)

Comment 23

a year ago
Created attachment 8824774 [details]
SimpleTest.js.rej

Applying that patch also produced this .rej file. If I m not wrong, the patch needs some change too.
Attachment #8824774 - Flags: feedback?(josh)
(In reply to Aman Dwivedi from comment #22)
> Created attachment 8824773 [details]
> test_bug682901.html
> 
> I applied the patch and then ran this test. The test passed. Please see and
> comment if something's wrong. :)

This test doesn't test the error path the patch would be checking for.
In this case, I think a test would be needed that would give an error that would be checked by Python mochitest code. But I guess something from js would be fine as well (although I think from outside of the mochitest js code is more correct), because it is done with expectUncaughtException and https://bug1237363.bmoattachments.org/attachment.cgi?id=8717624 .
Comment on attachment 8824773 [details]
test_bug682901.html

As pointed out, this is not checking the condition that we care about, since SimpleTest.waitForExplicitFinish() is never called.
Flags: needinfo?(josh)
Attachment #8824773 - Flags: review?(josh) → review-
Comment on attachment 8824774 [details]
SimpleTest.js.rej

Yes. A .rej file means that there were conflicts merging the changes that need to be addressed. If you look at the contents of the file, you can try to apply the same changes to the original files by hand and generate a new patch that applies cleanly.
Attachment #8824774 - Flags: feedback?(josh)
(Assignee)

Comment 27

a year ago
Created attachment 8824809 [details] [diff] [review]
BugFix682901.patch

Thanks for help. I was not clear about it. Please review this patch.
Attachment #8824773 - Attachment is obsolete: true
Attachment #8824774 - Attachment is obsolete: true
Flags: needinfo?(josh)
Attachment #8824809 - Flags: review?(josh)
Comment on attachment 8824809 [details] [diff] [review]
BugFix682901.patch

Review of attachment 8824809 [details] [diff] [review]:
-----------------------------------------------------------------

We should rename the new test so it describes what it's testing: test_finish_called_before_waitForExplicitFinish.html.

Sorry for the delay in reviewing this change!

::: testing/mochitest/tests/Harness_sanity/test_bug682901.html
@@ +20,5 @@
> +/** Test for Bug 682901 **/
> +SimpleTest.expectUncaughtException();
> +throw "an uncaught exception";
> +SimpleTest.waitForExplicitFinish();
> +SimpleTest.finish();

There are a couple problems here:
* throwing an exception causes us to stop executing the rest of the test
* the point of this bug is to identify cases where a test calls `SimpleTest.finish()` before `SimpleTest.waitForExplicitFinish()`. This test is currently doing the opposite, so it should not cause an exception to be thrown.
Attachment #8824809 - Flags: review?(josh) → review-

Updated

a year ago
Flags: needinfo?(josh)
(Assignee)

Comment 29

a year ago
Created attachment 8828002 [details] [diff] [review]
BugFix682901.patch

Should it be like this? The test doesn't pass using this patch. Maybe I'm going wrong somewhere. :(
Attachment #8824809 - Attachment is obsolete: true
Flags: needinfo?(josh)
Attachment #8828002 - Flags: review?(josh)
For the record, there's no need to use "Need for information from" every time you request a patch. I get an email notification every time you add the review flag, so needinfo is redundant (and generates an extra email for me to read).
Flags: needinfo?(josh)
Comment on attachment 8828002 [details] [diff] [review]
BugFix682901.patch

Review of attachment 8828002 [details] [diff] [review]:
-----------------------------------------------------------------

Getting closer! Let me know if any of my comments are unclear!

::: testing/mochitest/tests/Harness_sanity/test_finish_called_before_waitForExplicitFinish.html
@@ +18,5 @@
> +<pre id="test">
> +<script class="testbody" type="text/javascript">
> +/** Test for Bug 682901 **/
> +SimpleTest.finish();
> +SimpleTest.waitForExplicitFinish();

The fact that this test now fails is good! Now we need to add a new API to SimpleTest (like expectUncaughtExceptions and expectRegisteredServiceWorker) that allows the test harness to treat that failing case as a success, so we can make this test pass.

::: testing/mochitest/tests/SimpleTest/SimpleTest.js
@@ +1065,5 @@
>  SimpleTest.finish = function() {
> +    /* disallow calling finish before waitForExplicitFinish
> +    */
> +    if (SimpleTest._stopOnLoad) {
> +        SimpleTest.ok(false, "[SimpleTest.finish()] waitForExplicitFinish() has not been invoked");

This is where we will want to check whether our special API was called, so we can do SimpleTest.ok(true, ...) instead if it was.
Attachment #8828002 - Flags: review?(josh) → feedback+
(Assignee)

Comment 32

a year ago
Created attachment 8829094 [details] [diff] [review]
BugFix682901.patch

Thanks Josh! Please review this patch.
Attachment #8828002 - Attachment is obsolete: true
Attachment #8829094 - Flags: review?(josh)
Comment on attachment 8829094 [details] [diff] [review]
BugFix682901.patch

Review of attachment 8829094 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mochitest/tests/SimpleTest/SimpleTest.js
@@ +1065,5 @@
>  SimpleTest.finish = function() {
> +    /* disallow calling finish before waitForExplicitFinish
> +    */
> +    if (SimpleTest._stopOnLoad) {
> +        SimpleTest.ok(true, "[SimpleTest.finish()] waitForExplicitFinish() has not been invoked");

This invalidates the purpose of this patch - the goal is to execute SimpleTest.ok(false) here when regular tests (ie. every test except test_finish_called_before_waitForExplicitFinish.html) do the wrong thing.

We need to add a separate API to SimpleTest (like the ones I mentioned in my previous review) which controls whether to use SimpleTest.ok(true) or SimpleTest.ok(false) here, so we can call that new API from test_finish_called_before_waitForExplicitFinish.html. Does that make sense?
Attachment #8829094 - Flags: review?(josh) → review-
(Assignee)

Comment 34

a year ago
Created attachment 8830650 [details] [diff] [review]
BugFix682901.patch

Hi Josh! Please review this patch.
Attachment #8829094 - Attachment is obsolete: true
Attachment #8830650 - Flags: review?(josh)
Comment on attachment 8830650 [details] [diff] [review]
BugFix682901.patch

Review of attachment 8830650 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there!

::: testing/mochitest/tests/Harness_sanity/test_finish_called_before_waitForExplicitFinish.html
@@ +19,5 @@
> +<script class="testbody" type="text/javascript">
> +/** Test for Bug 682901 **/
> +SimpleTest.expectExplicitFinish();
> +SimpleTest.finish();
> +SimpleTest.waitForExplicitFinish();

We can remove this waitForExplicitFinish call; it's not important for this test.

::: testing/mochitest/tests/SimpleTest/SimpleTest.js
@@ +1069,5 @@
> +        if (SimpleTest._expectingExplicitFinish) {
> +            SimpleTest.ok(true, "[SimpleTest.finish()] waitForExplicitFinish() has been invoked");
> +        }
> +        else {
> +            SimpleTest.ok(false, "[SimpleTest.finish()] waitForExplicitFinish() has not been invoked");

We can simplify this to:
SimpleTest.ok(SimpleTest._expectingExplicitFinish, "[SimpleTest.finish()] waitForExplicitFinish() has not been invoked");
Attachment #8830650 - Flags: review?(josh) → review-
(Assignee)

Comment 36

a year ago
Created attachment 8831042 [details] [diff] [review]
BugFix682901.patch

Thanks Josh! Hope this works. :)
Attachment #8830650 - Attachment is obsolete: true
Attachment #8831042 - Flags: review?(josh)
Comment on attachment 8831042 [details] [diff] [review]
BugFix682901.patch

Review of attachment 8831042 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good! Do you have access to the tryserver yet? If not, you should apply for it by following https://www.mozilla.org/hacking/committer/ so you can submit this patch and see if any tests report failures.
Attachment #8831042 - Flags: review?(josh) → review+
(Assignee)

Comment 38

a year ago
Hi Josh! I'm unable to push to try. I'm getting some authentication error due to public keys.
Flags: needinfo?(josh)
Have you created a `~/.ssh/config` file that contains
Host hg.mozilla.org
User dwivedi.aman96@gmail.com
Flags: needinfo?(josh) → needinfo?(dwivedi.aman96)
(Assignee)

Comment 40

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3554fbebb664f1b80d5a3a40c96059c11ac1a64e

I pushed to try. Do I need to add any new jobs on the Treeherder?
Flags: needinfo?(dwivedi.aman96) → needinfo?(josh)
(Assignee)

Updated

a year ago
Flags: needinfo?(josh)
Yes, adding jobs for mochitests would be valuable. M, M-e10s, tc-M, and tc-M-e10s should be what you're looking for.
(Assignee)

Comment 42

a year ago
Seems the tests have failed. What to do next?
Flags: needinfo?(josh)
The tests that failed will need to be modified. Either they are calling SimpleTest.finish() in a test that does not need asynchronous behaviour (in which case we need to remove the call to SimpleTest.finish), or they are executing SimpleTest.finish() before they have called SimpleTest.waitForExplicitFinish(), so they need to be modified to call it sooner.
Flags: needinfo?(josh)
You should be able to replicate the problem (and verify that it's fixed) by running ./mach mochitest path/to/test.html
(Assignee)

Comment 45

a year ago
Created attachment 8836505 [details]
test_result.txt

Hi Josh! I ran all the tests in Harness_sanity separately and all did pass. I have done a full mochitest of Harness_sanity using './mach mochitest testing/mochitest/tests/Harness_sanity'. Here attaching the log. Some tests have failed. How to fix these as they do not show any failures when they are run separately?
Attachment #8836505 - Flags: feedback?(josh)
Ah, looks like the failing test is test_finish_called_before_waitForExplicitFinish.html because it ends up trying to call SimpleTest.finish() twice (due to the window's load event handler that calls it). We should be able to fix that by making the test look this like:
SimpleTest.expectExplicitFinish();
SimpleTest.finish();
SimpleTest.waitForExplicitFinish();
By the way, you can use http://trychooser.pub.build.mozilla.org/ to choose a list of jobs to run on a try push without having to add them through the treeherder interface manually.
(Assignee)

Comment 50

a year ago
Hi Josh! Tests have failed again. Most of them show 

'TEST UNEXPECTED FAIL'- [SimpleTest.finish()] waitForExplicitFinish() has not been invoked.
Flags: needinfo?(josh)
I explained what to do about that in comment 43.
Flags: needinfo?(josh)
(Assignee)

Comment 52

a year ago
Created attachment 8837930 [details] [diff] [review]
bugfix.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=281ee95eaee0f6068b4ab6c6b9c80c948e89175b
Attachment #8831042 - Attachment is obsolete: true
Attachment #8836505 - Attachment is obsolete: true
Attachment #8836505 - Flags: feedback?(josh)
Attachment #8837930 - Flags: review?(josh)
I apologize for the delay; the past couple weeks were exceedingly busy for me, and I'm now traveling with unreliable internet. The try push shows that at least some changes appear to be incorrect, such as dom/media/tests/mochitest/test_dataChannel_basicVideo.html and a number of other tests in dom/media. Any results that have "Result logged after SimpleTest.finish()" mean that the test is continuing to execute after SimpleTest.finish() was called, and that means that SimpleTest.finish() needs to execute later than it does.
I will attempt to review these changes over the next few days.
Comment on attachment 8837930 [details] [diff] [review]
bugfix.patch

Review of attachment 8837930 [details] [diff] [review]:
-----------------------------------------------------------------

Most of these changes look good! We can reduce the number of test files that need changing by a lot if we make a few specific changes to files that they import, instead. I would like to see what the patch looks like after making those changes and reverting the unnecessary changes. Please do another try push after making the requested changes, too!

::: devtools/client/shared/components/reps/test/mochitest/test_reps_array.html
@@ +17,4 @@
>  <script src="head.js" type="application/javascript;version=1.8"></script>
>  <script type="application/javascript;version=1.8">
>  "use strict";
> +SimpleTest.waitForExplicitFinish();

Instead of adding this to each test file in devtools/client/shared/components/reps/test/mochitest/, let's add it to head.js. This should let us revert the changes to all the test files.

::: dom/console/tests/test_bug978522.html
@@ +25,4 @@
>    });
>  
>    SimpleTest.waitForExplicitFinish();
> +  SimpleTest.finish();

This change is not correct. Instead, we should call waitForExplicitFinish before console.log.

::: dom/events/test/test_bug667919-1.html
@@ +33,4 @@
>  event.initDeviceOrientationEvent('deviceorientation', true, true, 1.5, 2.25, 3.667, true);
>  window.dispatchEvent(event);
>  SimpleTest.waitForExplicitFinish();
> +SimpleTest.finish();

We should call waitForExplicitFinish sooner, rather than moving SimpleTest.finish.

::: dom/indexedDB/test/test_bug937006.html
@@ +21,4 @@
>      setTimeout(indexedDB.deleteDatabase.bind(indexedDB), 0, 'x');
>      setTimeout(function() {
>        ok(true, "Still alive");
> +      SimpleTest.waitForExplicitFinish();

We should be calling this before runTest instead.

::: dom/media/tests/mochitest/identity/identityPcTest.js
@@ +1,2 @@
>  function identityPcTest(remoteOptions) {
> +  SimpleTest.waitForExplicitFinish();

This should not be necessary if we modify dom/media/tests/mochitest/head.js.

::: dom/media/tests/mochitest/identity/test_getIdentityAssertion.html
@@ +28,4 @@
>  
>  var test;
>  function theTest() {
> +  SimpleTest.waitForExplicitFinish();

Instead, what if we add this to dom/media/tests/mochitest/head.js inside setupEnvironment?

::: dom/media/tests/mochitest/identity/test_loginNeeded.html
@@ +41,4 @@
>  }
>  
>  function theTest() {
> +  SimpleTest.waitForExplicitFinish();

This (and other changes to tests in dom/media/tests/mochitest) should not be necessary if we change head.js.

::: dom/tests/mochitest/bugs/test_DOMWindowCreated_chromeonly.html
@@ +7,4 @@
>    <p id="display"></p>
>  
>    <script type="application/javascript">
> +  

This change should not be necessary. We should investigate what happens if we revert the changes to this file.

::: layout/mathml/tests/test_opentype-fraction.html
@@ +97,4 @@
>  
>          ok(almostEqual(getBox("d9").bottom - getBox("dref").bottom, ref*3),
>             "Bad FractionDenominatorDisplayStyleShiftDown");
> +        SimpleTest.waitForExplicitFinish();

Even though the effect is the same, this should be called earlier (up near `var epsilon = 5;`).

::: netwerk/test/mochitests/test_viewsource_unlinkable.html
@@ +12,4 @@
>    SimpleTest.doesThrow(function() {
>      window.open('view-source:' + location.href, "_blank");
>    }, "Trying to access view-source URL from unprivileged code should throw.");
> +  SimpleTest.waitForExplicitFinish();

Let's call this outside of runTest for clarity.

::: security/manager/ssl/tests/mochitest/mixedcontent/test_cssBefore1.html
@@ +19,4 @@
>    <script class="testbody" type="text/javascript">
>    /* import-globals-from mixedContentTest.js */
>    "use strict";
> +  SimpleTest.waitForExplicitFinish();

Rather than adding SimpleTest.waitForExplicitFinish to each of the tests in security/manager/ssl/tests/mochitest/mixedcontent/, let's add it to mixedContentTest.js instead. This should allow us to revert the change to each of the mixedcontent/ test files.

::: testing/mochitest/tests/Harness_sanity/test_finish_called_before_waitForExplicitFinish.html
@@ +19,5 @@
> +<script class="testbody" type="text/javascript">
> +/** Test for Bug 682901 **/
> +SimpleTest.expectExplicitFinish();
> +SimpleTest.finish();
> +SimpleTest.waitForExplicitFinish();

No need for this waitForExplicitFinish.
Attachment #8837930 - Flags: review?(josh) → review-
(Assignee)

Comment 56

11 months ago
Created attachment 8848053 [details] [diff] [review]
bugfix.patch

Thanks Josh! Sorry for the delay. I had to revert many of the changes as suggested by you. 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=13416ebe3602c3f6d23087c7658ec71451105476
Attachment #8837930 - Attachment is obsolete: true
Attachment #8848053 - Flags: review?(josh)

Comment 57

11 months ago
Comment on attachment 8848053 [details] [diff] [review]
bugfix.patch

Review of attachment 8848053 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking better! The try run still shows a bunch of test failures which you will need to investigate; please run them locally (./mach mochitest path/to/test.html) to ensure that they're passing after making changes to them. Additionally, there are a lot of files included in this patch which used to have meaningful changes, and now only contain leftover indentation on blank lines. This makes the patch harder to review; please look at the diff locally and edit the files to remove the changes that only affect whitespace. I stopped commenting on those files about halfway through. Thanks for following up on my previous review!

::: dom/console/tests/test_bug978522.html
@@ +24,5 @@
>        });
>      }
>    });
>  
> +  SimpleTest.finish();

We should leave the SimpleTest.finish() call where it was originally.

::: dom/events/test/test_bug667919-1.html
@@ +33,4 @@
>  event = document.createEvent("DeviceOrientationEvent");
>  event.initDeviceOrientationEvent('deviceorientation', true, true, 1.5, 2.25, 3.667, true);
>  window.dispatchEvent(event);
> +SimpleTest.finish();

We should leave the SimpleTest.finish() call where it was originally.

::: dom/media/tests/mochitest/head.js
@@ +4,4 @@
>  
>  "use strict";
>  
> +SimpleTest.waitForExplicitFinish();

This is redundant with the call in setupEnvironment. Let's remove this one.

@@ +307,4 @@
>  var testConfigured = new Promise(r => setTestOptions = r);
>  
>  function setupEnvironment() {
> +  SimpleTest.waitForExplicitFinish();

We should move this after the check for window.SimpleTest on the next line.

::: dom/media/tests/mochitest/identity/test_fingerprints.html
@@ -102,4 @@
>      });
>  }
>  
> -SimpleTest.waitForExplicitFinish();

This test doesn't load head.js, so we need to leave this here.

::: dom/media/tests/mochitest/identity/test_idpproxy.html
@@ -169,4 @@
>      .then(() => SimpleTest.finish());
>  }
>  
> -SimpleTest.waitForExplicitFinish();

This test doesn't load head.js, so we need to leave this alone.

::: dom/media/tests/mochitest/test_dataChannel_noOffer.html
@@ +20,4 @@
>      pc.createOffer(options).then(offer => {
>        ok(!offer.sdp.includes("m=application"),
>          "m=application is not contained in the SDP");
> +      

Let's undo this whitespace change.

::: dom/media/tests/mochitest/test_getUserMedia_bug1223696.html
@@ +18,4 @@
>    runTest(() => Promise.resolve()
>      .then(() => getUserMedia({audio:true, video: true})).then(stream => {
>        info("Test addTrack()ing a video track to an audio-only gUM stream");
> +      

Undo this whitespace change.

::: dom/media/tests/mochitest/test_ondevicechange.html
@@ -42,4 @@
>  var videoTracks;
>  
>  SimpleTest.requestCompleteLog();
> -SimpleTest.waitForExplicitFinish();

This test doesn't use head.js, so we need to leave this here.

::: dom/media/tests/mochitest/test_peerConnection_bug825703.html
@@ +40,4 @@
>  
>  // This is a test of the iceServers parsing code + readable errors
>  runNetworkTest(() => {
> +  SimpleTest.waitForExplicitFinish();

Isn't this unnecessary with the change to head.js?

::: dom/tests/mochitest/bugs/test_DOMWindowCreated_chromeonly.html
@@ +7,4 @@
>    <p id="display"></p>
>  
>    <script type="application/javascript">
> +  SimpleTest.waitForExplicitFinish();  

nit: remove the trailing whitespace on this line.

::: security/manager/ssl/tests/mochitest/mixedcontent/test_bug329869.html
@@ +9,4 @@
>    <script class="testbody" type="text/javascript">
>    /* import-globals-from mixedContentTest.js */
>    "use strict";
> +  

nit: remove this trailing whitespace.

::: security/manager/ssl/tests/mochitest/mixedcontent/test_bug383369.html
@@ +11,4 @@
>    /* global sendAsyncMessage */
>    /* import-globals-from mixedContentTest.js */
>    "use strict";
> +  

nit: remove this trailing whitespace.

::: security/manager/ssl/tests/mochitest/mixedcontent/test_bug455367.html
@@ +13,4 @@
>    <script class="testbody" type="text/javascript">
>    /* import-globals-from mixedContentTest.js */
>    "use strict";
> +  

nit: remove this trailing whitespace.

::: security/manager/ssl/tests/mochitest/mixedcontent/test_bug472986.html
@@ +13,4 @@
>    <script class="testbody" type="text/javascript">
>    /* import-globals-from mixedContentTest.js */
>    "use strict";
> +  

nit: remove this trailing whitespace.

::: security/manager/ssl/tests/mochitest/mixedcontent/test_bug477118.html
@@ +13,4 @@
>    <script class="testbody" type="text/javascript">
>    /* import-globals-from mixedContentTest.js */
>    "use strict";
> +  

nit: remove this trailing whitespace.

::: security/manager/ssl/tests/mochitest/mixedcontent/test_bug521461.html
@@ +13,4 @@
>    <script class="testbody" type="text/javascript">
>    /* import-globals-from mixedContentTest.js */
>    "use strict";
> +  

nit: remove this trailing whitespace.

::: security/manager/ssl/tests/mochitest/mixedcontent/test_cssBefore1.html
@@ +19,4 @@
>    <script class="testbody" type="text/javascript">
>    /* import-globals-from mixedContentTest.js */
>    "use strict";
> +  

nit: remove this trailing whitespace.

::: security/manager/ssl/tests/mochitest/mixedcontent/test_cssContent1.html
@@ +20,4 @@
>    <script class="testbody" type="text/javascript">
>    /* import-globals-from mixedContentTest.js */
>    "use strict";
> +  

nit: remove this trailing whitespace.

::: security/manager/ssl/tests/mochitest/mixedcontent/test_cssContent2.html
@@ +13,4 @@
>    <script class="testbody" type="text/javascript">
>    /* import-globals-from mixedContentTest.js */
>    "use strict";
> +  

nit: remove this trailing whitespace.

::: toolkit/components/passwordmgr/test/mochitest/test_xhr_2.html
@@ +22,4 @@
>   * 2. connect authenticate.sjs that this time expects differentuser2:pass2 password
>   *    we must use the creds that are provided to the xhr witch are different and expected
>   */
> + 

nit: remove this trailing whitespace.
Attachment #8848053 - Flags: review?(josh) → review-
You need to log in before you can comment on or make changes to this bug.