Closed Bug 1310355 Opened 8 years ago Closed 7 years ago

Improve resiliency for using webrtc permission hooks

Categories

(Core :: WebRTC, defect, P1)

Product:
Core
Component:
WebRTC
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: aswan, Assigned: aswan)

References

Details

Attachments

(1 file)

Bug 1310355 Improve resiliency of webrtc add-on hooks
58 bytes, text/x-review-board-request
jib
: review+
florian
: review+
Details 
In bug 1189060, hooks were added to webrtc to let add-ons observe or participate in the process of granting permissions for various webrtc-related events (i.e., calls to getUserMedia() and establishment of rtc peer connections).  But the method exposed (overriding webrtcUI.receiveMessage) is not robust to multiple add-ons installing and uninstalling their own hooks in an arbitrary order.

This bug is to improve the hook, by providing observable events for things that add-ons want to passively watch and a safe way to register a blocking callback for peer connection establishment.
Jan, the attached patch doesn't have any unit tests but I don't see any tests for the existing hook.  I'd be happy to write some if you can suggest where they should go and point me to existing webrtc tests that I can use as a model.
Rank: 19
Priority: -- → P1
Comment on attachment 8801393 [details]
Bug 1310355 Improve resiliency of webrtc add-on hooks

https://reviewboard.mozilla.org/r/86146/#review85330

Nice code! Need to move "peer-request" I think, and catch exception from first handler, otherwise just nits. Would like to see it again.

::: browser/modules/webrtcUI.jsm:181
(Diff revision 1)
> -  receiveMessage: function(aMessage) {
> -    switch (aMessage.name) {
> -
> -      // Add-ons can override stock permission behavior by doing:
> +  // Add-ons can override stock permission behavior by doing:
> -      //
> +  //
> -      //   var stockReceiveMessage = webrtcUI.receiveMessage;
> +  //   webrtcUI.addPeerConnectionBlocker(function(aParams() {

There's a stray paren in here.

::: browser/modules/webrtcUI.jsm:211
(Diff revision 1)
> +  addPeerConnectionBlocker: function(aCallback) {
> +    this.peerConnectionBlockers.add(aCallback);
> +  },
> +
> +  removePeerConnectionBlocker: function(aCallback) {
> +    this.peerConnectionBlockers.delete(aCallback);
> +  },

Any reason you're choosing to wrap webrtcUI.peerConnectionBlockers and not webrtcUI.emitter ?

I don't have a preference, it just seemed asymmetric.

::: browser/modules/webrtcUI.jsm:219
(Diff revision 1)
> +  receiveMessage: function(aMessage) {
> +    switch (aMessage.name) {
>  
>        case "rtcpeer:Request": {
> -        // Always allow. This code-point exists for add-ons to override.
> -        let { callID, windowID } = aMessage.data;
> +        let params = Object.assign({origin: aMessage.target.contentPrincipal.origin},
> +                                   aMessage.data);
> -        // Also available: isSecure, innerWindowID. For contentWindow:
> -        //
> -        //   let contentWindow = Services.wm.getOuterWindowWithId(windowID);
>  
>          let mm = aMessage.target.messageManager;
> -        mm.sendAsyncMessage("rtcpeer:Allow",
> -                            { callID: callID, windowID: windowID });
> +        function answer(reply) {
> +          if (reply == "rtcpeer:Allow") {
> +            this.emitter.emit("peer-request", params);
> +          }

Shouldn't "peer-request" be emitted right away rather than on reply (on "rtcpeer:Request" rather than "rtcpeer:Allow") ?

::: browser/modules/webrtcUI.jsm:223
(Diff revision 1)
> -        // Always allow. This code-point exists for add-ons to override.
> -        let { callID, windowID } = aMessage.data;
> +        let params = Object.assign({origin: aMessage.target.contentPrincipal.origin},
> +                                   aMessage.data);

Making a copy is a good idea.

Might it be even safer to make a fresh copy for each listener? That way one listener cannot mess up another.

I realize this might involve changing EventEmitter. Just an idea. Not sure how paranoid we are with extensions.

::: browser/modules/webrtcUI.jsm:238
(Diff revision 1)
> +        let blockers = Array.from(this.peerConnectionBlockers);
> +        function next() {
> +          if (blockers.length == 0) {
> +            answer("rtcpeer:Allow");
> +            return undefined;
> +          }

Just a thought, if you set:

    let blockers = [...peerConnectionBlockers, () => "rtcpeer:Allow"];

Then you wouldn't need to check for end of array in next().

::: browser/modules/webrtcUI.jsm:242
(Diff revision 1)
> +
> +        let blockers = Array.from(this.peerConnectionBlockers);
> +        function next() {
> +          if (blockers.length == 0) {
> +            answer("rtcpeer:Allow");
> +            return undefined;

Nit:

    return;

is shorter, and prevailing (at least in this file).

::: browser/modules/webrtcUI.jsm:245
(Diff revision 1)
> +          let blocker = blockers.shift();
> +          return Promise.resolve(blocker(params)).then(result => {

If the very first blocker throws then we get an exception here rather than a rejection. Instead:

    return Promise.resolve()
      .then(() => blocker(params)).then(result => {

would ensure even the first error is caught.

Also, should we catch errors from individual listeners here, so a broken listener won't wreak it for everyone? E.g.:

    return Promise.resolve()
      .then(() => blocker(params))
      .catch(e => {
        Cu.reportError(`error in PeerConnection blocker: ${err.message}`);
      })
      .then(result => {
?

::: browser/modules/webrtcUI.jsm:249
(Diff revision 1)
> +              return undefined;
> +            } else if (result == "deny") {

Nit: else is redundant after return.

::: browser/modules/webrtcUI.jsm:284
(Diff revision 1)
> +        let origin = aMessage.target.contentPrincipal.origin;
> +        let {camera, microphone} = aMessage.data;
> +        let params = {camera, microphone, origin: aMessage.target.contentPrincipal.origin};

Nit: Since you already have origin, maybe:

    let params = {camera, microphone, origin};
?

::: browser/modules/webrtcUI.jsm:287
(Diff revision 1)
>        case "webrtc:UpdateBrowserIndicators":
> +        // Beware, this will need to change when https://bugzil.la/1299577 lands
> +        let origin = aMessage.target.contentPrincipal.origin;
> +        let {camera, microphone} = aMessage.data;
> +        let params = {camera, microphone, origin: aMessage.target.contentPrincipal.origin};
> +        this.emitter.emit("media-permissions", origin, params);

Should origin be passed in here? Seems unexpected.
Attachment #8801393 - Flags: review?(jib) → review-
Comment on attachment 8801393 [details]
Bug 1310355 Improve resiliency of webrtc add-on hooks

https://reviewboard.mozilla.org/r/86146/#review85694

::: browser/modules/webrtcUI.jsm:211
(Diff revision 1)
> +  addPeerConnectionBlocker: function(aCallback) {
> +    this.peerConnectionBlockers.add(aCallback);
> +  },
> +
> +  removePeerConnectionBlocker: function(aCallback) {
> +    this.peerConnectionBlockers.delete(aCallback);
> +  },

Hm, good point, there's no good reason.  I originally wanted to use `EventEmitter.decorate()` like the devtools EventEmitter has (http://searchfox.org/mozilla-central/source/devtools/shared/event-emitter.js#81-94) but the version I'm using here doesn't support that.  If you don't have a preference, I'll manually put on/off methods onto webrtcUI (ie wrap the emitter).

::: browser/modules/webrtcUI.jsm:219
(Diff revision 1)
> +  receiveMessage: function(aMessage) {
> +    switch (aMessage.name) {
>  
>        case "rtcpeer:Request": {
> -        // Always allow. This code-point exists for add-ons to override.
> -        let { callID, windowID } = aMessage.data;
> +        let params = Object.assign({origin: aMessage.target.contentPrincipal.origin},
> +                                   aMessage.data);
> -        // Also available: isSecure, innerWindowID. For contentWindow:
> -        //
> -        //   let contentWindow = Services.wm.getOuterWindowWithId(windowID);
>  
>          let mm = aMessage.target.messageManager;
> -        mm.sendAsyncMessage("rtcpeer:Allow",
> -                            { callID: callID, windowID: windowID });
> +        function answer(reply) {
> +          if (reply == "rtcpeer:Allow") {
> +            this.emitter.emit("peer-request", params);
> +          }

I wasn't sure about this, since I don't actually have any immediate applications for passive observation of peer requests.  But it seemed logical that if you wanted to observe them, that you wouldn't want to see requests that were blocked.  Other options would be to create a separate event for blocked requests or always emit this event after a decision and add a parameter indicating whether it was allowed or not.
Any preference?

::: browser/modules/webrtcUI.jsm:223
(Diff revision 1)
> -        // Always allow. This code-point exists for add-ons to override.
> -        let { callID, windowID } = aMessage.data;
> +        let params = Object.assign({origin: aMessage.target.contentPrincipal.origin},
> +                                   aMessage.data);

Good point, to avoid the cost of copying (or changing the emitter) I've just changed it to freeze the params object.

::: browser/modules/webrtcUI.jsm:242
(Diff revision 1)
> +
> +        let blockers = Array.from(this.peerConnectionBlockers);
> +        function next() {
> +          if (blockers.length == 0) {
> +            answer("rtcpeer:Allow");
> +            return undefined;

Right but then eslint complains about a function with a mix of valued and unvalued return statements...

::: browser/modules/webrtcUI.jsm:245
(Diff revision 1)
> +          let blocker = blockers.shift();
> +          return Promise.resolve(blocker(params)).then(result => {

Good catch (pun partially intended)
Comment on attachment 8801393 [details]
Bug 1310355 Improve resiliency of webrtc add-on hooks

https://reviewboard.mozilla.org/r/86146/#review86298

Lgtm.

::: browser/modules/webrtcUI.jsm:236
(Diff revisions 1 - 2)
> +        let emitter = this.emitter;
>          function answer(reply) {
>            if (reply == "rtcpeer:Allow") {
> -            this.emitter.emit("peer-request", params);
> +            emitter.emit("peer-request", params);

Let's make answer an arrow function instead.

::: browser/modules/webrtcUI.jsm:248
(Diff revisions 1 - 2)
>              callID: params.callID,
>              windowID: params.windowID,
>            });
>          }
>  
> -        let blockers = Array.from(this.peerConnectionBlockers);
> +        let blockers = [ ...Array.from(this.peerConnectionBlockers),

Do we need Array.from() here when this.peerConnectionBlockers is already iterable?

::: browser/modules/webrtcUI.jsm:255
(Diff revisions 1 - 2)
> +            .catch(err => {
> +              Cu.reportError(`error in PeerConnection blocked: ${err.message}`);
> +            })

Nit: { } brackets not needed.
Attachment #8801393 - Flags: review?(jib) → review+
Comment on attachment 8801393 [details]
Bug 1310355 Improve resiliency of webrtc add-on hooks

https://reviewboard.mozilla.org/r/86146/#review85694

> I wasn't sure about this, since I don't actually have any immediate applications for passive observation of peer requests.  But it seemed logical that if you wanted to observe them, that you wouldn't want to see requests that were blocked.  Other options would be to create a separate event for blocked requests or always emit this event after a decision and add a parameter indicating whether it was allowed or not.
> Any preference?

Forgot about this. We should be consistent with naming whichever way we go. If it's called "rtcpeer:Request" then it should fire when a request is made IMHO, so that "rtcpeer:CancelRequest" makes sense. That is, every "rtcpeer:Request" is matched by either "Allow", "Deny" or "rtcpeer:CancelRequest".
Comment on attachment 8801393 [details]
Bug 1310355 Improve resiliency of webrtc add-on hooks

https://reviewboard.mozilla.org/r/86146/#review86298

> Nit: { } brackets not needed.

I used the brackets to ensure that anything returned from `Cu.reportError()` didn't actually leak down to the next handler, but it looks like reportError doesn't return anything...
Comment on attachment 8801393 [details]
Bug 1310355 Improve resiliency of webrtc add-on hooks

https://reviewboard.mozilla.org/r/86146/#review85694

> Forgot about this. We should be consistent with naming whichever way we go. If it's called "rtcpeer:Request" then it should fire when a request is made IMHO, so that "rtcpeer:CancelRequest" makes sense. That is, every "rtcpeer:Request" is matched by either "Allow", "Deny" or "rtcpeer:CancelRequest".

Yeah this part is kind of clunky, I think the cancel request event is really only useful if you've registered a blocking handler for peer connections (to let you know that the request has been canceled so for example an interactive prompt to the user can be dismissed).  But I'll add another peer-request-blocked event, so passive observers can see all 3 possibilities if they're interested...
Comment on attachment 8801393 [details]
Bug 1310355 Improve resiliency of webrtc add-on hooks

https://reviewboard.mozilla.org/r/86146/#review86298

Thanks, just pushed another revision addressing the most recent comments.
Two testing questions before landing: should this code include any tests?  (I've tested it manually with a webextensions experiment but do we want something in the test suite to catch potential future regressions?)  Also, I'm not that familiar with existing webrtc tests, can you suggest a good subset of tests to run on try before landing?
Comment on attachment 8801393 [details]
Bug 1310355 Improve resiliency of webrtc add-on hooks

https://reviewboard.mozilla.org/r/86146/#review86408

::: browser/modules/webrtcUI.jsm:18
(Diff revision 3)
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  
>  XPCOMUtils.defineLazyModuleGetter(this, "AppConstants",
>                                    "resource://gre/modules/AppConstants.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "EventEmitter",

The module getter is lazy here, but "new EventEmitter()" will be called while initializing the webrtcUI object 6 lines later.

Either make this a normal Cu.import, or find a way to make 'emitter' a lazy getter on the webrtcUI object.

::: browser/modules/webrtcUI.jsm:251
(Diff revision 3)
> +
> +          mm.sendAsyncMessage(reply, {
> +            callID: params.callID,
> +            windowID: params.windowID,
> +          });
> +        }

nit: missing ;

::: browser/modules/webrtcUI.jsm:258
(Diff revision 3)
> +        let blockers = [ ...this.peerConnectionBlockers, () => "allow" ];
> +
> +        function next() {
> +          let blocker = blockers.shift();
> +          return Promise.resolve()
> +            .then(() => blocker(params))

Is there a way to make this code easier to follow using a task instead of this cascade of promises?

::: browser/modules/webrtcUI.jsm:259
(Diff revision 3)
> +
> +        function next() {
> +          let blocker = blockers.shift();
> +          return Promise.resolve()
> +            .then(() => blocker(params))
> +            .catch(err => Cu.reportError(`error in PeerConnection blocked: ${err.message}`))

nit: 'blocker' not 'blocked'.
(In reply to Andrew Swan [:aswan] from comment #10)

> Two testing questions before landing: should this code include any tests? 
> (I've tested it manually with a webextensions experiment but do we want
> something in the test suite to catch potential future regressions?)

This would be very desirable, if you don't want this to break.

Existing tests for behaviors implemented in webrtcUI.jsm are in browser/base/content/test/webrtc/

> Also,
> I'm not that familiar with existing webrtc tests, can you suggest a good
> subset of tests to run on try before landing?

You need at least mochitest-bc,mochitest-e10s-bc to run the tests in browser/base/content/test/webrtc/

I think you'll also want mochitest-media,mochitest-media-e10s to run webrtc mochitests implemented in dom/media.
I'm in the process of writing tests and will push (hopefully just) one more revision with the tests.
In the process of writing tests, it occurred to me that the current behavior of having a connection be immediately allowed if a blocking handler returns "allow" is bogus, we should immediately deny if a handler says "deny" but continue to give other handlers a chance to handle connections that are explicitly allowed by one handler.
The whole scenario (multiple blockers) seems rather unlikely to actually happen in practice but we should still handle it properly.
Florian/Jan, thank you for the reviews, the latest patch on mozreview addresses Florian's comments, adds some tests, and applies the change described in comment 14.  I think this should be the last big change but its enough that it could use another look over from you guys, thanks!
Since I am completely inept with trychooser, the relevant tests here are split across two runs.
This run covers the mochitest that is added in the patch for this bug (it doesn't cover linux since I fat-fingered it, it would have tested "linuw" if such a platform existed):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1dee5198a220
And this run covers the existing media tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e54dad72035
Setting ni? for the extra review request from comment 16
Flags: needinfo?(jib)
Flags: needinfo?(florian)
Comment on attachment 8801393 [details]
Bug 1310355 Improve resiliency of webrtc add-on hooks

https://reviewboard.mozilla.org/r/86146/#review87190

Some feedback. Haven't reviewed the test yet. Will try to get to it later today or tomorrow.

::: browser/modules/webrtcUI.jsm:243
(Diff revisions 2 - 4)
>            if (reply == "rtcpeer:Allow") {
> -            emitter.emit("peer-request", params);
> +            this.emitter.emit("peer-request", params);

Shouldn't this be "peer-request-allowed" then? That would make more sense to me.

If so, then without a "peer-request" event (which signals the start of a user request), then "peer-request-cancel" (which signals the user request was aborted) doesn't make any sense to me. What's the plausible use-case here again?

::: browser/modules/webrtcUI.jsm:255
(Diff revisions 2 - 4)
> -        let blockers = [ ...Array.from(this.peerConnectionBlockers),
> -                         () => "allow" ];
> +        let blockers = Array.from(this.peerConnectionBlockers);
> +

Why lose this invariant?

::: browser/modules/webrtcUI.jsm:257
(Diff revisions 2 - 4)
> +        let next = Task.async(function*() {
> +          if (blockers.length == 0) {
> +            return "rtcpeer:Allow";
> +          }
>  
> -        function next() {
>            let blocker = blockers.shift();
> -          return Promise.resolve()
> -            .then(() => blocker(params))
> -            .catch(err => {
> -              Cu.reportError(`error in PeerConnection blocked: ${err.message}`);
> -            })
> +          let result;
> +          try {
> +            result = yield blocker(params);
> +          } catch (err) {
> +            Cu.reportError(`error in PeerConnection blocker: ${err.message}`);
> -            .then(result => {
> -              if (result == "allow") {
> -                answer("rtcpeer:Allow");
> -                return undefined;
> -              }
> +          }
> +
> -              if (result == "deny") {
> +          if (result == "deny") {
> -                answer("rtcpeer:Deny");
> +            return "rtcpeer:Deny";
> -                return undefined;
> -              }
> +          }
> -              return next();
> +          return yield next();
> -            });
> +        });

Do we still need to recurse? Why not:

    Task.async(function*() {
      for (blocker of blockers) {
        let result;
        try {
          if (yield blocker(params) == "deny") {
            return "rtcpeer:Deny";
          }
        } catch (err) {
          Cu.reportError(`error in PeerConnection blocker: ${err.message}`);
        }
      }
    })().then(answer);

?
Comment on attachment 8801393 [details]
Bug 1310355 Improve resiliency of webrtc add-on hooks

https://reviewboard.mozilla.org/r/86146/#review87190

> Shouldn't this be "peer-request-allowed" then? That would make more sense to me.
> 
> If so, then without a "peer-request" event (which signals the start of a user request), then "peer-request-cancel" (which signals the user request was aborted) doesn't make any sense to me. What's the plausible use-case here again?

Updated peer-request to peer-request-allowed, I'll upload the new patch momentarily.
peer-request-cancel is described in the comment but it is useful in conjunction with a blocker.  For example, if a blocker creates a dialog asking the user to allow or deny and then the request is canceled, the dialog can be dismissed without waiting for a decision from the user.

> Why lose this invariant?

Because with the change described in a comment on the bug, "allow" doesn't immediately allow the connection, it gives other blockers a chance to run.  So this doesn't terminate the loop, it is now just superfluous.

> Do we still need to recurse? Why not:
> 
>     Task.async(function*() {
>       for (blocker of blockers) {
>         let result;
>         try {
>           if (yield blocker(params) == "deny") {
>             return "rtcpeer:Deny";
>           }
>         } catch (err) {
>           Cu.reportError(`error in PeerConnection blocker: ${err.message}`);
>         }
>       }
>     })().then(answer);
> 
> ?

Yep, that's much nicer, thanks!
Comment on attachment 8801393 [details]
Bug 1310355 Improve resiliency of webrtc add-on hooks

https://reviewboard.mozilla.org/r/86146/#review87290

::: browser/base/content/test/webrtc/browser_webrtc_hooks.js:35
(Diff revision 5)
> +  }
> +}
> +
> +var gTests = [
> +  {
> +    desc: "Basic media-permisisons event",

typo: permisisons

::: browser/base/content/test/webrtc/browser_webrtc_hooks.js:37
(Diff revision 5)
> +
> +var gTests = [
> +  {
> +    desc: "Basic media-permisisons event",
> +    run: function* testMediaPermissionsEvent() {
> +      let eventPromise = new Promise(resolve => {

This is duplicated several times, make it a helper function that takes the name of the event as a parameter and returns a promise.

::: browser/base/content/test/webrtc/browser_webrtc_hooks.js:159
(Diff revision 5)
> +      let blocker = params => "deny";
> +      webrtcUI.addPeerConnectionBlocker(blocker);
> +
> +      let peerRequestFired = false;
> +      let prListener = () => { peerRequestFired = true; };
> +      webrtcUI.on("peer-request-allowed", prListener);

Should we also check that peer-request-blocked isn't fired for the previous tests?

::: browser/base/content/test/webrtc/browser_webrtc_hooks.js:178
(Diff revision 5)
> +
> +      webrtcUI.removePeerConnectionBlocker(blocker);
> +      webrtcUI.off("peer-request-allowed");
> +
> +      is(webrtcUI.peerConnectionBlockers.size, 0, "Peer connection blockers list is empty");
> +      ok(!peerRequestFired, "peer-request event is not triggered for blocked peer connection");

peer-request-allowed event. (Check this throughout the file, there are several occurences of check descriptions that include the peer-request event name.)

::: browser/base/content/test/webrtc/browser_webrtc_hooks.js:185
(Diff revision 5)
> +  },
> +
> +  {
> +    desc: "Multiple blockers work (both allow)",
> +    run: function* testMultipleAllowBlockers(browser) {
> +      let resolve1, promise1 = new Promise(resolve => { resolve1 = resolve; });

I don't think you need the resolve1 variable, as you don't need to need to access the resolve function outside of the promise callback:

let blocker1, promise1 = new Promise(resolve => {
  blocker1 = params => {
    resolve();
    return "allow";
  };
});

This applies throughout the file.

::: browser/base/content/test/webrtc/browser_webrtc_hooks.js:225
(Diff revision 5)
> +    desc: "Multiple blockers work (allow then deny)",
> +    run: function* testAllowDenyBlockers(browser) {
> +      let resolve1, promise1 = new Promise(resolve => { resolve1 = resolve; });
> +      let blocker1 = params => {
> +        resolve1();
> +        return "allow";

For this specific test it would make a lot of sense to verify that we are not firing the peer-request-allowed event.

::: browser/base/content/test/webrtc/browser_webrtc_hooks.js:394
(Diff revision 5)
> +      yield ContentTask.spawn(browser, null, function*() {
> +        let pc = new content.RTCPeerConnection();
> +        pc.createOffer({offerToReceiveAudio: true});
> +      });
> +
> +      yield blockerPromise;

I think you want to start waiting for the peer-request-cancel event here, after the blockerPromise has resolved, and before reloading the page.

::: browser/modules/ContentWebRTC.jsm:48
(Diff revision 5)
>      this._initialized = false;
>    },
>  
>    // Called only for 'unload' to remove pending gUM prompts in reloaded frames.
>    handleEvent: function(aEvent) {
> +    dump("in handleEvent\n");

please remove this debug line.

::: browser/modules/webrtcUI.jsm:204
(Diff revision 5)
> +  //                        blocked by some blocking connection handler.
> +  //   peer-request-cancel is emitted when a peer-request connection request
> +  //                       is canceled.  (This would typically be used in
> +  //                       conjunction with a blocking handler to cancel
> +  //                       a user prompt or other work done by the handler)
> +  //   media-permissions is emitted when new getUserMedia() user permissions

I don't understand what "when new getUserMedia() user permissions are established" means.

Or rather I could understand it as meaning one of several things:
- setting/removing persistent permissions for gUM devices
- the user granting access to devices
- starting a stream (whether started using persistent permissions, or after prompting the user)
- accessing the current permissions.

I think this comment should include an example.

[Edit after reading the actual code: it actually does something that I couldn't have guessed; it fires whenever the UI sharing indicators need to be updated.]

::: browser/modules/webrtcUI.jsm:270
(Diff revision 5)
> +              Cu.reportError(`error in PeerConnection blocker: ${err.message}`);
> +            }
> +          }
> +          return "rtcpeer:Allow";
> +        }).then(msg => {
> +          answer(msg);

This can be simplified to .then(answer);

But it seems it would be even simpler to replace the two return "rtcpeer... lines in the task with answer("rtcpeer...

Another possibility is to just inline the answer function here:
}).then(reply => {
  if (reply == "rtcpeer:Allow") {
...
});

I think I prefer this third idea because the code is then written in the order in which it will run, making it easier to follow.

::: browser/modules/webrtcUI.jsm:299
(Diff revision 5)
>        case "webrtc:UpdateBrowserIndicators":
> +        // Beware, this will need to change when https://bugzil.la/1299577 lands
> +        let origin = aMessage.target.contentPrincipal.origin;
> +        let {camera, microphone} = aMessage.data;
> +        let params = Object.freeze({camera, microphone, origin});
> +        this.emitter.emit("media-permissions", params);

I have doubts about the usefulness of this event. What are the use cases?
Comment on attachment 8801393 [details]
Bug 1310355 Improve resiliency of webrtc add-on hooks

https://reviewboard.mozilla.org/r/86146/#review87332

Tests look great! Just nits. Like Florian I'd like to see less repetition, using helpers. I'd also love less state, like avoiding the deferred pattern, and avoiding promises altogether where flags will do.

::: browser/base/content/test/webrtc/browser_webrtc_hooks.js:13
(Diff revision 5)
> +
> +registerCleanupFunction(function() {
> +  gBrowser.removeCurrentTab();
> +});
> +
> +const permissionError = "The request is not allowed by the user agent or the platform in the current context.";

We should test error.name == "NotAllowedError" rather than the actual error.message.

::: browser/base/content/test/webrtc/browser_webrtc_hooks.js:15
(Diff revision 5)
> +  gBrowser.removeCurrentTab();
> +});
> +
> +const permissionError = "The request is not allowed by the user agent or the platform in the current context.";
> +
> +function* makePeerConnection(browser, expectPermissionError = false) {

Nit: Boolean args are hard to make sense of at the call site. I suggest passing in expectedError instead (default to null). Then it's more obvious what the argument does, e.g.:

    yield makePeerConnection(browser, "NotAllowedError");

I also dislike the name of this function as it sounds like it should return a peer connection or fail, when what it does is more of a test itself (that never fails).

testCreateOfferPermission?

::: browser/base/content/test/webrtc/browser_webrtc_hooks.js:26
(Diff revision 5)
> +  if (expectPermissionError) {
> +    is(errmsg, permissionError, "createOffer() threw a permission error for a blocked peer connection");
> +  } else {
> +    is(errmsg, null, "createOffer() succeeded");
> +  }

Then this folds to:

    is(err.name, expectedError, "...");

::: browser/base/content/test/webrtc/browser_webrtc_hooks.js:62
(Diff revision 5)
> +      let eventPromise = new Promise(resolve => {
> +        let listener = details => {
> +          webrtcUI.off("peer-request-allowed", listener);
> +          resolve(details);
> +        };
> +        webrtcUI.on("peer-request-allowed", listener);
> +      });

Tip: We can avoid { } brackets here by using a named function (like onload below):

    let eventPromise = new Promise(resolve => 
        webrtcUI.on("peer-request-allowed", function listener(details) {
      webrtcUI.off("peer-request-allowed", listener);
      resolve(details);
    }));

::: browser/base/content/test/webrtc/browser_webrtc_hooks.js:82
(Diff revision 5)
> +      let blockerResolve;
> +      let blockerPromise = new Promise(resolve => {
> +        blockerResolve = resolve;
> +      });
> +
> +      let blocker = params => {
> +        is(params.origin, ORIGIN, "Peer connection blocker origin parameter is correct");
> +        blockerResolve();
> +        return "allow";
> +      };
> +      webrtcUI.addPeerConnectionBlocker(blocker);

Let's avoid the deferred pattern whenever possible by putting this inside the promise constructor executor function (applies throughout).

::: browser/base/content/test/webrtc/browser_webrtc_hooks.js:147
(Diff revision 5)
> +      isnot(details.callID, undefined, "peer-request event includes callID");
> +      isnot(details.windowID, undefined, "peer-request event includes windowID");
> +      is(details.origin, ORIGIN, "peer-request event has correct origin");
> +
> +      webrtcUI.removePeerConnectionBlocker(blocker);
> +      is(webrtcUI.peerConnectionBlockers.size, 0, "Peer connection blockers list is empty");

Maybe testing this just once is sufficient?

::: browser/base/content/test/webrtc/browser_webrtc_hooks.js:209
(Diff revision 5)
> +        webrtcUI.on("peer-request-allowed", listener);
> +      });
> +
> +      yield makePeerConnection(browser);
> +
> +      let [, , details] = yield Promise.all([promise1, promise2, eventPromise]);

promise1 and promise2 seem like glorified flags. In fact, I see sometimes you use flags (blocker2Called).

If we used flags throughout then tests would report failure rather than just timeout like it would now, which I think would be better.

Applies throughout.

::: browser/base/content/test/webrtc/browser_webrtc_hooks.js:259
(Diff revision 5)
> +  },
> +
> +  {
> +    desc: "Multiple blockers work (deny first)",
> +    run: function* testDenyAllowBlockers(browser) {
> +      let resolve1, promise1 = new Promise(resolve => { resolve1 = resolve; });

Nit: redundant { }

::: browser/base/content/test/webrtc/browser_webrtc_hooks.js:390
(Diff revision 5)
> +        let pc = new content.RTCPeerConnection();
> +        pc.createOffer({offerToReceiveAudio: true});

Nit: one-liner (pc is redundant).
If you need further review, you can also set the r? mark on the attachment in bugzilla, but this worked.
Flags: needinfo?(jib)
(In reply to Jan-Ivar Bruaroey [:jib] from comment #23)

> ::: browser/base/content/test/webrtc/browser_webrtc_hooks.js:147
> (Diff revision 5)
> > +      isnot(details.callID, undefined, "peer-request event includes callID");
> > +      isnot(details.windowID, undefined, "peer-request event includes windowID");
> > +      is(details.origin, ORIGIN, "peer-request event has correct origin");
> > +
> > +      webrtcUI.removePeerConnectionBlocker(blocker);
> > +      is(webrtcUI.peerConnectionBlockers.size, 0, "Peer connection blockers list is empty");
> 
> Maybe testing this just once is sufficient?

It's easier to make sense of test failure logs if we can be sure that the previous test didn't leave anything behind, so I think it makes sense to test this all the time. Actually, I would suggest moving this test to the "test" function after the "yield test.run(browser);" line.
Flags: needinfo?(florian)
Comment on attachment 8801393 [details]
Bug 1310355 Improve resiliency of webrtc add-on hooks

https://reviewboard.mozilla.org/r/86146/#review87290

> This is duplicated several times, make it a helper function that takes the name of the event as a parameter and returns a promise.

Florian pointed out that for the most part, we don't actually need the Promise for handling the flow of control, I've tried to abstract away the repeated bits with the `Events` object which gets rid of a bunch of the repeated code...

> Should we also check that peer-request-blocked isn't fired for the previous tests?

The new Events object does this automatically and throughout the tests.

> I think you want to start waiting for the peer-request-cancel event here, after the blockerPromise has resolved, and before reloading the page.

Sorry I didn't follow this, the listener for the -cancel event is already set up at this point but we can't block waiting for it here since it is caused by the reload which happens below.

> I don't understand what "when new getUserMedia() user permissions are established" means.
> 
> Or rather I could understand it as meaning one of several things:
> - setting/removing persistent permissions for gUM devices
> - the user granting access to devices
> - starting a stream (whether started using persistent permissions, or after prompting the user)
> - accessing the current permissions.
> 
> I think this comment should include an example.
> 
> [Edit after reading the actual code: it actually does something that I couldn't have guessed; it fires whenever the UI sharing indicators need to be updated.]

This is what the rest of the comment immediately following this line is meant to explain.  I haven't really changed anything here, this is the feature that was added as part of bug 1189060 (and is used by this extension for instance https://github.com/fippo/plumber).  This patch just lets addons access it by listening to an event rather than overriding receiveMessage().  If you think there's something else that would be more appropriate for this use case, can that be discussed/implemented in a separate bug?

> I have doubts about the usefulness of this event. What are the use cases?

(see comment above)
Comment on attachment 8801393 [details]
Bug 1310355 Improve resiliency of webrtc add-on hooks

https://reviewboard.mozilla.org/r/86146/#review87332

> Let's avoid the deferred pattern whenever possible by putting this inside the promise constructor executor function (applies throughout).

A bunch of this went away with the event handling refactoring described above, the cancel test is the only one where I had to keep promises to maintain the correct sequence of events.
But I made the changes suggested above in that test case.
(In reply to Andrew Swan [:aswan] from comment #26)
> I haven't really changed anything here, this is the
> feature that was added as part of bug 1189060 (and is used by this extension
> for instance https://github.com/fippo/plumber).

The add-on is not doing what it thinks it does. This message is also emitted when the sharing indicators should be removed. It seems the add-on wants to know when a stream is starting.

Also, this has not been intentionally added in bug 1189060, which was specifically about peer connection (createOffer() and createAnswer()).
Comment on attachment 8801393 [details]
Bug 1310355 Improve resiliency of webrtc add-on hooks

https://reviewboard.mozilla.org/r/86146/#review89612

r- for the media-permissions event. We should either implement instead something that matches what add-ons need, or leave this out entirely. And if we do implement an event, the comment next to it should give an accurate description.

::: browser/base/content/test/webrtc/browser_webrtc_hooks.js:52
(Diff revisions 5 - 6)
> +      this.handlers.set(event, handler);
> -  }
> +    }
> +  },
> +  expect(event) {
> +    let result = this.details.get(event);
> +    isnot(result, undefined, `${event} event was triggered`);

Should we check how many times the event has been triggered, so that the test fails if the code ever fires duplicate events?

::: browser/base/content/test/webrtc/browser_webrtc_hooks.js:73
(Diff revisions 5 - 6)
> +  },
> +};
>  
>  var gTests = [
>    {
> -    desc: "Basic media-permisisons event",
> +    desc: "Basic media-permisions event",

There's still a typo on this word: "permis*s*ions" not "permisions".

::: browser/base/content/test/webrtc/browser_webrtc_hooks.js:130
(Diff revisions 5 - 6)
> -      isnot(details.callID, undefined, "peer-request event includes callID");
> -      isnot(details.windowID, undefined, "peer-request event includes windowID");
> -      is(details.origin, ORIGIN, "peer-request event has correct origin");
>  
>        webrtcUI.removePeerConnectionBlocker(blocker);
>        is(webrtcUI.peerConnectionBlockers.size, 0, "Peer connection blockers list is empty");

Did you not like my suggestion (comment 25) of moving this at the bottom of the test file, outside of the individual tests?

::: browser/base/content/test/webrtc/browser_webrtc_hooks.js:327
(Diff revisions 5 - 6)
> -        return new Promise(resolve => {});
> +          return new Promise(innerResolve => {});
> -      };
> +        };
> +      });
>        webrtcUI.addPeerConnectionBlocker(blocker);
>  
>        let eventPromise = new Promise(resolve => {

This block (starting with "let eventPromise = ") should be moved after the "yield blockerPromise;" line (but stay before the block that will call "location.reload"). This is what I meant in comment 22.
Attachment #8801393 - Flags: review-
(In reply to Florian Quèze [:florian] [:flo] from comment #29)
> (In reply to Andrew Swan [:aswan] from comment #26)
> > I haven't really changed anything here, this is the
> > feature that was added as part of bug 1189060 (and is used by this extension
> > for instance https://github.com/fippo/plumber).
> 
> The add-on is not doing what it thinks it does. This message is also emitted
> when the sharing indicators should be removed. It seems the add-on wants to
> know when a stream is starting.

Sorry I'm not following, what is the scenario that you think is broken?

> Also, this has not been intentionally added in bug 1189060, which was
> specifically about peer connection (createOffer() and createAnswer()).

Okay, if the ask is for a way for addons to know that getUserMedia() permission has been granted (either as a one-time thing or persistently granted) to avoid double-prompting, what technique would you suggest they use?
(In reply to Andrew Swan [:aswan] from comment #31)
> (In reply to Florian Quèze [:florian] [:flo] from comment #29)
> > (In reply to Andrew Swan [:aswan] from comment #26)
> > > I haven't really changed anything here, this is the
> > > feature that was added as part of bug 1189060 (and is used by this extension
> > > for instance https://github.com/fippo/plumber).
> > 
> > The add-on is not doing what it thinks it does. This message is also emitted
> > when the sharing indicators should be removed. It seems the add-on wants to
> > know when a stream is starting.
> 
> Sorry I'm not following, what is the scenario that you think is broken?

The code in the current patch exposes an implementation detail of the current implementation, we don't want to guarantee the behaviors associated to the webrtc:UpdateBrowserIndicators message won't change.
Looking at the add-on, what's broken is that it'll mistakenly record information at the time a stream stops.

> > Also, this has not been intentionally added in bug 1189060, which was
> > specifically about peer connection (createOffer() and createAnswer()).
> 
> Okay, if the ask is for a way for addons to know that getUserMedia()
> permission has been granted (either as a one-time thing or persistently
> granted) to avoid double-prompting, what technique would you suggest they
> use?

Send an event to addons near the two "mm.sendAsyncMessage("webrtc:Allow"..." lines in webrtcUI.jsm. You may want to add details on the event to distinguish the two (one is when we are automatically granting access due to persistent permissions, the other is when permission has been granted due to a user action).
Sorry for letting this one slide for a few weeks.  I've removed the media-permissions stuff, that can be left for a follow-up.
And hopefully addressed the other comments...
... and with a rebase onto recent central there are some eslint issues to clean up.  I'll push a fix here, but mozreview interdiffs for rebases are always a mess.
Comment on attachment 8801393 [details]
Bug 1310355 Improve resiliency of webrtc add-on hooks

Arg I tried to re-request review from Florian over on mozreview but it looks like it never propagated over here
Attachment #8801393 - Flags: review?(florian)
Comment on attachment 8801393 [details]
Bug 1310355 Improve resiliency of webrtc add-on hooks

https://reviewboard.mozilla.org/r/86146/#review100296

r=me (focused on verifying the points discussed above have been satisfyingly addressed and didn't do a thorough review of everything again)

::: browser/base/content/test/webrtc/browser_webrtc_hooks.js:201
(Diff revision 8)
> +      ok(blocker2Called, "Second blocker was called");
> +
> +      webrtcUI.removePeerConnectionBlocker(blocker1);
> +      webrtcUI.removePeerConnectionBlocker(blocker2);
> +      Events.off();
> +

nit: remove useless empty line.
Attachment #8801393 - Flags: review?(florian) → review+
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eba5bc9fc212
Improve resiliency of webrtc add-on hooks r=florian,jib
https://hg.mozilla.org/mozilla-central/rev/eba5bc9fc212
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: