Rework RTCDTMFSender-ontonechange tests to use cumulative time when checking whether tonechange events are happening too early
Categories
(Core :: WebRTC: Signaling, enhancement, P2)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox67 | --- | fixed |
People
(Reporter: bwc, Assigned: bwc)
Details
Attachments
(1 file)
The reason why these tests are flaky right now seems to be because when there is a delay between setting the timer for the next tone, and calling the tonechange event handler, the next time the tonechange handler is called will appear to happen too soon. An example:
0ms: Schedule next tone to fire in 170ms.
... some processing delay
2ms: tonechange handler called, remembers time, expects next call in 170ms, at t=172ms
170ms: Next tone fires, right on time
170ms: tonechange handler called, and test is sad
|
Assignee | |
Comment 1•7 years ago
|
||
This is a bit of a mess. The way this test is written, all of the test-cases run simultaneously, because waiting for tone events to fire is kinda time-consuming. Unfortunately, that means that we're creating a bunch of PCs (which involves quite a bit of time) which significantly delays the first tonechange callbacks (by about 200ms). This padding is more than enough to hide errors in test-cases that do fire events way too early.
|
|
Assignee | |
Comment 2•7 years ago
|
||
|
|
Assignee | |
Comment 3•7 years ago
|
||
|
|
Assignee | |
Comment 4•7 years ago
|
||
Almost works. Seeing intermittent nullptr crashes on linux32 debug only. Looking into it.
|
|
Assignee | |
Comment 5•7 years ago
|
||
Ok... seeing a nullptr crash entirely in binding code. |mImpl| is null here:
void
RTCRtpTransceiver::SetStopped(ErrorResult& aRv, JS::Realm* aRealm)
{
return mImpl->SetStopped(aRv, aRealm);
}
The binding code never nulls out |mImpl| (except in d'tor code maybe), but the cycle collector might. This is very weird.
|
|
Assignee | |
Comment 6•7 years ago
|
||
I should point out that this happens only on the non-e10s tests; this seems to be why it happens only on linux32 debug. It would be nice if there were some way to coerce try into running some non-e10s tests on ASAN...
|
|
Assignee | |
Comment 7•7 years ago
|
||
|
|
Assignee | |
Comment 8•7 years ago
|
||
|
|
Assignee | |
Comment 9•7 years ago
|
||
|
|
Assignee | |
Comment 10•7 years ago
|
||
FWIW, I cannot reproduce this (on non-e10s) on linux ASAN locally.
|
|
Assignee | |
Comment 11•7 years ago
|
||
Ok, bug 1531448 is definitely the culprit here. I wonder why it is not safe to call that code in close()?
|
|
Assignee | |
Comment 12•7 years ago
|
||
|
|
Assignee | |
Comment 13•7 years ago
|
||
|
|
Assignee | |
Comment 14•7 years ago
|
||
Dunno why mach try is putting links in here that don't work, here is one that does.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f57adc1a45ec
|
|
Assignee | |
Comment 15•7 years ago
|
||
Ok, so the crash in comment 5 is happening when we close the pc due to the window going away. Evidently, we're in the middle of a cycle collection, and some of the stuff in the cycle has already been cleaned up. This particular problem is easy enough to work around (see patches in comment 14), but I'm left with a nagging feeling that this sort of bug is liable to happen again.
|
|
Assignee | |
Comment 16•7 years ago
|
||
|
|
Assignee | |
Comment 17•7 years ago
|
||
|
||
Comment 18•7 years ago
|
||
|
||
Comment 19•7 years ago
|
||
| bugherder | ||
Description
•