Closed Bug 1253825 Opened 8 years ago Closed 8 years ago

Intermittent chrome/content/panels/test/index.html | should load the tests without errors - AssertionError: expected 'Error: Cannot clear timer: timer created with setInterval() but cleared with clearTimeout()' to be undefinedAssertionError@http://localho

Categories

(Hello (Loop) :: General, defect, P1)

Product:
Hello (Loop)
Component:
General
Type:
defect

Tracking

(firefox47 wontfix, firefox48 fixed, firefox49 fixed, firefox50 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla50
Iteration:
50.1 - Jun 20
Tracking Flags:
Tracking Status
firefox47 --- wontfix
firefox48 --- fixed
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: philor, Assigned: standard8)

References

Details

(Keywords: intermittent-failure, Whiteboard: [btpp-fix-now][47])

Attachments

(1 file)

[loop] Standard8:bug-1253825-inter > mozilla:master
40 bytes, text/x-github-pull-request
mancas
: review+
Details | Review
Component: Marionette → General
Product: Testing → Hello (Loop)
Version: Trunk → unspecified
Mark, do you have any sense of whether this is a real failure inside a test, or us simplying timing out an entire suite too soon, or something?
Rank: 15
Flags: needinfo?(standard8)
Priority: -- → P1
Whiteboard: [btpp-followup-2016-03-16]
Rank: 15 → 19
So it looks like this is Sinon, detecting what it thinks was an error condition.  We did upgrade Sinon in this timeframe, but it's not clear to me even after some Changelog grovelling exactly when this detection was added to Sinon.  

There are plenty of clearTimeouts in our codebase, both in libraries, tests, and code.  What's frustrating is that chai does not (is not able to?) obtain a real stack (using Error.stack) from Error object it has access to.  If we had a real stack, it would be trivial to figure out whether this bug was real or not.

We could potentially back out the sinon change diagnostically to see if the intermittent goes away, but if it does, that might just be papering over an error that's already there.  Still, not being able to trust our tests seems worse, so I guess I'd vote for that.  Standard8, anything to add here, or should we just try that?
Flags: needinfo?(standard8) → needinfo?(dmose)
Flags: needinfo?(dmose)
09:31:30 <Standard8> 1.1.11 went out on the Friday
09:31:50 <Standard8> https://brasstacks.mozilla.com/orangefactor/?display=Bug&tree=trunk&startday=2016-03-03&endday=2016-03-10&bugid=1253825
09:32:48 <Standard8> https://github.com/mozilla/loop/blob/release/CHANGELOG.md
09:36:02 <Standard8> previous version was 1.1.9

More info from Mark about the changes, just so we don't lose them.  Note that the changes we made did involve _removing_ timeouts from loopapi-client and its tests, but that doesn't seem wildly likely to have cause this.
I don't see a sinon bump in the timeframe here:

http://hg.mozilla.org/mozilla-central/filelog/dd1abe874252/browser/extensions/loop/chrome/content/shared/test/vendor/sinon.js

So I'm not sure what you're seeing.
My mistake; sorry.  I can't see anything in the changed timeframe that would likely cause this failure.
We should at least get some additional eyes on this and go through some of the flows and see if we can work out what's happening or not.
Whiteboard: [btpp-followup-2016-03-16] → [btpp-fix-now][47]
Rank: 19 → 22
I do not know why there's been a sudden increase in Windows failures for this. We've not changed our code recently, so maybe caused by something else - probably something has changed timing/performance.

I've taken a bit more of a look at it, here's some findings:

- The error message is actually wrong, see https://github.com/sinonjs/lolex/pull/68
- The error message should be:

"Cannot clear timer: timer created with setTimeout() but cleared with clearInterval()"

- This happens in the panels tests "panels/test/index.html"
- The only clearInterval that are called are in views.jsx (ConversationToolbar)
- For the AppControllerView test, we're not using fake timers but the clearInterval etc still get called (because we don't have a way of stubbing out sub-views atm).

My best guess therefore, is that the AppControllerView tests are accidentally setting timers with setInterval/setTimeout.

These timers are then kicking in after 4 or 6 seconds, which also causes clearInterval to be called.

If we're in the middle of another test cycle which is using fake timers, then we run the risk of calling the fake timers version of clearInterval on a browser-supplied timer ID for the setInterval, which could easily cause conflicts, and the error shown.

I suspect this just started when it did because of changes in performance or timing of how the code runs.

Patch coming up.
Comment on attachment 8761944 [details] [review]
[loop] Standard8:bug-1253825-inter > mozilla:master

Patch to fix it. I've checked there's no other places this needs stubbing for this test.

Once I've got r+ I'll push it straight to fx-team as well, to help start fixing the intermittents there.
Attachment #8761944 - Flags: review?(fernando.campo)
Attachment #8761944 - Flags: review?(edilee)
Attachment #8761944 - Flags: review?(b.mcb)
Comment on attachment 8761944 [details] [review]
[loop] Standard8:bug-1253825-inter > mozilla:master

r=me

Thanks!
Attachment #8761944 - Flags: review?(fernando.campo)
Attachment #8761944 - Flags: review?(edilee)
Attachment #8761944 - Flags: review?(b.mcb)
Attachment #8761944 - Flags: review+
Assignee: nobody → standard8
Iteration: --- → 50.1
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/9b508b2c3d87
Fix intermittent issue with clearInterval attempting to clear the wrong timers, caused by missing stubs. r=mancas
https://hg.mozilla.org/mozilla-central/rev/9b508b2c3d87
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Blocks: 1281619
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: