869272 - Null not preserved when passed as dictionary argument to webidl JS constructor.

Status: RESOLVED INVALID

(Core :: DOM: Core & HTML, defect)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Webidl:

  Constructor (optional mozRTCConfiguration configuration = null,
               optional mozRTCConstraints constraints = null)]
  interface mozRTCPeerConnection : EventTarget  {...}

Call:

  var rtcConfig;
  var pc1 = new mozRTCPeerConnection(rtcConfig || null);

Binding:

  static JSBool
  _constructor(JSContext* cx, unsigned argc, JS::Value* vp)
  {
    JS::Rooted<JSObject*> obj(cx, JSVAL_TO_OBJECT(JS_CALLEE(cx, vp)));
    JS::Value* argv = JS_ARGV(cx, vp);

    GlobalObject global(cx, obj);
    if (global.Failed()) {
      return false;
    }

    mozRTCConfiguration arg0;
.-- if (!arg0.Init(cx, (0 < argc) ?
|     JS::Handle<JS::Value>::fromMarkedLocation(&argv[0]) : JS::NullHandleValue)) {
|     return false;
|   }
|
'>bool
  mozRTCConfiguration::Init(JSContext* cx, JS::Handle<JS::Value> val)
  {
    // Passing a null JSContext is OK only if we're initing from null,
    // Since in that case we will not have to do any property gets
    MOZ_ASSERT_IF(!cx, val.isNull());
    if (cx && !initedIds && !InitIds(cx)) {
      return false;
    }
    JSBool found;
    JS::Rooted<JS::Value> temp(cx);
    bool isNull = val.isNullOrUndefined();
    if (!IsConvertibleToDictionary(cx, val)) {
      return ThrowErrorMessage(cx, MSG_NOT_DICTIONARY);
    }

    if (isNull) {
      found = false;
    } else if (!JS_HasPropertyById(cx, &val.toObject(), iceServers_id, &found)) {
      return false;
    }
    if (found) {
      // <-- never gets here fyi.
    }
    return true;
  }

Implementation in PeerConnection.js:
  __init: function(rtcConfig) {
    if (!rtcConfig) {
       // No config provided. Use default from about:config. <-- Never gets here. Problem!
      rtcConfig = {iceServers:
        JSON.parse(Services.prefs.getCharPref("media.peerconnection.default_iceservers"))};
    }

Marking as blocking bug 823512 for now, though we may be able to work around it.

The WebRTC spec is vague about the RTCConfiguration constructor argument, and Firefox is alone in implementing default iceServers, so we have to be careful about subtle behavior here.

Right now:
 A. pc = new mozPeerConnection();              // uses default server.
 B. pc = new mozPeerConnection({})             // throws.
 C. pc = new mozPeerConnection({iceServer:[]}) // overrides the built-in server (which a
                                               // user observed worked faster for his case)

Hence the need to detect null dictionary arg. The reason we throw in B is that I thought it was unclear whether the user desired behavior A or C in this case.

Workaround:
- If we decide that B should work like A (either permanently or temporarily or for FF23),
  then we can work around this by allowing an empty dictionary object here and the null
  case will work the same, with no need to solve the issue reported here.

Comment 1

[Boris Zbarsky [:bzbarsky]]

>  Constructor (optional mozRTCConfiguration configuration = null,

That's a dictionary, right?

Per spec, dictionaries initialized with null have the same behavior as dictionaries initialized with an empty object or dictionaries that are not passed at all: all end up as an empty dictionary.  There is no provision in WebIDL for treating "not passed" differently from "empty dictionary passed": that's just not how dictionaries behave.

Basically, web authors expect case B to work like case A if this is really a dictionary...

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Right. Okay, thanks for clarifying.

I think I can safely implement the workaround as described then.

PS: As a side-note, I was a little surprised to see that the dictionaries were not primitives anymore, though seeing the bindings it makes sense. Is it also desired?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Not primitives in that you always get an object?

If so, that's desired, yes: we just canonicalize all the equivalent stuff so you always get an object which may or may not have some properties set.

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

> Not primitives in that you always get an object?

No, I meant in that the resulting object looked more opaque, but it turns out I made a bad observation as the resulting object just looked more opaque to me (using dump(JSON.stringify(rtcConfig)), because it filters out unknown keys, which since I was passing in bad input to provoke my error message where I dump, had my object come out as {}. My bad.

After some more experimentation though, I see we're doing some normalizing, which still surprises me, but I guess is benign:

Passed in: {"iceServers":[{"url":"stun:stun.l.google.com:19302"}]}

Processed:
 {"iceServers":
  [{"credential":null,"url":"stun:stun.l.google.com:19302","username":null}]}

So it is clearly not the original primitive. That's probably fine. :-)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Right, the normalizing "null" thing there there happens because the IDL has default null values for those dictionary members, so if they're not provided we supply those default values in the binding layer.  If you don't want that behavior, you don't provide default values.  ;)

So to be clear, one of the goals here is to never pass content JS objects directly to chrome.  For dictionaries, we create a new JS object in the chrome scope and copy the relevant parts to it.

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Makes sense, and thanks, didn't realize that was because of the default values.

This is great, I can remove some of our manual validation code now.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Awesome.  That's the general idea.  ;)