995266 - mochitest-browser leaks SimpleTest methods from previous test run until overriden

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

The mochitest-browser harness overrides SimpleTest methods with ones defined by the testScope instance. That's not cleaned up and can make the last test in a test run being accused of leaking global variables.

We have many of those failures in the last days due to m-bc chunking, let's fix this.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-995266-Prevent-mochitest-browser-harness-from-le.patch

I tried using Cu.cloneInto() for a nice one-line fix but that didn't work, it failed for some reason.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

FTR, I tested that this fix works with one of the previous failures we encountered.

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 8405460 [details] [diff] [review]
0001-Bug-995266-Prevent-mochitest-browser-harness-from-le.patch

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

Bleh. Would be nice to figure out a cleaner way to do this, but let's stop the bleeding at least.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c28992b63962

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8405460 [details] [diff] [review]
0001-Bug-995266-Prevent-mochitest-browser-harness-from-le.patch

It's quite possible I'm confused here, but isn't the "Restore original SimpleTest methods" code setting properties on a different |this| than the "Override SimpleTest methods with ours" code?

+    SIMPLETEST_OVERRIDES.forEach(function(m) {
       this.SimpleTest[m] = this[m];
     }, this.currentTest.scope);

this is setting this.currentTest.scope.SimpleTest[m]

+      SIMPLETEST_OVERRIDES.forEach(m => {
+        this.SimpleTest[m] = this.SimpleTestOriginal[m];
+      });

this is setting this.SimpleTest[m]

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

The current code is quite confusing... A few lines above the loop we do:

this.currentTest.scope.SimpleTest = this.SimpleTest;

So they are the same instances of SimpleTest. The first loop then copies values like this:

this.currentTest.scope.SimpleTest[m] = this.currentTest.scope[m];

We are thus leaking because we copy methods from the |testScope| instance to the |Tester.SimpleTest| property before each test run. I chose |this.SimpleTest| for cleanup because I thought that would be easier to understand than going through the this.currentTest.scope.SimpleTest indirection.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2d271cfe3e55
https://hg.mozilla.org/releases/mozilla-beta/rev/f11f4dda1cde