Closed Bug 1167922 Opened 9 years ago Closed 9 years ago

Handle broken entries in media.peerconnection.default_iceservers more gracefully

Categories

(Core :: WebRTC: Signaling, defect, P4)

defect

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: drno, Assigned: rshenthar, Mentored)

References

Details

(Whiteboard: [good first bug])

Attachments

(9 files, 5 obsolete files)

40 bytes, text/x-review-board-request
Details
40 bytes, text/x-review-board-request
Details
40 bytes, text/x-review-board-request
Details
40 bytes, text/x-review-board-request
Details
40 bytes, text/x-review-board-request
Details
40 bytes, text/x-review-board-request
jib
: review+
Details
40 bytes, text/x-review-board-request
Details
40 bytes, text/x-review-board-request
Details
40 bytes, text/x-review-board-request
Details
Current code in PeerConnection.js assumes that media.peerconnection.default_iceservers contains at least an empty list. As https://bugzilla.mozilla.org/show_bug.cgi?id=1143827#c20 points out other values are not very nicely handled right now.
See Also: → 1143827
give intelligent error when someone passes not good values.  web devs are the ones likely to hit - so more edge.
backlog: --- → webRTC+
Rank: 45
Priority: -- → P4
Assignee: nobody → r.shenthar
Mentor: drno
Whiteboard: [good first bug]
Assignee: r.shenthar → rshenthar
Bug 1167922: Handle broken entries in media.peerconnection.default_iceservers more gracefully
Comment on attachment 8620395 [details]
MozReview Request: Bug 1167922: Handle broken entries in media.peerconnection.default_iceservers more gracefully

Bug 1167922: Handle broken entries in media.peerconnection.default_iceservers more gracefully
Attachment #8620395 - Flags: review?(jib)
Comment on attachment 8620395 [details]
MozReview Request: Bug 1167922: Handle broken entries in media.peerconnection.default_iceservers more gracefully

https://reviewboard.mozilla.org/r/10745/#review9423

Thanks, a couple of issues need addressing.

::: dom/media/PeerConnection.js:337
(Diff revision 1)
> +          throw new this._win.DOMException("Input is not an array", "Invalid User preference setting for media.peerconnection.default_iceservers");

Same here.

::: dom/media/PeerConnection.js:341
(Diff revision 1)
>      rtcConfig.iceServers.forEach(server => {

While WebIDL guarantees validity of any rtcConfig.iceServers passed-in from content scripts, none of these guarantees apply to the about:config back door.

We know rtcConfig.iceServers is an array here, but it may still contain things that aren't valid, which may cause TypeErrors when the following code iterates through and reads it.

The safest thing to do here would be to copy out the known pieces of the array from prefs first, and do so inside the try-catch. Then use the copy.

::: dom/media/PeerConnection.js:332
(Diff revision 1)
> +          throw new this._win.DOMException("Invalid input", "Invalid User preference setting for media.peerconnection.default_iceservers");

Error-name is the second argument in our DOMException constructor, not the first. [1]

Also, the error-name must be a pre-existing approved name, either from the webidl spec [2] or (worst case) from the webrtc spec [3] (which errors still need some baking).

Which one to pick is tricky, since this about:config "feature" is not part of any spec. Also, this isn't really "input" from the viewpoint of the content script, which rules out "TypeError" or anything implying input, as there's nothing a web developer can do in response to it, since this is really a misconfiguration of the browser. From discussions in #content yesterday I think we landed on "InvalidStateError" being the most descriptive here (we don't want to create new types of errors when we can avoid it).

Lets go with that.

Also maybe s/Invalid User preference setting for/User-misconfiguration of browser's/ or something like that.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/webidl/DOMException.webidl?rev=fea8c83aef60&mark=78-78#74
[2] http://heycam.github.io/webidl/#dfn-exception-error-name
[3] http://w3c.github.io/webrtc-pc/#rtcsdperror
Change of plan a little bit. After thinking about this and chatting with jesup in #media, we think it's better to not fail over this.  Instead, we should log the error to web console and continue.

It's not spec-compliant to block webrtc here by failing with InvalidStateError over this. Most likely the call would have succeeded anyway with the ice servers the content script was required to provide, and if it doesn't then there's nothing the web developer can do about it, and it seems no different than if no custom ice servers were configured (which is the default).

If the user is in a bunker where the custom ice servers are required for anything to work, then the information in web console should be enough to pinpoint the problem.

Does that sound good?
Bug 1167922: Incorporated review comments to continue after logging warning
Attachment #8621810 - Flags: review?(jib)
Comment on attachment 8621810 [details]
MozReview Request: Bug 1167922: Incorporated review comments to continue after logging warning

https://reviewboard.mozilla.org/r/11057/#review9647

Good, except we lost the constructor input validation, and we could emit useful info in the warnings.

::: dom/media/PeerConnection.js
(Diff revision 1)
> -    this._mustValidateRTCConfiguration(rtcConfig,
> -        "RTCPeerConnection constructor passed invalid RTCConfiguration");

We're losing the constructor input validation here, which is not ok.

::: dom/media/PeerConnection.js:461
(Diff revision 1)
>     * WebIDL normalizes structure for us, so we test well-formed stun/turn urls,
>     * but not validity of servers themselves, before passing along to C++.

Add to this comment that this function now also validates raw input from about:config.

::: dom/media/PeerConnection.js:328
(Diff revision 1)
>        try {
>            rtcConfig.iceServers =
>                JSON.parse(Services.prefs.getCharPref("media.peerconnection.default_iceservers"));
> +          this._mustValidateRTCConfiguration(rtcConfig,
> +              "RTCPeerConnection constructor passed invalid RTCConfiguration");
>        } catch (e) {

Indent everything inside try {} block by 2 spaces less.

::: dom/media/PeerConnection.js:331
(Diff revision 1)
> +          this._mustValidateRTCConfiguration(rtcConfig,
> +              "RTCPeerConnection constructor passed invalid RTCConfiguration");

This passed-in error message is wrong here (even though it ends up not being used, but see below).

::: dom/media/PeerConnection.js:335
(Diff revision 1)
> -      }
> +            "Invalid User preference setting for media.peerconnection.default_iceservers - Setting it to [] i.e default value",null,0);

This line is a little long, both for wordwrap reasons and brievity. Maybe also mention about:config? How about:

    "Ignoring invalid media.peerconnection.default_iceservers in about:config"

?

Also spaces after commas.

::: dom/media/PeerConnection.js:333
(Diff revision 1)
>        } catch (e) {
> -          throw new this._win.DOMException("Invalid input", "Invalid User preference setting for media.peerconnection.default_iceservers");
> -      }
> -      // Check if the input is an array and error out if it is not. The reason for this is that in the User preferences field, the user is expected to
> +          this.logWarning(
> +            "Invalid User preference setting for media.peerconnection.default_iceservers - Setting it to [] i.e default value",null,0);
> +          rtcConfig.iceServers = [];

This is great, but mustValidateRTCConfiguration gives useful errors, and it's a shame they're not emitted here to help about:config users get the syntax right.

Could we move the mustValidateRTCConfiguration call to a separate try/catch block? Something like:

    try {
      this._mustValidateRTCConfiguration(rtcConfig,
          "Ignoring invalid media.peerconnection.default_iceservers in about:config - ");
    } catch (e) {
      this.logWarning(e.message, null, 0);
      rtcConfig.iceServers = [];
    }
Attachment #8621810 - Flags: review?(jib)
Bug 1167922: Handle broken entries in media.peerconnection.default_iceservers more gracefully
Bug 1167922: Incorporated review comments to continue after logging warning
Bug 1167922:Incorporated review changes
Added newline to cause a hg diff
Removed newline to cause a hg diff
Attachment #8622574 - Flags: review?(jib)
https://reviewboard.mozilla.org/r/11393/#review9777

Good, except I don't think this succeeds on empty string does it?

Adding a quick test to this might be appropriate, to make sure this little-used feature stays working.

You could extend dom/media/tests/mochitest/test_peerConnection_bug825703.html which already tests ice servers, with SpecialPowers.pushPrefEnv [1]

Not thinking anything fancy: one test with one valid server, one test with an invalid server, and one test with empty string, should do. 

[1] http://mxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/test_getUserMedia_constraints.html?rev=c4a454c2b1c3&force=1#82

::: dom/media/PeerConnection.js:333
(Diff revision 1)
> +          "Ignoring invalid media.peerconnection.default_iceservers in about:config", null, 0);

We want to try to stay within 80 characters. A little over can be fine, but here there are commas, so I would break at the first one.

::: dom/media/PeerConnection.js:335
(Diff revision 1)
> +      } 

trailing whitespace

::: dom/media/PeerConnection.js:469
(Diff revision 1)
> -   * WebIDL normalizes structure for us, so we test well-formed stun/turn urls,
> -   * but not validity of servers themselves, before passing along to C++.
> +   * This function normalizes the structure of the input for rtcConfig.iceServers for us, 
> +   * so we test well-formed stun/turn urls and the validity of servers themselves, 

trailing whitespace x 2

We're still not testing that the ice servers actually work, so I would phrase it differently.

::: dom/media/PeerConnection.js:329
(Diff revision 1)
> -      rtcConfig.iceServers =
> +        rtcConfig.iceServers =
> -        JSON.parse(Services.prefs.getCharPref("media.peerconnection.default_iceservers"));
> +          JSON.parse(Services.prefs.getCharPref("media.peerconnection.default_iceservers"));

Do we get a warning on empty string? Don't want that.

Putting || []; at the tail of this line should take care of it.

::: dom/media/PeerConnection.js:338
(Diff revision 1)
> +          "RTCPeerConnection constructor passed invalid RTCConfiguration");

Wrong message. Should be:
"Ignoring invalid media.peerconnection.default_iceservers in about:config: "

::: dom/media/PeerConnection.js:344
(Diff revision 1)
> -    rtcConfig.iceServers.forEach(server => {
> +      // This gets executed in the typical case when iceServers are passed in through the web page. 

Shorten (to 80+ chars) or wordwrap comment at 80 chars. remove trailing whitespace
Bug 1167922:Incorporated review changes
Added newline to cause a hg diff
Removed newline to cause a hg diff
Bug 1167922: Incorporated mochitest verification and escaped empty string warning
Attachment #8624972 - Flags: review?(jib)
https://reviewboard.mozilla.org/r/10743/#review10251

Ship it! (with changes)

::: dom/media/PeerConnection.js:329
(Diff revision 3)
> +        var userPrefInput = Services.prefs.getCharPref("media.peerconnection.default_iceservers");
> +        // if userPrefInput = "" i.e an empty string then directly assign it to []
> +        // we do not want to print a warning message in this case.
> +        if (userPrefInput) {
> -      rtcConfig.iceServers =
> +          rtcConfig.iceServers =
> -        JSON.parse(Services.prefs.getCharPref("media.peerconnection.default_iceservers"));
> +            JSON.parse(Services.prefs.getCharPref("media.peerconnection.default_iceservers"));
> +        } else {
> +          rtcConfig.iceServers = [];
> -    }
> +        }

Can be simplified to:

    rtcConfig.iceServers = 
        JSON.parse(Services.prefs.getCharPref("media.peerconnection.default_iceservers") || "[]");

::: dom/media/PeerConnection.js:352
(Diff revision 3)
> +      // This gets executed in the typical case when iceServers 

trailing whitespace

::: dom/media/PeerConnection.js:346
(Diff revision 3)
> +          "Ignoring invalid media.peerconnection.default_iceservers in about:config: ");

Drop the ": ", as _mustValidateRTCConfiguration adds " - " as separation (my bad).

::: dom/media/tests/mochitest/test_peerConnection_bug825703.html:77
(Diff revision 3)
> +  // invoked. See Bug 1167922 for more information. 
> +  // Note - We implement promises here since the SpecialPowers API will be 
> +  // performed asynchronously.  

trailing whitespace and s/implement/use/

::: dom/media/PeerConnection.js:339
(Diff revision 3)
> -    rtcConfig.iceServers.forEach(server => {
> -      if (typeof server.urls === "string") {
> -        server.urls = [server.urls];
> +        this.logWarning(
> +          "Ignoring invalid media.peerconnection.default_iceservers in about:config",
> +           null, 0);

Same-line indent is usually 4 spaces I believe, or after the parenthesis (I for one don't mind going over a little here and there).

::: dom/media/tests/mochitest/test_peerConnection_bug825703.html:80
(Diff revision 3)
> +  var r;
> +  var p = new Promise(resolve => r = resolve);
> +  SpecialPowers.pushPrefEnv({
> +    set : [ ['media.peerconnection.default_iceservers', ""]]
> +  }, function() {
> +    makePC(); 
> +    SpecialPowers.pushPrefEnv({
> +      set : [ ['media.peerconnection.default_iceservers', "kkk"]]
> +    }, function(){
> +      makePC();
> +      SpecialPowers.pushPrefEnv({
> +        set : [ ['media.peerconnection.default_iceservers', "kkk"]]
> +      }, function(){
> +        makePC();
> +        // we resolve the promise so that the test can finish
> +        r();
> +      });
> +    });
> +  });
> +
> +  p.then(networkTestFinished);

I like that we're using promises here. We should even be able to avoid plough-style code. Something like:

    var push = prefs => new Promise(resolve =>
        SpecialPowers.pushPrefEnv(prefs, resolve));
    
    push({ set: [['media.peerconnection.default_iceservers', ""]] })
    .then(() => makePC())
    .then(() => push({ set: [['media.peerconnection.default_iceservers', "k"]] }))
    .then(() => makePC())
    .then(() => push({ set: [['media.peerconnection.default_iceservers', "k"]] }))
    .then(() => makePC())
    .then(networkTestFinished);

should be equivalent to what you have. Maybe replace the last "k" with something like "{ urls:"stun:127.0.0.1" }". I realize the test has no way to observe the difference, but it's good to exercise all the code paths. I trust you've verified that the errors appear and look good?
You should be able to close and discard bz://1167922/rshenthar2 from inside Review board to delete it, so there's no confusion about which patch set to land.
Attachment #8624972 - Flags: review?(jib) → review+
Attachment #8622570 - Attachment is obsolete: true
Attachment #8622571 - Attachment is obsolete: true
Attachment #8622572 - Attachment is obsolete: true
Attachment #8622573 - Attachment is obsolete: true
Attachment #8622574 - Attachment is obsolete: true
(In reply to Jan-Ivar Bruaroey [:jib] from comment #19)
> You should be able to close and discard bz://1167922/rshenthar2 from inside
> Review board to delete it, so there's no confusion about which patch set to
> land.

Done!
Finalized comments with r+
Bug 1167922:Corrected third check
Bug 1167922: Corrected third check to test a valid stun server as well
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/64cb3eddd1a9
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: