1025352 - Race condition in checkMediaTracks

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Nils Ohlmeier [:drno]]

Tests can currently pass the checkMediaTracks verification, but still later get terminated by the addStreamTimeout.

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: bug_1025352_fix_checkMediaTracks_race_condition.patch

Rather then trying to prevent the race condition this patch should make the ok() test in the timeout function succeed in case the media tracks got verified before the timeout handler got setup.

Comment 2

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

In the original code,

var addStreamTimeout = null;

    function _checkMediaTracks(constraintsRemote, onSuccess) {

      if (self.addStreamTimeout !== null) {
        clearTimeout(self.addStreamTimeout);

But where the timeout is set...

 addStreamTimeout = setTimeout(function () {
        ok(false, self + " checkMediaTracks() timed out waiting for onaddstream event to fire");

...which modified the var addStreamTimeout at the top.

Isn't the problem that we're checking self.addStreamTimeout instead of the var addStreamTimeout you declared? 

My guess is that the check passes (because undefined !== null) but then we proceed to clearTimeout(undefined) because it's still using self.

I suspect we might just need to remove the self. from those two usages in _checkMediaTracks().

Comment 3

[Nils Ohlmeier [:drno]]

(In reply to Geo Mealer [:geo] from comment #2)
> Isn't the problem that we're checking self.addStreamTimeout instead of the
> var addStreamTimeout you declared? 
> 
> My guess is that the check passes (because undefined !== null) but then we
> proceed to clearTimeout(undefined) because it's still using self.
> 
> I suspect we might just need to remove the self. from those two usages in
> _checkMediaTracks().

You are absolutely right. Thanks for catching that!

New patch coming...

Comment 4

[Nils Ohlmeier [:drno]]

Attachment: bug_1025352_fix_checkMediaTracks_race_condition.patch

This patch fixes a couple of issues around the checkMediaTracks timeout.

Comment 5

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 8440878 [details] [diff] [review]
bug_1025352_fix_checkMediaTracks_race_condition.patch

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

Looks fine. The logic around onAddStreamFired is starting to get a little complex, though, and you have a lot commented with a "TODO: Remove..." making me wonder how much we want to lean on the flag.

r+, but something we might want to think about refactoring a bit.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Can you please run this through Try and post a link to the results? Thanks :)

Comment 7

[Nils Ohlmeier [:drno]]

Attachment: bug_1025352_fix_checkMediaTracks_race_condition.patch

Updated against latest m-c.
Carrying forward r+=geo

I thought this is low risk enough to have only try it locally, but it looks like I'm getting too lazy ;-)
Try run: https://tbpl.mozilla.org/?tree=Try&rev=022ead66723d

Comment 8

[Nils Ohlmeier [:drno]]

I don't see any new problems in the test run :-)

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3be598cf0653

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3be598cf0653