742197 - Verify that unannotated string conversions are correct in Paris bindings

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Need to check that we're doing what WebIDL actually calls for; I think right now we do what quickstubs do, which may not be the same thing.

We should think about whether we want to do this before initial ship...

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I believe new bindings match quickstubs, which match old XPConnect, which converts null to a void nsAString. WebIDL wants "null", and I think we should move towards that.

Maybe it would be best to annotate all the Paris bindings we add with the old behaviour for now, though; we're already changing a lot of (I assume) web-compat sensitive stuff.

Comment 2

[Boris Zbarsky [:bzbarsky]]

WebIDL wants "null" for non-nullable strings.  For nullable strings it wants the null passed through (in our case void nsAString).

The cases which were really web compat sensitive in the Paris bindings are all "DOMString?" in the IDL, so they're going to Just Work.

So yes, I think we should move towards what webidl wants, but I don't think we should mass-annotate with the old behavior.  The only question is whether we want to wait until we branch for 14 to flip XHR to the webidl behavior, to mitigate our initial-landing risk a bit.

Comment 3

[Boris Zbarsky [:bzbarsky]]

So to be precise, the per-spec WebIDL behavior mapped onto our code would be:

1)  If the string is nullable, then null is converted to a void string.
2)  If the string is nullable and [TreatUndefinedAs] is "EmptyString" then undefined is
    converted to an empty string.
3)  If the string is nullable and [TreatUndefinedAs] is "Null" then undefined is 
    converted to a void string.
4)  If the string is nullable and [TreatUndefinedAs] is "Missing" then the spec does not
    define behavior, if I read it right.  Perhaps this code cannot be entered for
    Missing?
5)  If the string is not nullable, null is converted to either "" or "null" depending on
    whether [TreatNullAs=EmptyString] is used.
6)  If the string is not nullable, undefined is converted to either "" or "undefined"
    depending on whether [TreatUndefinedAs=EmptyString] is used.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

We talked this over in the meeting today and figure we'll land it after the next aurora branchpoint.  But just to start the ball rolling...

Comment 5

[Cameron McCormack (:heycam)]

The spec does call into the "convert an ECMAScript value to an IDL value of type DOMString" algorithm for an argument that is [TreatUndefinedAs=Missing], in cases such as:

  void f(long x,
         [TreatUndefinedAs=Missing] optional DOMString y,
         [TreatUndefinedAs=Missing] optional DOMString z);

and you call it with:

  f(1, undefined, "a");

The behaviour is defined, since when the undefined is converted to a DOMString it falls through to step 3 of #es-DOMString.  The spec doesn't actually allow for example [TreatUndefinedAs=Missing,EmptyString] to cause the above call to be treated as f(1, "", "a").  If we need this, I'll need to change the spec.  I haven't tested: does XHR need an undefined passed as the username be treated as "" if a non-undefined value is passed as the password?

Comment 6

[Boris Zbarsky [:bzbarsky]]

> The behaviour is defined, since when the undefined is converted to a DOMString it falls
> through to step 3 of #es-DOMString.

Te behavior I was complaining about being undefined involves nullable strings annotated with [TreatUndefinedAs=Missing].  In that sitation, nothing even says we end up in #es-DOMString that I can tell.

> The spec doesn't actually allow for example [TreatUndefinedAs=Missing,EmptyString] to
> cause the above call to be treated as f(1, "", "a")

Hrm.  That seems ... broken to me.  I didn't realize that you could specify both Missing and EmptyString together, for that matter!

> does XHR need an undefined passed as the username be treated as "" if a non-undefined
> value is passed as the password?

No idea, but XHR uses nullable strings, and doesn't use Missing, so for its purposes what Missing does is completely irrelevant.

Comment 7

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky (:bz) from comment #6)
> Te behavior I was complaining about being undefined involves nullable
> strings annotated with [TreatUndefinedAs=Missing].  In that sitation,
> nothing even says we end up in #es-DOMString that I can tell.

Oh right, in that case yes the #es-nullable-type wants to say that null gets returned.  The algorithm there does need tweaking to make sure it continues on with step 2.

> Hrm.  That seems ... broken to me.  I didn't realize that you could specify
> both Missing and EmptyString together, for that matter!

Oh you can't.  But if we need it to we can allow it.

> > does XHR need an undefined passed as the username be treated as "" if a non-undefined
> > value is passed as the password?
> 
> No idea, but XHR uses nullable strings, and doesn't use Missing, so for its
> purposes what Missing does is completely irrelevant.

Huh.  I did introduce Missing for XHR:

  http://lists.w3.org/Archives/Public/public-script-coord/2012JanMar/0011.html

Comment 8

[Boris Zbarsky [:bzbarsky]]

That said, the fact that XHR is not using TreatUndefinedAs=Missing may well be a spec bug in XHR, since it _does_ want that behavior, iirc....

Comment 9

[Boris Zbarsky [:bzbarsky]]

I wonder whether we should file a separate bug on carefully reviewing the XHR IDL here.  It looks like what's in the spec isn't quite right.  :(

> The algorithm there does need tweaking to make sure it continues on with step 2.

Yes, that would solve the problem.

> Oh you can't.  But if we need it to we can allow it.

Let's not add that complexity until someone asks for it... ;)

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

For XHR we absolutely need all of the following to be equivalent:

xhr.send();
xhr.send(null);
xhr.send("");

I'm not sure, but I suspect that the following also should be treated the same way

xhr.send(undefined);


Note that in general I prefer 'undefined' to always be treated the same as if the argument wasn't passed at all for optional arguments. This isn't what WebIDL currently says though, but it's an open issue at TC39.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> For XHR we absolutely need all of the following to be equivalent:

Yes, we have that behavior.  Sort of.  Because the string overload for send() is nullable.

But note that per spec as currently written those first two you list are not equivalent to the third one (though I'm not sure we do that right now).  The case that take undefined _is_ equivalent to the one that takes null per spec and in our current impl.

If you actually want all of those to be equivalent, then the IDL should be:

  void send([TreatNullAs=EmptyString,TreatUndefinedAs=EmptyString]
            optional DOMString data = "");

or so...  But again, that gives a different behavior from what the spec has right now.  Whether the spec is correct is a separate matter.

Comment 12

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/projects/birch/rev/28d4b29abc21

Comment 13

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/28d4b29abc21