Closed Bug 1189060 Opened 4 years ago Closed 4 years ago

Add hooks to allow an extension to hook into createOffer() and createAnswer()

Categories

(Core :: WebRTC, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox40 --- wontfix
firefox41 --- wontfix
firefox42 --- fixed
firefox43 --- fixed
Blocking Flags:

People

(Reporter: jesup, Assigned: jib)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-needed)

Attachments

(2 files)

This is to enable an extension to be written to show UI in cases where no streams with permission are attached to a PeerConnection when createOffer/Answer are called, or to generally pop a requester when createOffer/Answer run.
Depends on: 1189097
backlog: --- → webRTC+
Rank: 12
Priority: -- → P1
Blocks: 1189167
Assignee: nobody → jib
Bug 1189060 - include ContentWebRTC.jsm in initial createOffer through hook.
Attachment #8644834 - Flags: review?(rjesup)
Attachment #8644834 - Flags: review?(mark.finkle)
Attachment #8644834 - Flags: review?(florian)
According to jesup, an add-on should be able to intercept observer notifications like those sent to the gUM permission code.

Based on this, this patch ropes in JS permission code (except it's no-op, always-allow) in to PeerConnection's launching of createOffer/CreateAnswer through similar observer notifications.

I've marked reviewers for the different corners, and I'm doing a try run. Works on desktop but not yet tested on android (I plan to pull down try builds).

What's left: This breaks B2G. B2G is going to be more difficult since the permissions UI code there is in C++ instead of JS, and with PeerConnection in JS, that's more interesting. Also, that c++ code is currently launched around gUM only. I'm going to have to think about that, so expect a followup.

How important is this feature for B2G? Given the time constraints, and assuming there's some way to detect B2G from chrome js, another option may to skip this feature for B2G for now. Thoughts?
Flags: needinfo?(mreavy)
Comment on attachment 8644834 [details]
MozReview Request: Bug 1189060 - let webrtcUI.jsm etc. block initial Offer/Answer exchange through hook.

Bug 1189060 - include ContentWebRTC.jsm in initial createOffer through hook.
Attachment #8644834 - Flags: review?(martin.thomson)
https://reviewboard.mozilla.org/r/15331/#review13725

::: browser/modules/ContentWebRTC.jsm:68
(Diff revision 1)
> +  let contentWindow = Services.wm.getOuterWindowWithId(aSubject.windowID);

secure, innerId and contentWindow don't seem to be used here.

::: browser/modules/ContentWebRTC.jsm:70
(Diff revision 1)
> +  Services.obs.notifyObservers(null, "PeerConnection:response:allow", aSubject.callID);

I don't see an obvious way to write an add-on replacing this.

The ContentWebRTC object is the only exported symbol, so unless I'm missing something, anything in this file that isn't part of this object can't be modified by add-ons.
(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> secure, innerId and contentWindow don't seem to be used here.

Right, they're only there for symmetry with the gUM notification. Presumably these would be useful to an add-on wanting to implement a CreateOffer/Answer policy. The idea was that such an add-on would intercept the gUM notification at the same time.
 
> I don't see an obvious way to write an add-on replacing this.
> 
> The ContentWebRTC object is the only exported symbol, so unless I'm missing
> something, anything in this file that isn't part of this object can't be
> modified by add-ons.

Hmm, I think we assumed that an add-on could listen to these notifications instead of the current code. Unfortunately, I have not experience with writing add-ons. Do you know what would be needed to accomplish this?
Flags: needinfo?(florian)
(In reply to Jan-Ivar Bruaroey [:jib] from comment #5)
> (In reply to Florian Quèze [:florian] [:flo] from comment #4)
> > secure, innerId and contentWindow don't seem to be used here.
> 
> Right, they're only there for symmetry with the gUM notification. Presumably
> these would be useful to an add-on wanting to implement a CreateOffer/Answer
> policy.

If they are there just to be noticed by add-on developers, they probably want to be a comment documenting the API being used, rather than actual code.

> > I don't see an obvious way to write an add-on replacing this.
> > 
> > The ContentWebRTC object is the only exported symbol, so unless I'm missing
> > something, anything in this file that isn't part of this object can't be
> > modified by add-ons.
> 
> Hmm, I think we assumed that an add-on could listen to these notifications
> instead of the current code.

An add-on could listen to these notifications in addition to the current code, not instead of it.

> Do you know what would be needed to accomplish this?

I have a few ideas, but none of them seems particularly great to me right now, so maybe I should think about it some more, and discuss it with others to see if they have something better to propose.

Here is a simple possibility:

this.ContentWebRTC = {
...
  init: function() {
...
    Services.obs.addObserver(this, "PeerConnection:request", false);
...
  },

  observe: function(aSubject, aTopic, aData) {
    // Override this method to provide a different behavior with an add-on.
    Services.obs.notifyObservers(null, "PeerConnection:response:allow", ...
  }


An add-on will then be able to override the behavior by doing:
ContentWebRTC.observe = /* new code provided by the add-on. */


The ContentWebRTC object can only have one 'observe' method, so if you want add-ons to be able to override the "getUserMedia:request" and "PeerConnection:request" behaviors separately, you would do something slightly more complicated:

this.ContentWebRTC = {
...
  init: function() {
...
    Services.obs.addObserver(this.pcObserver, "PeerConnection:request", false);
...
  },

  pcObserver: {observe: function(aSubject, aTopic, aData) {
    // Override this method to provide a different behavior with an add-on.
    Services.obs.notifyObservers(null, "PeerConnection:response:allow", ...
  }}

and an add-on would then do:
ContentWebRTC.pcObserver.observe = ...


This solution isn't very add-on friendly, as add-on authors would still need to register a content script to be able to have their code run in the content process, and then they'll need to send messages to the chrome process to be able to show UI. If you expect add-ons overriding this behavior to want to show UI, forwarding the messages to the chrome process and putting the add-on hooks in webrtcUI.jsm would be friendlier.

I have 2 questions about this, that could help me give better advice:
- Are you trying to make this somehow extensible so that we can claim that it's possible to change the behavior with an add-on; even though making such an add-on would require some work? Or are you trying to make a friendly API and encourage people to use it?
- Do you care about 2 different add-ons being able to tweak this without overriding each other, or is it the users' responsibility to not install at once 2 add-ons using this API?
Flags: needinfo?(florian)
(In reply to Florian Quèze [:florian] [:flo] from comment #6)
> Here is a simple possibility:

That seems like it should work. The add-on can even save off the original functions and pass "up" to them.

Thanks!

> This solution isn't very add-on friendly, as add-on authors would still need
> to register a content script to be able to have their code run in the
> content process, and then they'll need to send messages to the chrome
> process to be able to show UI. If you expect add-ons overriding this
> behavior to want to show UI, forwarding the messages to the chrome process
> and putting the add-on hooks in webrtcUI.jsm would be friendlier.

Aha! webrtcUI.jsm is where I thought I was. Last I looked at this was before e10s...

> I have 2 questions about this, that could help me give better advice:
> - Are you trying to make this somehow extensible so that we can claim that
> it's possible to change the behavior with an add-on; even though making such
> an add-on would require some work? Or are you trying to make a friendly API
> and encourage people to use it?

Maybe Maire has input here, but my impression was that there's a time component to this, meaning "make it possible" is the first priority.

> - Do you care about 2 different add-ons being able to tweak this without
> overriding each other, or is it the users' responsibility to not install at
> once 2 add-ons using this API?

Having two add-ons compete to send PeerConnection "allow" and "deny" messages at the same time would be inherently confusing, so I don't think we care too much. I suppose it would be nice if they added and removed cleanly.
(In reply to Jan-Ivar Bruaroey [:jib] from comment #7)
> (In reply to Florian Quèze [:florian] [:flo] from comment #6)

> > This solution isn't very add-on friendly, as add-on authors would still need
> > to register a content script to be able to have their code run in the
> > content process, and then they'll need to send messages to the chrome
> > process to be able to show UI. If you expect add-ons overriding this
> > behavior to want to show UI, forwarding the messages to the chrome process
> > and putting the add-on hooks in webrtcUI.jsm would be friendlier.
> 
> Aha! webrtcUI.jsm is where I thought I was. Last I looked at this was before
> e10s...

webrtcUI.jsm would seem to be the place for this; we do expect them to expose UI.

> > I have 2 questions about this, that could help me give better advice:
> > - Are you trying to make this somehow extensible so that we can claim that
> > it's possible to change the behavior with an add-on; even though making such
> > an add-on would require some work? Or are you trying to make a friendly API
> > and encourage people to use it?
> 
> Maybe Maire has input here, but my impression was that there's a time
> component to this, meaning "make it possible" is the first priority.

Much more on the "possible" side, than "encourage".  Especially if that affects how quickly it can get in, and to a slightly lesser extent how easy it is to uplift to 41.

> > - Do you care about 2 different add-ons being able to tweak this without
> > overriding each other, or is it the users' responsibility to not install at
> > once 2 add-ons using this API?
> 
> Having two add-ons compete to send PeerConnection "allow" and "deny"
> messages at the same time would be inherently confusing, so I don't think we
> care too much. I suppose it would be nice if they added and removed cleanly.

No interest in two addons sharing this.

(Clearing mreavy NI)
Flags: needinfo?(mreavy)
+1 to everything Jesup said.  One question that didn't get answerwed: "How important is this feature for B2G? Given the time constraints, and assuming there's some way to detect B2G from chrome js, another option may to skip this feature for B2G for now. Thoughts?"

We should skip B2G for this bug.  We should file a follow up bug at a lower priority and discuss on the bug if we actually need to do anything for B2G.  

FYI: My opinion currently is that there is no demand for this on B2G.  So I think we should file a bug and then put it in parking lot until there is a real demand.  Thanks, jib!
Comment on attachment 8644834 [details]
MozReview Request: Bug 1189060 - let webrtcUI.jsm etc. block initial Offer/Answer exchange through hook.

https://reviewboard.mozilla.org/r/15333/#review13747

I'm not sure that this is going to work.  See comment below.  I'd be happier if there were a test proving that it is possible to block createOffer/createAnswer.

::: dom/media/PeerConnection.js:763
(Diff revision 1)
> +  getPermission: function() {
> +    if (!this._havePermission) {
> +      this._havePermission = new Promise((resolve, reject) =>
> +          this._settlePermission = { allow: resolve, deny: reject });
> +
> +      let outerId = this._win.QueryInterface(Ci.nsIInterfaceRequestor).
> +          getInterface(Ci.nsIDOMWindowUtils).outerWindowID;
> +      let request = { windowID: outerId,
> +                      innerWindowId: this._winID,
> +                      callID: this._globalPCListId,
> +                      isSecure: this._https };
> +      request.wrappedJSObject = request;
> +      Services.obs.notifyObservers(request, "PeerConnection:request", null);
> +    }
> +    return this._havePermission;
> +  },

Can you explain why this is called every time?  This results in a call do the parent process for every instance of RTCPeerConnection, rather than once per origin per session.  Are you expecting the addon to manage persistence?

::: browser/modules/ContentWebRTC.jsm:64
(Diff revision 1)
> -function handleRequest(aSubject, aTopic, aData) {
> +function handlePCRequest(aSubject, aTopic, aData) {
> +  aSubject = aSubject.wrappedJSObject;
> +  let secure = aSubject.isSecure;
> +  let innerId = aSubject.innerWindowID;
> +  let contentWindow = Services.wm.getOuterWindowWithId(aSubject.windowID);
> +
> +  Services.obs.notifyObservers(null, "PeerConnection:response:allow", aSubject.callID);
> +}

Is this actually going to work?  If an addon registers an observer for PeerConnection:request, how can it prevent this from sending the :allow response before it can send a :deny?

::: browser/modules/ContentWebRTC.jsm:66
(Diff revision 1)
> +  let secure = aSubject.isSecure;
> +  let innerId = aSubject.innerWindowID;

unused

::: mobile/android/chrome/content/WebrtcUI.js:79
(Diff revision 1)
> +    let secure = aSubject.isSecure;
> +    let innerId = aSubject.innerWindowID;

unused

::: webapprt/WebRTCHandler.jsm:18
(Diff revision 1)
> +  let { windowID, innerWindowID, callID, isSecure } = aSubject;

Different from the others.

Also, how in the hell do we justify creating identical code between the different platforms?

::: dom/media/PeerConnection.js:780
(Diff revision 1)
>    setLocalDescription: function(desc, onSuccess, onError) {

Please verify that it isn't possible to synthesize an offer and pass that into setLocalDescription.  If that is possible, then it might be possible to circumvent your permission check.

We should be checking ice ufrag, pwd, a fingerprint attributes for equality, but I want to double-check.
Attachment #8644834 - Flags: review?(martin.thomson)
https://reviewboard.mozilla.org/r/15333/#review13747

> Is this actually going to work?  If an addon registers an observer for PeerConnection:request, how can it prevent this from sending the :allow response before it can send a :deny?

I'm redoing this as Florian suggested in comment 6, so the add-on can override methods on the handler.

> Can you explain why this is called every time?  This results in a call do the parent process for every instance of RTCPeerConnection, rather than once per origin per session.  Are you expecting the addon to manage persistence?

It is not called every time, only once per PeerConnection. This is because peerConnections can be opened concurrently with each other, and they all need to be blocked until the add-on gives the go-ahead (which can take a long time if it comes with a doorhanger). As for optimization, isn't generating the certificate the red line here?

> Please verify that it isn't possible to synthesize an offer and pass that into setLocalDescription.  If that is possible, then it might be possible to circumvent your permission check.
> 
> We should be checking ice ufrag, pwd, a fingerprint attributes for equality, but I want to double-check.

Good point, others might want to speak whether that is possible. Adding a dependency on the permission promise in setLocal and setRemote is trivial and probably a good idea, so lets do that. Better to stop them at the door. Better API behavior too.

> Different from the others.
> 
> Also, how in the hell do we justify creating identical code between the different platforms?

APIs have to be called from more than one place to be justified right? You haven't even seen the gonk code for handling GetUserMediaRequest. It's in c++ :)
(In reply to Martin Thomson [:mt:] from comment #10)
> I'd be happier if there were a test proving that it is possible to block
> createOffer/createAnswer.

(Review board ate my last comment before publish)

FWIW I've stepped through the code and verified that all the right steps happen to make it work (the way it is written it's more of a miracle when it *doesn't* block). Automated tests are ideal of course.
The add-on is going to need to keep state if it is going to produce a door-hanger similar to the gum permission one, is the thinking. See Bug 1189097 comment 14.
Attachment #8644834 - Attachment description: MozReview Request: Bug 1189060 - include ContentWebRTC.jsm in initial createOffer through hook. → MozReview Request: Bug 1189060 - let webrtcUI.jsm etc. block initial Offer/Answer exchange through hook.
Attachment #8644834 - Flags: review?(martin.thomson)
Attachment #8644834 - Flags: review?(fabrice)
Comment on attachment 8644834 [details]
MozReview Request: Bug 1189060 - let webrtcUI.jsm etc. block initial Offer/Answer exchange through hook.

Bug 1189060 - let webrtcUI.jsm etc. block initial Offer/Answer exchange through hook.
Comment on attachment 8644834 [details]
MozReview Request: Bug 1189060 - let webrtcUI.jsm etc. block initial Offer/Answer exchange through hook.

Bug 1189060 - let webrtcUI.jsm etc. block initial Offer/Answer exchange through hook.
Comment on attachment 8644834 [details]
MozReview Request: Bug 1189060 - let webrtcUI.jsm etc. block initial Offer/Answer exchange through hook.

Bug 1189060 - let webrtcUI.jsm etc. block initial Offer/Answer exchange through hook.
Comment on attachment 8644834 [details]
MozReview Request: Bug 1189060 - let webrtcUI.jsm etc. block initial Offer/Answer exchange through hook.

https://reviewboard.mozilla.org/r/15333/#review13861

::: mobile/android/chrome/content/WebrtcUI.js:13
(Diff revision 4)
> +  // Add-ons are encouraged to override stock permission behavior by doing:
> +  //
> +  //   var stockObserve = WebrtcUI.observe;
> +  //
> +  //   webrtcUI.observe = function(aSubject, aTopic, aData) {
> +  //     switch (aTopic) {
> +  //      case "PeerConnection:request": {
> +  //        // new code.
> +  //        break;
> +  //      ...
> +  //      default:
> +  //        return stockObserve(aSubject, aTopic, aData);
> +  //

This isn't exactly an API for add-ons. This is monkey patching.

What's the long term plan?

::: mobile/android/chrome/content/WebrtcUI.js:95
(Diff revision 4)
> +    aSubject = aSubject.wrappedJSObject;
> +    let { windowID, callID } = aSubject; // Also avail: isSecure, innerWindowID

I'd rather see this documented in a comment rather than have unused variables in the code.

::: dom/media/PeerConnection.js:780
(Diff revision 4)
> +        request.wrappedJSObject = request;

Using wrappedJSObject like this is a little hacky. Too bad we don't have a better way to handle this.
Attachment #8644834 - Flags: review?(mark.finkle)
https://reviewboard.mozilla.org/r/15333/#review13861

> This isn't exactly an API for add-ons. This is monkey patching.
> 
> What's the long term plan?

Yes, this is a fire drill of sorts to try to address IP leakage concerns for the 41-42 timeframe. Not sure about our plans here beyond that frankly, or what that should look like - this isn't something we expect a lot of add-ons to do, or even encourage, since it arguably disables or limits/hampers the webrtc experience - though I'm interested in feedback on how this is typically done.
https://reviewboard.mozilla.org/r/15333/#review13861

> Using wrappedJSObject like this is a little hacky. Too bad we don't have a better way to handle this.

Yes too bad. I had a version that did JSON.stringify/parse in the data field, but that didn't seem better.
FWIW we already have someone lined up to do a sample add-on. ni Maire on long-term plans.
Flags: needinfo?(mreavy)
Flags: needinfo?(fippo)
(In reply to Jan-Ivar Bruaroey [:jib] from comment #20)
> FWIW we already have someone lined up to do a sample add-on.

Not Android though initially AFAIK.
Comment on attachment 8644834 [details]
MozReview Request: Bug 1189060 - let webrtcUI.jsm etc. block initial Offer/Answer exchange through hook.

Bug 1189060 - let webrtcUI.jsm etc. block initial Offer/Answer exchange through hook.
Attachment #8644834 - Flags: review?(mark.finkle)
> This isn't exactly an API for add-ons. This is monkey patching.

You're right.  We're not taking the time to really design an API because we're not sure we'll need one.  And given what we're trying to do (more about that below), I think that's the best use of our time and people.

We want to land something so that a very specific user group can experiment with this functionality and tell us what will and won't work for them.  Some of them feel that not having this functionality is a real safety issue for them; so we're trying to move quickly.


> What's the long term plan?

Both we and Chrome are providing ASAP mitigations (i.e. I'm talking to our relman team about uplifting these patches to Fx41) that allow extensions to limit or query on access before allowing pieces of code to run (ICE use of non-default routes, PeerConnection creation (and ICE start) without user interaction (i.e. this bug) -- enable query on all creations, or block all of them unless the user flips a switch (up to the user and the extension), etc).

We'll then assess what makes the most sense longer-term based on further discussion and user feedback.  2 big questions we plan to answer for the long-term: How much of this should be built in? And should there be any changes to the defaults (like flipping on some of these by default in private browsing mode, or dependent on the level of privacy asked for there, etc)?

If we need a more generalized API at that point, we'd update this. If we just need a hook for a extension to be able to intercept this, we may well stick with what we have here -- though I have no problem with moving to a "cleaner" API once discussions have settled and this particular group of users feel they have the functionality they need.
Flags: needinfo?(mreavy)
Comment on attachment 8644834 [details]
MozReview Request: Bug 1189060 - let webrtcUI.jsm etc. block initial Offer/Answer exchange through hook.

https://reviewboard.mozilla.org/r/15333/#review13883

Maire has answered the larger concerns here.  So long as this provides the necessary hooks (even if a little ugly), that meets the need.  I don't think a separate review by me is needed on this code.
Attachment #8644834 - Flags: review?(rjesup)
Attachment #8644834 - Flags: review?(fabrice)
Comment on attachment 8644834 [details]
MozReview Request: Bug 1189060 - let webrtcUI.jsm etc. block initial Offer/Answer exchange through hook.

https://reviewboard.mozilla.org/r/15333/#review13887

::: dom/media/PeerConnection.js:52
(Diff revision 5)
> +  Services.obs.addObserver(this, "PeerConnection:response:deny", true);

Please rename to peerconnection-response-allow and peerconnection-response-deny to follow the format used by observer notification topics. Yours look like ipc message names.

::: dom/media/PeerConnection.js:766
(Diff revision 5)
> +    if (!this._havePermission) {

I would early return:
if (this._havePermission) {
  return this_havePermission;
}
if (AppConstants.MOZ_B2G || Services...) {
  return this._havePermission = Promise.resolve();
}
return this._havePermission = new Promise((...)
https://reviewboard.mozilla.org/r/15333/#review13887

> Please rename to peerconnection-response-allow and peerconnection-response-deny to follow the format used by observer notification topics. Yours look like ipc message names.

Fabrice, these were picked for symmetry with "getUserMedia:response:allow", "getUserMedia:response:deny" and "getUserMedia:request", the other permission observer messages, which the same add-on(s) would have to deal with in parallel to solve permissions here, and whose mechanisms are almost identical, so I think consistency with those matters more. The getUserMedia ones have been there for a very long time, so I don't know what would break if we changed those [1]. OK to leave as is?

[1] http://mxr.mozilla.org/mozilla-central/source/webapprt/WebRTCHandler.jsm?mark=85-103#84
Comment on attachment 8644834 [details]
MozReview Request: Bug 1189060 - let webrtcUI.jsm etc. block initial Offer/Answer exchange through hook.

Bug 1189060 - let webrtcUI.jsm etc. block initial Offer/Answer exchange through hook.
Attachment #8644834 - Flags: review?(rjesup)
Attachment #8644834 - Flags: review?(fabrice)
Comment on attachment 8644834 [details]
MozReview Request: Bug 1189060 - let webrtcUI.jsm etc. block initial Offer/Answer exchange through hook.

Bug 1189060 - let webrtcUI.jsm etc. block initial Offer/Answer exchange through hook.
Attachment #8644834 - Flags: review?(rjesup)
Attachment #8644834 - Flags: review?(mark.finkle) → review+
Comment on attachment 8644834 [details]
MozReview Request: Bug 1189060 - let webrtcUI.jsm etc. block initial Offer/Answer exchange through hook.

https://reviewboard.mozilla.org/r/15333/#review13897

The mobile parts look OK, given the requirements.
Comment on attachment 8644834 [details]
MozReview Request: Bug 1189060 - let webrtcUI.jsm etc. block initial Offer/Answer exchange through hook.

https://reviewboard.mozilla.org/r/15333/#review13901

I think this now does what you intend. What's the plan for tests?

::: browser/modules/ContentWebRTC.jsm:26
(Diff revision 6)
> +    Services.obs.addObserver(handlePCRequest, "PeerConnection:request", false);

Having 'Gum' and 'PC' next to each other seem inconsistent. I think this should either be 'Gum' and 'Pc' or 'GUM' and 'PC'.

::: browser/modules/ContentWebRTC.jsm:50
(Diff revision 6)
> +        let topic = (aMessage.name == "rtcpeer:Allow")? "PeerConnection:response:allow" :

nit: missing space before '?'

::: browser/modules/ContentWebRTC.jsm:80
(Diff revision 6)
> +  aSubject = aSubject.wrappedJSObject;

Why is this .wrappedJSObject needed?

If there's a good reason, I think it would make sense to have a comment explaining it here.

::: browser/modules/webrtcUI.jsm:132
(Diff revision 6)
> +      // Add-ons are encouraged to override stock permission behavior by doing:

"Add-ons can override" seems to match better the discussions we had in the bug than "Add-ons are encouraged to override".

::: browser/modules/webrtcUI.jsm:143
(Diff revision 6)
> +      //        return stockReceiveMessage(aMessage);

The current receiveMessage function doesn't use 'this' so this currently makes no difference, but I think it would be safer to encourage add-on authors to propagate the 'this' value:
  stockReceiveMessage.call(this, aMessage);

::: browser/modules/ContentWebRTC.jsm:101
(Diff revision 6)
> +  let { windowID, innerWindowID, callID, isSecure } = aSubject;

This change to a destructuring assignment at the beginning of the function instead of using attributes of aSubject in mozGetUserMediaDevices' callbacks means that this JS code no longer keeps a reference to aSubject. That's cleaner, but there's a slight potential for this change to expose existing but invisibile-until-now back-end bugs. I'm not saying we shouldn't do this, just pointing out that if after landing this patch turns out to produce unexpected and random behavior changes, this is where we should look.
Attachment #8644834 - Flags: review?(florian)
Comment on attachment 8644834 [details]
MozReview Request: Bug 1189060 - let webrtcUI.jsm etc. block initial Offer/Answer exchange through hook.

https://reviewboard.mozilla.org/r/15333/#review13935

The b2g specific pieces are fine for me.
Attachment #8644834 - Flags: review?(fabrice) → review+
https://reviewboard.mozilla.org/r/15333/#review13901

I've tested that everything works manually. AFAIK automating tests of permission code end-to-end is tricky, since permissions are effectively disabled in the tree, but I see browser/base/content/test/general/browser_devices_get_user_media.js, so I should maybe look at duplicating that effort at least. I understand there was some interest in landing this asap, so is it an option to land tests in a follow-up?

> This change to a destructuring assignment at the beginning of the function instead of using attributes of aSubject in mozGetUserMediaDevices' callbacks means that this JS code no longer keeps a reference to aSubject. That's cleaner, but there's a slight potential for this change to expose existing but invisibile-until-now back-end bugs. I'm not saying we shouldn't do this, just pointing out that if after landing this patch turns out to produce unexpected and random behavior changes, this is where we should look.

The GetUserMediaRequest browser object is fire-and-forget without important resources attached, so whether JS keeps a reference to it or not should not matter. That said, I'll remove any concern and reduce the footprint of the patch here, to have potential uplifting go smoother.
Comment on attachment 8644834 [details]
MozReview Request: Bug 1189060 - let webrtcUI.jsm etc. block initial Offer/Answer exchange through hook.

Bug 1189060 - let webrtcUI.jsm etc. block initial Offer/Answer exchange through hook.
Attachment #8644834 - Flags: review?(mark.finkle)
Attachment #8644834 - Flags: review?(florian)
Attachment #8644834 - Flags: review?(fabrice)
Attachment #8644834 - Flags: review+
Attachment #8644834 - Flags: review?(mark.finkle)
Attachment #8644834 - Flags: review?(fabrice)
Attachment #8644834 - Flags: review+
Attachment #8644834 - Flags: review?(martin.thomson) → review+
Comment on attachment 8644834 [details]
MozReview Request: Bug 1189060 - let webrtcUI.jsm etc. block initial Offer/Answer exchange through hook.

https://reviewboard.mozilla.org/r/15333/#review13955

Ship It!
[Tracking Requested - why for this release]:
Plan is to uplift this into 41 to support an extension that can mitigate these issues ASAP (see mail to drivers)
Comment on attachment 8644834 [details]
MozReview Request: Bug 1189060 - let webrtcUI.jsm etc. block initial Offer/Answer exchange through hook.

https://reviewboard.mozilla.org/r/15333/#review13987

::: browser/modules/ContentWebRTC.jsm:81
(Diff revision 7)
> +  // WebIDL interface didn't work here as object comes from JSImplemented code).

I'm afraid I still don't understand this. If WebIDL doesn't work here (I'm not really familiar with how WebIDL works), can't we use an xpidl interface, and implement QueryInterface on the object?
Attachment #8644834 - Flags: review?(florian)
(In reply to Jan-Ivar Bruaroey [:jib] from comment #32)
> https://reviewboard.mozilla.org/r/15333/#review13901
> 
> I've tested that everything works manually. AFAIK automating tests of
> permission code end-to-end is tricky, since permissions are effectively
> disabled in the tree, but I see
> browser/base/content/test/general/browser_devices_get_user_media.js, so I
> should maybe look at duplicating that effort at least.

A test would probably be a browser-chrome test, similar to browser_devices_get_user_media.js, yes.

It should import webrtcUI.jsm, override receiveMessage, and load a webpage that initializes a PeerConnection object and tries to get the local description.

Then I think the tests that we want would:
- initially, just count the notifications and do nothing else, to ensure that the existing fallback code works.
- then, create a new pc object (eg. reload the page), and make the override function block the PC request. Ensure the appropriate failure callback is called on the pc object.
- if we already support modifying the way candidates are generated during that callback, we should reload the page again and test that too.

> I understand there
> was some interest in landing this asap, so is it an option to land tests in
> a follow-up?

Landing on central soon even without tests seems fine, if it lets QA and add-on authors start working and give us feedback faster. But I would really like this to be covered by tests landing in the same cycle, as this feature isn't used by anything in the tree, so we may not notice until it's released if we somehow break it. Having reasonable test coverage also helps to get uplift approvals.
(In reply to Florian Quèze [:florian] [:flo] from comment #36)
> I'm afraid I still don't understand this. If WebIDL doesn't work here (I'm
> not really familiar with how WebIDL works), can't we use an xpidl interface,
> and implement QueryInterface on the object?

IMHO wrappedJSObject is a lot less messy than QueryInterface. There's JS on either end here. I think I'd rather do JSON stuffing than XPCOM.
(In reply to Florian Quèze [:florian] [:flo] from comment #37)
> A test would probably be a browser-chrome test, similar to
> browser_devices_get_user_media.js, yes.
> 
> It should import webrtcUI.jsm, override receiveMessage, and load a webpage
> that initializes a PeerConnection object and tries to get the local
> description.
> 
> Then I think the tests that we want would:

Thanks for this. I'll do these steps in a browser_peer_connection_ice.js

> Landing on central soon even without tests seems fine, if it lets QA and
> add-on authors start working and give us feedback faster. But I would really
> like this to be covered by tests landing in the same cycle, as this feature
> isn't used by anything in the tree, so we may not notice until it's released
> if we somehow break it. Having reasonable test coverage also helps to get
> uplift approvals.

Agree.
(In reply to Jan-Ivar Bruaroey [:jib] from comment #38)
> (In reply to Florian Quèze [:florian] [:flo] from comment #36)
> > I'm afraid I still don't understand this. If WebIDL doesn't work here (I'm
> > not really familiar with how WebIDL works), can't we use an xpidl interface,
> > and implement QueryInterface on the object?
> 
> IMHO wrappedJSObject is a lot less messy than QueryInterface. There's JS on
> either end here. I think I'd rather do JSON stuffing than XPCOM.

Either we are not understanding each other correctly (and should maybe discuss on IRC), or we disagree.

It seems to me that what's messy here is trying to circumvent defining the API that's used. Especially when an almost identical API is used for the other similar notification for gum.
I'm not trying to circumvent anything, except maybe bugs. The gUm api uses a webidl interface from c++.

While I had a webidl interface as well in an early version, it still required the client to use .wrappedJSObject for some reason, which I suspect is a bug. bz is on vacation, so I haven't been able to confirm this.

How about I put the webidl interface back - that way the api will be defined - I file a bug and put a comment above the .wrappedJSObject use that links to the bug?
Flags: needinfo?(florian)
We discussed this over IRC. The current plan seems to be to replace the wrappedJSObject hack with an idl interface, and have the implementation also implement nsIClassInfo.
Flags: needinfo?(florian)
Flags: needinfo?(fippo)
https://reviewboard.mozilla.org/r/15333/#review13987

> I'm afraid I still don't understand this. If WebIDL doesn't work here (I'm not really familiar with how WebIDL works), can't we use an xpidl interface, and implement QueryInterface on the object?

OK I'm embarrassed. In the process of filing a bug about WebIDL not working here, I revived an earlier patch and tried to reproduce my problem, but I couldn't. In other words, it worked! - I could have sworn the code was creating a webidl interface last time, but as I redid it, it became clear that it couldn't have, as I now got errors I didn't see last time. So patch up next. Florian, thanks for sticking to your guns as I wouldn't have looked back, and peterv, both, sorry for wasting your time.
Bug 1189060 - add CreateOfferRequest.webidl interface for add-ons
Attachment #8646519 - Flags: review?(peterv)
Attachment #8646519 - Flags: review?(florian)
Attachment #8646519 - Flags: review?(florian) → review+
Comment on attachment 8646519 [details]
MozReview Request: Bug 1189060 - add CreateOfferRequest.webidl interface for add-ons

https://reviewboard.mozilla.org/r/15791/#review14061

::: dom/media/PeerConnection.js:1498
(Diff revision 1)
> +//  this.wrappedJSObject = this;

Please remove this line of dead code before pushing.
Comment on attachment 8644834 [details]
MozReview Request: Bug 1189060 - let webrtcUI.jsm etc. block initial Offer/Answer exchange through hook.

https://reviewboard.mozilla.org/r/15333/#review14065
Attachment #8644834 - Flags: review+
Attachment #8646519 - Flags: review+ → review?(florian)
Comment on attachment 8646519 [details]
MozReview Request: Bug 1189060 - add CreateOfferRequest.webidl interface for add-ons

Bug 1189060 - add CreateOfferRequest.webidl interface for add-ons
Attachment #8646519 - Flags: review?(florian) → review+
Comment on attachment 8646519 [details]
MozReview Request: Bug 1189060 - add CreateOfferRequest.webidl interface for add-ons

Bug 1189060 - add CreateOfferRequest.webidl interface for add-ons
Attachment #8646519 - Flags: review?(peterv)
Attachment #8646519 - Flags: review?(khuey)
Attachment #8646519 - Flags: review?(florian)
Attachment #8646519 - Flags: review+
Attachment #8646519 - Flags: review?(florian) → review+
Comment on attachment 8646519 [details]
MozReview Request: Bug 1189060 - add CreateOfferRequest.webidl interface for add-ons

r+ from khuey in #media.
Attachment #8646519 - Flags: review?(khuey) → review+
https://hg.mozilla.org/mozilla-central/rev/bd7d66caa793
https://hg.mozilla.org/mozilla-central/rev/162488161353
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Tracked as this work is planned to ship in FF41.
(In reply to Jan-Ivar Bruaroey [:jib] from comment #51)
> r+ from khuey in #media.

Next time please be careful to use the correct reviewer in the checkin comment too. Thanks!
Comment on attachment 8644834 [details]
MozReview Request: Bug 1189060 - let webrtcUI.jsm etc. block initial Offer/Answer exchange through hook.

Approval Request Comment
[Feature/regressing bug #]: N/A

[User impact if declined]: Inability to use extensions to hook createOffer/Answer, to enable users to confirm if they want PeerConnections set up or to be informed of their use.

[Describe test coverage new/current, TreeHerder]: Manual testing with an extension done by :fippo.  Adding tests based on the extension code, and will uplift the tests.

[Risks and why]: Low risk, only asking for Aurora. Should have no impact when not hooked, but does require extra steps to allow for hooking.  Risk when not hooked should be very low.

[String/UUID change made/needed]: none
Attachment #8644834 - Flags: approval-mozilla-aurora?
Attachment #8646519 - Flags: approval-mozilla-aurora?
Comment on attachment 8644834 [details]
MozReview Request: Bug 1189060 - let webrtcUI.jsm etc. block initial Offer/Answer exchange through hook.

Approving now to increase the testing coverage. Thanks
Attachment #8644834 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8646519 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
The team and I have decided not to ask for uplift to Beta.
(In reply to Jan-Ivar Bruaroey [:jib] from comment #39)
> (In reply to Florian Quèze [:florian] [:flo] from comment #37)
> > A test would probably be a browser-chrome test, similar to
> > browser_devices_get_user_media.js, yes.
> > 
> > It should import webrtcUI.jsm, override receiveMessage, and load a webpage
> > that initializes a PeerConnection object and tries to get the local
> > description.
> > 
> > Then I think the tests that we want would:
> 
> Thanks for this. I'll do these steps in a browser_peer_connection_ice.js
> 
> > Landing on central soon even without tests seems fine, if it lets QA and
> > add-on authors start working and give us feedback faster. But I would really
> > like this to be covered by tests landing in the same cycle, as this feature
> > isn't used by anything in the tree, so we may not notice until it's released
> > if we somehow break it. Having reasonable test coverage also helps to get
> > uplift approvals.
> 
> Agree.

Is there a bug on file to handle writing this test?
Flags: needinfo?(jib)
Filed Bug 1200846. Thanks.
Flags: needinfo?(jib)
Depends on: 1207784
Blocks: 1050930
Will, is this something you should look into or do you need me to handle it? It's support for add-ons to be able to control access to WebRTC functionality.
Flags: needinfo?(wbamberg)
sheppy, I'm prioritizing WebExtensions, so unless/until this is a WebExtension API, then I don't expect to document it.
Flags: needinfo?(wbamberg)
You need to log in before you can comment on or make changes to this bug.