Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

getStats fails in Firefox (freedom integration test) #4

Closed
agallant opened this issue Apr 8, 2015 · 10 comments · Fixed by #6
Closed

getStats fails in Firefox (freedom integration test) #4

agallant opened this issue Apr 8, 2015 · 10 comments · Fixed by #6

Comments

@agallant
Copy link
Contributor

agallant commented Apr 8, 2015

Specifically this assertion: https://github.com/freedomjs/freedom/blob/master/spec/providers/coreIntegration/rtcpeerconnection.integration.src.js#L192

I've done a bit of investigation and think that the fix probably comes back to this dependency - if that ends up being erroneous I'll close this out.

@willscott
Copy link
Owner

@bemasc - I think you added the getStats test here. Is it a problem that
firefox doesn't return stats at the moment?

On Wed, Apr 8, 2015 at 12:43 PM, soycode notifications@github.com wrote:

Specifically this assertion:
https://github.com/freedomjs/freedom/blob/master/spec/providers/coreIntegration/rtcpeerconnection.integration.src.js#L192

I've done a bit of investigation and think that the fix probably comes
back to this dependency - if that ends up being erroneous I'll close this
out.


Reply to this email directly or view it on GitHub
#4.

@bemasc
Copy link

bemasc commented Apr 9, 2015

That does sound like a problem, but it doesn't reproduce for me. (Also, it shouldn't affect uProxy, because uProxy no longer uses getStats.)

@agallant
Copy link
Contributor Author

agallant commented Apr 9, 2015

Hrm I thought I was reproducing this on Shippable but can't any more, so I'll close for now. Sorry for the false alarm.

@agallant agallant closed this as completed Apr 9, 2015
@agallant
Copy link
Contributor Author

agallant commented Apr 9, 2015

Err I lied, and/or Shippable is playing tricks on me. This issue is live on the most recent Shippable build: https://app.shippable.com/builds/55256fd1bd14530c001eb753

>> integration: core.rtcpeerconnection getStats works failed
>> Expected 0 to be greater than 0.

If this test isn't expected to pass in Firefox then I'd suggest adding at least some sort of "if firefox then don't try to do this test" logic somewhere, so we can get Shippable nice and green.

@agallant agallant reopened this Apr 9, 2015
@bemasc
Copy link

bemasc commented Apr 9, 2015

I expect this test to pass (and on my machine it does!), so we should investigate.

@agallant
Copy link
Contributor Author

I'm seeing error messages like this:

console.info: freedom_for_firefox_tests: [failed]: integration: core.rtcpeerconnection getStats works
JavaScript error: , line 0: TypeError: Return value of mozRTCPeerConnection.createOffer does not implement interface Promise.
JavaScript error: , line 0: TypeError: Return value of mozRTCPeerConnection.setLocalDescription does not implement interface Promise.
JavaScript error: , line 0: TypeError: Return value of mozRTCPeerConnection.setRemoteDescription does not implement interface Promise.
JavaScript error: , line 0: TypeError: Return value of mozRTCPeerConnection.addIceCandidate does not implement interface Promise.
JavaScript error: , line 0: TypeError: Return value of mozRTCPeerConnection.addIceCandidate does not implement interface Promise.
JavaScript error: , line 0: TypeError: Return value of mozRTCPeerConnection.addIceCandidate does not implement interface Promise.
JavaScript error: , line 0: TypeError: Return value of mozRTCPeerConnection.createAnswer does not implement interface Promise.
JavaScript error: , line 0: TypeError: Return value of mozRTCPeerConnection.setLocalDescription does not implement interface Promise.
JavaScript error: , line 0: TypeError: Return value of mozRTCPeerConnection.setRemoteDescription does not implement interface Promise.
JavaScript error: , line 0: TypeError: Return value of mozRTCPeerConnection.addIceCandidate does not implement interface Promise.
JavaScript error: , line 0: TypeError: Return value of mozRTCPeerConnection.addIceCandidate does not implement interface Promise.
JavaScript error: , line 0: TypeError: Return value of mozRTCPeerConnection.addIceCandidate does not implement interface Promise.

May be spurious as I see similar errors on tests that are passing. The main difference for getStats is it then leads to what looks like a Firefox-crashing error: JavaScript error: resource://jid1-mkagayemb0e5nq-at-jetpack/freedom_for_firefox_tests/data/spec.jsm, line 2000: NS_ERROR_UNEXPECTED:

Corresponding to: https://github.com/freedomjs/freedom/blob/e9f1f140b6b2a98eca77542608cf7cce2cc7073d/providers/core/core.rtcpeerconnection.js#L92

@agallant
Copy link
Contributor Author

From some investigation, looks like the Firefox getStats just returns an iterable RTCStatsReport, rather than taking a callback/resolving a promise. I think this adapter may still be the right place for a fix, if so then pull request will be forthcoming.

@agallant
Copy link
Contributor Author

Or it does return something with "then" and "catch", but it's just not playing nice.

@agallant
Copy link
Contributor Author

So I think I've finally dug down to a fairly informative error:

JavaScript error: resource://gre/components/PeerConnection.js, line 244: TypeError:
this.__DOM_IMPL__.wrappedJSObject is not an object

This refers to: https://dxr.mozilla.org/mozilla-central/source/dom/media/PeerConnection.js?from=PeerConnection.js#244

So I'm guessing there's something funky about this.__DOM_IMPL__, particularly when running in a web worker. I can see two possible approaches - either keep digging and hope to find a fix in Firefox, or possibly switch to a polyfill of some sort (though I'm not sure if it's possible to just fill getStats and keep the rest of the implementation).

Of course this is all somewhat strange in light of the test apparently passing on other machines. As of now it is failing on my machine (OSX 10.9.5 Firefox 37) and also on Travis (I think effectively some-sort-of-Linux/Firefox 37). If we can reproduce success on other machines that may change things. @bemasc, does it still pass for you and on what platform?

@bemasc
Copy link

bemasc commented Apr 13, 2015

Works for me on FF36, Linux. I guess FF must have changed something.

On Mon, Apr 13, 2015 at 4:55 PM, soycode notifications@github.com wrote:

So I think I've finally dug down to a fairly informative error:

JavaScript error: resource://gre/components/PeerConnection.js, line 244: TypeError:
this.DOM_IMPL.wrappedJSObject is not an object

This refers to:
https://dxr.mozilla.org/mozilla-central/source/dom/media/PeerConnection.js?from=PeerConnection.js#244

So I'm guessing there's something funky about this.DOM_IMPL,
particularly when running in a web worker. I can see two possible
approaches - either keep digging and hope to find a fix in Firefox, or
possibly switch to a polyfill of some sort (though I'm not sure if it's
possible to just fill getStats and keep the rest of the implementation).

Of course this is all somewhat strange in light of the test apparently
passing on other machines. As of now it is failing on my machine (OSX
10.9.5 Firefox 37) and also on Travis (I think effectively
some-sort-of-Linux/Firefox 37). If we can reproduce success on other
machines that may change things. @bemasc https://github.com/bemasc,
does it still pass for you and on what platform?


Reply to this email directly or view it on GitHub
#4 (comment)
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants