gorhill / uBO-Extra

A companion extension to uBlock Origin
GNU General Public License v3.0
654 stars 42 forks source link

uBO-extra breaks twilio-video.js sdk's webrtc on localhost #41

Closed jskrzypek closed 7 years ago

jskrzypek commented 7 years ago

uBO-extra's wrapper around RTCPeerConnection is breaking my ability to develop with Twilio's twilio-video javascript sdk (https://github.com/twilio/twilio-video.js). I lost like 4 days of dev time confused as to why this was broken before I realized that uBO-extra was wrapping RTCPeerConnection

If you want to reproduce this you can use their quickstart node app: https://github.com/TwilioDevEd/video-quickstart-node

With uBO-extra enabled you get this error, but it disappears when the extension is disabled.

this._peerConnection.addStream is not a function1 at PeerConnectionV2.addMediaStream (twilio-video.js:4052) at PeerConnectionManager._getOrCreate (twilio-video.js:4309) at getConfigurationSucceeded (twilio-video.js:4338)

It would be nice to be able to add domains to the exception lists without needing an actual source code change.

gorhill commented 7 years ago

What is the browser version?

It would be nice to be able to add domains to the exception lists without needing an actual source code change.

Not possible. If it was possible, wrapping in this extension would not be required, it could be done uBO-side. There is already a filter on uBO side to filter out WebRTC connection, it does not work if a web page uses RCPeerConnection before uBO can wrap it. uBO-Extra wraps unconditionally before any script on the page has run, this is why it can filter out WebRTC connections on some sites whereas it's not possible with filter-based uBO.

gorhill commented 7 years ago

Did you look at uBO's logger to see if there were filters potentially causing this?

jskrzypek commented 7 years ago

I'm on Chrome 58.0.3029.54 (beta).

I have localhost whitelisted in uBO so no uBO-side filters ought be affecting it.

I cloned uBO-Extra locally and added 'localhost' to the domains in the exceptions list here: https://github.com/gorhill/uBO-Extra/blob/master/contentscript.js#L544-L548, then installed my local clone of the repo as an extension.

With the edited version, my app on localhost does not have the issue, but with the chrome web store version I do have the errors.

jskrzypek commented 7 years ago

So looking again at the scriptlet I noticed that you're only including 3 methods in the wrapped RTCPeerConnection's prototype. Is there a reason you don't carry over everything? Also testing with my local dev version of the extension, I found that this will also fix my issue:

bound.prototype = {};
for (var propertyName of Object.getOwnPropertyNames(RealRTCPeerConnection.prototype)) {
    Object.defineProperty(bound.prototype, propertyName, Object.getOwnPropertyDescriptor(RealRTCPeerConnection.prototype, propertyName));
}
for (var propertySymbol of Object.getOwnPropertySymbols(RealRTCPeerConnection.prototype)) {
    Object.defineProperty(bound.prototype, propertySymbol, Object.getOwnPropertyDescriptor(RealRTCPeerConnection.prototype, propertySymbol));
}

If those other methods are attack vectors, then fine, but if not I think something like this would be the most non-disruptive way to wrap the class.

gorhill commented 7 years ago

uBO-Extra 2.45 no longer wrapping RTCPeerConnection.