985274 - [meta] Solve Unexpected callback with message = 'undefined' bugs in WebRTC tests

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Nils Ohlmeier [:drno]]

We seem to have quite a few intermittent test failures all with the same test failure message:
  Unexpected callback with message = 'undefined'

Attempts to fill the empty unexpected callback catch statements with proper messages failed to solve this issue.

Track down and fix why the callback messages get lost in the callback stack trace.

Comment 1

[Nils Ohlmeier [:drno]]

Jonathan: as you can see for example in Bug 971740 the actual error happens somewhere in dom/media/tests/mochitest/mediaStreamPlayback.js invoked from dom/media/tests/mochitest/test_getUserMedia_gumWithinGum.html:45. The unexpectedCallbackAndFinish() function in line 45 properly catches the error, but we loose the stack trace and therefore don't really know what is going on. My attempt to put more proper messages in mediaStreamPlayback.js weren't successful.
Any advice on how to improve the logging to solve all these test failures?

Comment 2

[Jonathan Griffin (:jgriffin)]

These onerror callbacks aren't being handled correctly, so the error isn't getting passed.  For instance, at

http://mxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/test_getUserMedia_gumWithinGum.html?force=1#51

instead of 

getUserMedia({'foo':'bar'}, function success(stream) { /* blah blah */}, unexpectedCallbackAndFinish())

You need either:

getUserMedia({'foo':'bar'}, function success(stream) { /* blah blah */}, unexpectedCallbackAndFinish)

or more explicitly:

getUserMedia({'foo':'bar'}, function success(stream) { /* blah blah */}, function error(err) { unexpectedCallbackAndFinish(err) })

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Good catch, you're calling unexpectedCallbackAndFinish() here with no arguments before even calling getUserMedia(), passing in the result of whatever unexpectedCallbackAndFinish() returns.

Comment 4

[[e-mail] [:onecyrenus]]

Attachment: bug985274.patch

A small patch so we can get a better bead on the failure.

Comment 5

[Jason Smith [:jsmith]]

Comment on attachment 8395307 [details] [diff] [review]
bug985274.patch

This looks right. Now we just need to update the other files in dom/media/tests/mochitest to follow this convention.

Comment 6

[Nils Ohlmeier [:drno]]

Thanks a lot Jonathan and David.
I'm working on a patch which fixes all the wrong calls to unexpectedCallbackAndFinish() in our tests.

Comment 7

[Nils Ohlmeier [:drno]]

Attachment: Fix incorect calls of unexpectedCallbackAndFinish in all WebRTC tests

Comment 8

[Nils Ohlmeier [:drno]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=dcf768490d1d

Comment 9

[Jason Smith [:jsmith]]

Comment on attachment 8395734 [details] [diff] [review]
Fix incorect calls of unexpectedCallbackAndFinish in all WebRTC tests

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

Haven't gone through the individual files yet, but I already notice that we're missing updates to the following files here:

* pc.js
* test_peerConnection_errorCallbacks.html

Comment 10

[[e-mail] [:onecyrenus]]

Comment on attachment 8395734 [details] [diff] [review]
Fix incorect calls of unexpectedCallbackAndFinish in all WebRTC tests

ditto

Comment 11

[[e-mail] [:onecyrenus]]

Attachment: Bug985274.patch

This seems to be correct.

Comment 12

[Nils Ohlmeier [:drno]]

Comment on attachment 8395940 [details] [diff] [review]
Bug985274.patch

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

I think the better solution according to Jonathan's comment is to use the explicit and more verbose version with function names, because some of these have as many as 4 nested calls to unexpectedCallbackAndFinish().
And I'm not sure, but do function arguments get passed if you don't hand them over explicitly?

Comment 13

[Nils Ohlmeier [:drno]]

Forgot to mention that in my review, but David's last patch is missing a fix for test_dataChannel_noOffer.html.

Comment 14

[Nils Ohlmeier [:drno]]

Attachment: Fix incorect calls of unexpectedCallbackAndFinish in all WebRTC tests

(In reply to Jason Smith [:jsmith] from comment #9)
> Haven't gone through the individual files yet, but I already notice that
> we're missing updates to the following files here:
> 
> * pc.js

Thanks I added that to this patch, plus some more files David found in his patch.

> * test_peerConnection_errorCallbacks.html

In this file the unexpectedCallbackAndFinish() is actually called explicitly with arguments to verify things have failed as expected. So I don't think we need to change this file.

Comment 15

[[e-mail] [:onecyrenus]]

(In reply to Nils Ohlmeier [:drno] from comment #12)
> Comment on attachment 8395940 [details] [diff] [review]
> Bug985274.patch
> 
> Review of attachment 8395940 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think the better solution according to Jonathan's comment is to use the
> explicit and more verbose version with function names, because some of these
> have as many as 4 nested calls to unexpectedCallbackAndFinish().
> And I'm not sure, but do function arguments get passed if you don't hand
> them over explicitly?

So there are two issues here. 

#1) Nested calls, to fix this we should use promises, and that is a subject for another bug. 

#2) Function arguments getting passed if you don't hand them over explicitly. yes they do, but I guess it is a matter of preference.  I would think that we would err on the side of succinct code rather than repetitive. By changing the function names for each function, I don't think that makes the code more correct, just repetitive.  here is an example, where if you deny the request for microphone access, the error handler is called. 

https://gist.github.com/anonymous/0d9ad88e1d1193254dab

i'll punt to jason, if he review +'s. It is ok with me. And I will change to a review+

Comment 16

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Jason Smith [:jsmith] from comment #5)
> Comment on attachment 8395307 [details] [diff] [review]
> bug985274.patch
> 
> This looks right. Now we just need to update the other files in
> dom/media/tests/mochitest to follow this convention.

Oops, wrong. I think we all got fooled by the testing framework here, including me in comment 3.

Looking at http://mxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/head.js#217 I see that unexpectedCallbackAndFinish is NOT the error-callback, but a function that generates the error-callback.

So the old code was correct, we were just omitting the error message to the generator, like this:

getUserMedia({'foo':'bar'},
             function success(stream) { /* blah blah */},
             unexpectedCallbackAndFinish("getUserMedia failed"));

Comment 17

[[e-mail] [:onecyrenus]]

jan-ivar: i think in this scenario we would want to pipe up the getUserMedia failure, and not squelch it. 

maybe i am confused, but there are multiple scenarios in play here. 

#1) when you explicitly call out the error 
unexpectedCallbackAndFinish("big error");

#2) when the error is implicitly set 
getUserMedia({'foo':'bar'},
             function success(stream) { /* blah blah */},
             unexpectedCallbackAndFinish);

where unexpectedCallbackAndFinish is the Error handler for getUserMedia. 

another example would be say in the camera tests
http://mxr.mozilla.org/mozilla-central/source/dom/camera/test/test_camera.html?force=1#31 you define the error function, and use it later when testing the api: 
http://mxr.mozilla.org/mozilla-central/source/dom/camera/test/test_camera.html?force=1#208

Am I confused here ?

Comment 18

[Jason Smith [:jsmith]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #16)
> (In reply to Jason Smith [:jsmith] from comment #5)
> > Comment on attachment 8395307 [details] [diff] [review]
> > bug985274.patch
> > 
> > This looks right. Now we just need to update the other files in
> > dom/media/tests/mochitest to follow this convention.
> 
> Oops, wrong. I think we all got fooled by the testing framework here,
> including me in comment 3.
> 
> Looking at
> http://mxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/head.
> js#217 I see that unexpectedCallbackAndFinish is NOT the error-callback, but
> a function that generates the error-callback.

Right - probably should change the name of the function to mean that it's a generator, not the function itself.

> 
> So the old code was correct, we were just omitting the error message to the
> generator, like this:
> 
> getUserMedia({'foo':'bar'},
>              function success(stream) { /* blah blah */},
>              unexpectedCallbackAndFinish("getUserMedia failed"));

I don't think that will fix the problem here - that still proves that we're ignoring the error object portion of the gUM error callback.

I think the problem is actually in the generator itself:

http://hg.mozilla.org/mozilla-central/file/5c0673441fc8/dom/media/tests/mochitest/head.js#l226

This line assumes that the object retrieved is an error object. However, if you inspect the object returned in the error callback in gUM, then you'll notice it's a string, not an error object. So that means that if statement ends up returning false, since aObj.name is undefined. That means the else path - which in the original code, references the message originally passed in from unexpectedCallbackAndFinish. We passed in no parameters, which ends up making message undefined - and that's what we're seeing here in the dependent bugs.

Comment 19

[Jason Smith [:jsmith]]

(In reply to [e-mail]  [:onecyrenus] from comment #17)
> jan-ivar: i think in this scenario we would want to pipe up the getUserMedia
> failure, and not squelch it. 

Right - the ultimate goal here is that we want to see the error from the gUM error callback.

> 
> maybe i am confused, but there are multiple scenarios in play here. 
> 
> #1) when you explicitly call out the error 
> unexpectedCallbackAndFinish("big error");

This would result in "big error" getting printed.

> 
> #2) when the error is implicitly set 
> getUserMedia({'foo':'bar'},
>              function success(stream) { /* blah blah */},
>              unexpectedCallbackAndFinish);

Based on my analysis in the above comment, I think this would actually cause us to timeout, as we would just return the function object that would fire the error & end the test.

> 
> where unexpectedCallbackAndFinish is the Error handler for getUserMedia. 
> 
> another example would be say in the camera tests
> http://mxr.mozilla.org/mozilla-central/source/dom/camera/test/test_camera.
> html?force=1#31 you define the error function, and use it later when testing
> the api: 
> http://mxr.mozilla.org/mozilla-central/source/dom/camera/test/test_camera.
> html?force=1#208
> 
> Am I confused here ?

I think the point of confusion here is due to the poor naming of the function - unexpectedCallbackAndFinish is actually a generator of the error handler, not an error handler in itself. We should probably fix the naming here.

Comment 20

[Jason Smith [:jsmith]]

Comment on attachment 8396000 [details] [diff] [review]
Fix incorect calls of unexpectedCallbackAndFinish in all WebRTC tests

See the above comments - the problem is in the generator itself, not the callers to unexpectedCallbackAndFinish.

Comment 21

[Jason Smith [:jsmith]]

Attachment: Ensure Generated Callback Handles Strings

Comment 22

[Jason Smith [:jsmith]]

I'm going to clean this up a bit with naming, but the patch attached here demonstrates what I think we need to fix this problem.

Comment 23

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Jason Smith [:jsmith] from comment #18)
> Right - probably should change the name of the function to mean that it's a
> generator, not the function itself.

Good idea.

> I think the problem is actually in the generator itself:
> 
> http://hg.mozilla.org/mozilla-central/file/5c0673441fc8/dom/media/tests/
> mochitest/head.js#l226
> 
> This line assumes that the object retrieved is an error object. However, if
> you inspect the object returned in the error callback in gUM, then you'll
> notice it's a string, not an error object.

That's it!

Comment 24

[Jason Smith [:jsmith]]

Attachment: Ensure Generated Callback Handles Strings v1

Comment 25

[Jason Smith [:jsmith]]

Attachment: Ensure Generated Callback Handles Strings v1

Comment 26

[Jason Smith [:jsmith]]

Comment on attachment 8396132 [details] [diff] [review]
Ensure Generated Callback Handles Strings v1

https://tbpl.mozilla.org/?tree=Try&rev=4648d4ca5643

Comment 27

[Nils Ohlmeier [:drno]]

Comment on attachment 8396132 [details] [diff] [review]
Ensure Generated Callback Handles Strings v1

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

lgtm

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef3e291155ae

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora approval for this as well.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Actually, this is test-only. I'll get it in the morning.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0483017bfec7
https://hg.mozilla.org/releases/mozilla-beta/rev/582f0df54f08
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/3c4317e180a0