Closed Bug 995266 Opened 5 years ago Closed 5 years ago

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

Categories

(Testing :: Mochitest, defect)

defect
Not set

Tracking

(firefox29 fixed, firefox30 fixed, firefox31 fixed)

RESOLVED FIXED
mozilla31
Tracking Status
firefox29 --- fixed
firefox30 --- fixed
firefox31 --- fixed

People

(Reporter: ttaubert, Assigned: ttaubert)

Details

Attachments

(1 file)

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.
I tried using Cu.cloneInto() for a nice one-line fix but that didn't work, it failed for some reason.
Attachment #8405460 - Flags: review?(ted)
FTR, I tested that this fix works with one of the previous failures we encountered.
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.
Attachment #8405460 - Flags: review?(ted) → review+
Component: Mochitest → BrowserTest
https://hg.mozilla.org/mozilla-central/rev/c28992b63962
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
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]
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.
Component: BrowserTest → Mochitest
You need to log in before you can comment on or make changes to this bug.