Closed Bug 985274 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: WebRTC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox29 --- fixed
firefox30 --- fixed
firefox31 --- fixed
firefox-esr24 --- unaffected
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: drno, Assigned: drno)

References

Details

Attachments

(1 file, 6 obsolete files)

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.
Depends on: 971740, 960488, 960610, 982550
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?
Flags: needinfo?(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) })
Flags: needinfo?(jgriffin)
Good catch, you're calling unexpectedCallbackAndFinish() here with no arguments before even calling getUserMedia(), passing in the result of whatever unexpectedCallbackAndFinish() returns.
Attached patch bug985274.patch (obsolete) — Splinter Review
A small patch so we can get a better bead on the failure.
Attachment #8395307 - Flags: review?(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.
Attachment #8395307 - Flags: review?(jsmith) → review+
Thanks a lot Jonathan and David.
I'm working on a patch which fixes all the wrong calls to unexpectedCallbackAndFinish() in our tests.
Assignee: nobody → drno
Attachment #8395734 - Flags: review?(jsmith)
Attachment #8395734 - Flags: review?(dclarke)
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
Attachment #8395734 - Flags: review?(jsmith) → review-
Comment on attachment 8395734 [details] [diff] [review]
Fix incorect calls of unexpectedCallbackAndFinish in all WebRTC tests

ditto
Attachment #8395734 - Flags: review?(dclarke) → review-
Attached patch Bug985274.patch (obsolete) — Splinter Review
This seems to be correct.
Attachment #8395734 - Attachment is obsolete: true
Attachment #8395940 - Flags: review?(jsmith)
Attachment #8395940 - Flags: review?(drno)
Attachment #8395307 - Attachment is obsolete: true
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?
Attachment #8395940 - Flags: review?(drno) → review-
Forgot to mention that in my review, but David's last patch is missing a fix for test_dataChannel_noOffer.html.
Attachment #8395940 - Attachment is obsolete: true
Attachment #8395940 - Flags: review?(jsmith)
(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.
Attachment #8396000 - Flags: review?(jsmith)
Attachment #8396000 - Flags: review?(dclarke)
(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+
Attachment #8396000 - Flags: review?(dclarke) → review-
(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"));
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 ?
(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.
(In reply to dclarke@mozilla.com  [: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 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.
Attachment #8396000 - Flags: review?(jsmith) → review-
Attachment #8396000 - Attachment is obsolete: true
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.
(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!
Attachment #8396125 - Attachment is obsolete: true
Attachment #8396130 - Attachment is obsolete: true
Comment on attachment 8396132 [details] [diff] [review]
Ensure Generated Callback Handles Strings v1

https://tbpl.mozilla.org/?tree=Try&rev=4648d4ca5643
Attachment #8396132 - Flags: review?(drno)
Comment on attachment 8396132 [details] [diff] [review]
Ensure Generated Callback Handles Strings v1

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

lgtm
Attachment #8396132 - Flags: review?(drno) → review+
https://hg.mozilla.org/mozilla-central/rev/ef3e291155ae
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Please request Aurora approval for this as well.
Flags: needinfo?(drno)
Actually, this is test-only. I'll get it in the morning.
Flags: needinfo?(drno)
Whiteboard: [checkin-needed-aurora]
You need to log in before you can comment on or make changes to this bug.