796850 - Implement ByteString in the WebIDL bindings

Status: RESOLVED FIXED

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

Description

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

Andrea, do you happen to be interested? :)

Comment 1

[Andrea Marchesini [:baku]]

I am interested... but I cannot work on this right now.

Comment 2

[James Kitchener (:jkitch)]

Attachment: WIP

My work so far.

Support for bytestrings has been added to the webidl parser and code generator.

Compiles, but it needs additional testing before I am confident of correctness.

Comment 3

[James Kitchener (:jkitch)]

Attachment: part 1. WebIDL parser changes to support ByteString

Now includes tests.  Is this a reasonable approach?

isString() is currently kept as syntatic sugar and tests against both isDOMString() and isByteString().  Should this be removed?  It may be a little ambiguous given the type of a DOMString is "String"

Comment 4

[James Kitchener (:jkitch)]

Attachment: part 2: Code Generation changes & tests

I'm less confident about the correctness of this patch.

It compiles, parses the Test*.webidl files and appears to generate code, but is otherwise lacking in tests. (In particular, the "Throw a TypeError if char value > 255" behaviour is in need of testing).

Can you suggest a way to perform runtime tests?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 747968 [details] [diff] [review]
part 1. WebIDL parser changes to support ByteString

Are there still consumers of isString when this is all landed?  I guess it would at least be used in isDistinguishableFrom, and its use there is sensible.

I don't think the IDLValue.coerceToType changes are needed or desired.  Per spec there is no way to specify constant bytestrings or to set default values for bytestring stuff, right?  If the latter _is_ possible, I see no indication that having values over 255 would be disallowed, nonsensical as they would be.  Please raise spec issues as needed.

>+    harness.check(len(members), 2, "Should be one production")

Two productions.

>+    #now check we haven't broken DOMStrings in the process.

Space after '#'.

>+              const ByteString = "hello"

  const ByteString foo = "hello";

no?  Check which exception you're getting, please.

And for the arg with default value test, I guess we should see what we decide the spec says?

Thanks for doing this!

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 747979 [details] [diff] [review]
part 2: Code Generation changes & tests

> Can you suggest a way to perform runtime tests?

We don't have a way to do that with WebIDL right now short of implementing an actual interface using the functionality and then writing mochitests for it....

So we'd just need to use this thing in XHR as specced and add some tests.

I'm not sure we need the mozilla::dom::ByteString class; It's really a performance optimization, and I would assume nothing really performance-sensitive uses ByteString.

I would also get rid of the Optional specialization.  Just use Optional<nsCString> to represent optional ByteString values.  We can't do that for DOMString because dependent strings are not nsString, but for ByteString we can't use dependent strings anyway.

Similar for FakeDependentCString, which is even worse because we have nothing to depend on!  We can just use an nsCString, I would think.

The version of ConvertJSValueToString that does ByteString stuff can be way simpler: there are no different stringification behaviors involved except to the extent that a nullable type is used, but then both undefined and null become null.

>+  result.SetData(ToNewCString(nsAutoString(chars)), len);

That's a leak right there.

What I suggest doing instead for ByteString is something more like what XPCConvert::JSData2Native does for the ACString type.  See http://hg.mozilla.org/mozilla-central/file/08be63954b6b/js/xpconnect/src/XPCConvert.cpp#l691 except your nsCString will already be on the stack instead of needing to heap-allocate it.

>+    # XXX Suspect this needs work

It can be simplified a good bit: Just always use nsCString as the decltype.  And if we use Optional<nsCString> for optionals we don't need special-case Optional handling here either: just return True for the fourth element of the return-value tuple, and the Optional bits will be done automatically.  And as I said above, we don't have different stringification behaviors here, modulo nullable, and the behavior for null and undefined is always the same.

>+        elif arg.type.isByteString():
>+            # XPConnect string-to-JS conversion wants to mutate the string.

It won't; see below.

>+MSG_DEF(MSG_INVALID_BYTESTRING, 0, "Cannot convert to ByteString with character exceeding 255.")

Maybe "Cannot convert string to ByteString because the character at index {0} has value {1} which is greater than 255" and pass in the values?

ByteStringToJsval should just call NonVoidByteStringToJsval when not null.  And both can always take a const string.

The implementation of NonVoidByteStringToJsval should probably just do something similar to what http://hg.mozilla.org/mozilla-central/file/08be63954b6b/js/xpconnect/src/XPCConvert.cpp#l285 does and be out of line.  There's no point calling into NonVoidStringToJsval with all its complexity.

I don't think we need the changes to nsrootidl or xpidl.py at all here.  Just take those out.

Sorry ByteString isn't more like DOMString... but it's really not.  :(

Comment 7

[James Kitchener (:jkitch)]

Attachment: part 1. WebIDL parser changes to support ByteString v2

Comment 8

[James Kitchener (:jkitch)]

Attachment: part 2: Code Generation changes & tests v2

Suggested changes have been made, with the exception of removing the Optional<nsACString> specialisation.  I was unable to get the code to compile without it (type casting issues).

Comment 9

[James Kitchener (:jkitch)]

Attachment: part 3: Convert XMLHttpRequest to use ByteString

This includes a behaviour change.  Previously the changed strings were interpreted as UTF8 encoded, rather than the raw 8-bit format of ByteString.  I've done a limited (linux64) try run, and fixed the test where this was assumed.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 757094 [details] [diff] [review]
part 1. WebIDL parser changes to support ByteString v2

>+            if self.type.isByteString():
>+                raise WebIDLError("ByteStrings cannot have a default value",
>+                                  [self.location])

Why is this needed?  I would have expected coerceToType() to fail on this situation anyway....

> dom/imptests/WebIDLParser.js

I don't think we want to change these files, since they're imported from another repository; they should be fixed there instead.  Sorry I missed that the first time.  Ms2ger, can you confirm?

r=me with those issues dealt with.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 757095 [details] [diff] [review]
part 2: Code Generation changes & tests v2

Was there a reason that using nsCString for the ByteString type did not work such that you ended up having to use nsACString?  Again, if we just used nsCString we wouldn't need the extra Optional specialization and we could leverage more of the "normal" (as opposed to DOMString) Optional handling?

There's also no need to put NonVoidByteStringToJsval and ByteStringToJsval under xpc stuff.  Just put them in BindingUtils.h/cpp.

Comment 12

[Boris Zbarsky [:bzbarsky]]

I'd like to get part 2 sorted through before worrying about part 3, though I suspect that even if the bindings use nsCString the callee can still use nsACString for both in and out params....

Comment 13

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

(In reply to Boris Zbarsky (:bz) from comment #10)
> Comment on attachment 757094 [details] [diff] [review]
> part 1. WebIDL parser changes to support ByteString v2
> 
> > dom/imptests/WebIDLParser.js
> 
> I don't think we want to change these files, since they're imported from
> another repository; they should be fixed there instead.  Sorry I missed that
> the first time.  Ms2ger, can you confirm?

Indeed. Upstream is <https://github.com/w3c/testharness.js/>; would be great if you could create a pull request there.

Comment 14

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

Comment on attachment 757094 [details] [diff] [review]
part 1. WebIDL parser changes to support ByteString v2

r- for the dom/imptests parts.

Comment 15

[James Kitchener (:jkitch)]

Attachment: error when using nsCString instead of nsACString

(In reply to Boris Zbarsky (:bz) from comment #11)
> Comment on attachment 757095 [details] [diff] [review]
> part 2: Code Generation changes & tests v2
> 
> Was there a reason that using nsCString for the ByteString type did not work
> such that you ended up having to use nsACString?  Again, if we just used
> nsCString we wouldn't need the extra Optional specialization and we could
> leverage more of the "normal" (as opposed to DOMString) Optional handling?

I wasn't able to get it to compile using nsCString and default Optional.  

The error message, and the generated code that produces the error message is attached.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> The error message, and the generated code that produces the error message is attached.

Thanks.  The callee (in TestBindingHeader.h) should be declared as taking an Optional<nsCString> but isn't, is what that error is saying.

Comment 17

[Boris Zbarsky [:bzbarsky]]

James, does comment 16 make sense?

Comment 18

[James Kitchener (:jkitch)]

Attachment: part 1. WebIDL parser changes to support ByteString v3

Corrections have been made

Comment 19

[James Kitchener (:jkitch)]

Attachment: part 2: Code Generation changes & tests v3

The suggested change fixed the problems.

Default Optional and nsCString version is attached.

Comment 20

[James Kitchener (:jkitch)]

Attachment: part 3: Convert XMLHttpRequest to use ByteString v2

The changes to part 2 didn't appear to require any changes to part 3.

The updated version simply uses a new GUID for nsIXMLHttpRequest.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Note: you'll need to merge part 1 to the changes from bug 829248.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 759661 [details] [diff] [review]
part 2: Code Generation changes & tests v3

>+++ b/dom/bindings/BindingUtils.cpp
>+NonVoidByteStringToJsval(JSContext *cx, const nsACString &str, JS::Value *rval)

Might be better to use "JS::MutableHandle<JS::Value> rval" for the last argument, now that bug 877281  is fixed (though you'll need to update to pull in that fix).  More on this below.

>+        *rval = JS_GetEmptyStringValue(cx);

And this would be rval.set(JS_GetEmptyStringValue(cx);

>+    *rval = STRING_TO_JSVAL(jsStr);

And rval.set(JS::StringValue(jsStr));

>+++ b/dom/bindings/BindingUtils.h
>+ConvertJSValueToString(JSContext* cx, const JS::Value& v, JS::Value* pval,

 JS::Handle<JS::Value> v, JS::MutableHandle<JS::Value> pval

Also, I don't expect this to be super-performance-sensitive, so might be better to not inline it.

Also, maybe this should be ConvertJSValueToByteString?

>+    if (nullable && (v.isNull() || v.isUndefined())) {

  if (nullable && v.isNullOrUndefined()) {

>+    pval->setString(s);  // Root the new string.

  pval.set(JS::StringValue(s));  // Root the new string

>+  for (unsigned int i = 0; i < length; i++) {

Probably better to make 'i' be of type size_t.

>+      sprintf(index, "%d",i);

Space before ",i", please.  Also, worth commenting that even with a 64-bit size_t 20 chars is enough?  And definitely using snprintf to make sure that if not enough for some odd reason bad things don't happen.

>+      sprintf(badChar,"%d", chars[i]);

Similar here: space after "badChar," and please us snprintf.

>+  result.SetLength(uint32_t(length));
>+  if (result.Length() != uint32_t(length)) {
>+    return false;
>+  }

Hmm.  We need to make sure we have space for the null-terminator too...

So what should probably happen here is making sure length is < UINT32_MAX, then SetCapacity(length + 1), writing into the string, putting a null char at result.BeginWriting()[length], and then SetLength(length).

>+  JS_EncodeStringToBuffer(cx, s, result.BeginWriting(), length);

uint32_t(length) for the last arg, since that's how much length you know you have to work with.

>+ * Note, the ownership of the string buffer may be moved from str to rval.
>+ * If that happens, str will point to an empty string after this call.

Please remove those two lines; they aren't true for ByteString.

>+inline bool ByteStringToJsval(JSContext *cx, const nsACString &str, JS::Value *rval)

Again, MutableHandle.

>+++ b/dom/bindings/Codegen.py
>+    if type.isByteString():
>+        if type.nullable():
>+            nullable = "true"
>+        else:
>+            nullable = "false"

          nullable = toStringBool(type.nullable())

>+                "if (!ConvertJSValueToString(cx, ${val}, ${valPtr}, %s, %s)) {\n"

Please replace ${valPtr} with ${mutableVal}.

>+                "}" % (nullable,varName, exceptionCodeIndented.define()))

Space after "nullable,"

>+            # We have to make a copy, because our jsval may well not
>+            # live as long as our string needs to.

Not relevant, since we already made a copy.  You shouldn't need any differences here between the isMember case and not, and shouldn't need the getConversionCode bits.  You should be able to just do:

        conversionCode = (
            "if (!ConvertJSValueToString(cx, ${val}, ${mutableVal}, %s, ${declName})) {\n"
            "%s\n"
            "}" % (nullable, varName, exceptionCodeIndented.define()))

        return JSToNativeConversionInfo(
            conversionCode,
            declType=CGGeneric("nsCString"),
            dealWithOptional=isOptional)


>+            return (wrapAndSetPtr("ByteStringToJsval(cx, %s, ${jsvalPtr})" % result), False)

That should now be ${jsvalHandle} instead of ${jsvalPtr} if ByteStringToJsval takes a MutableHandle.  (Yes, I know the DOMString case hasn't gotten fixed yet...)

>+            return (wrapAndSetPtr("NonVoidByteStringToJsval(cx, %s, ${jsvalPtr})" % result), False)

And here.

>+++ b/dom/bindings/test/TestBindingHeader.h
>   static
>   already_AddRefed<TestInterface>
>+    Constructor(const GlobalObject&, const nsACString&, ErrorResult&);

This is unused.  Please drop it.

Thank you for adding tests!

r=me with the above fixed, though I'd appreciate an interdiff showing the changes.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 759663 [details] [diff] [review]
part 3: Convert XMLHttpRequest to use ByteString v2

>+++ b/content/base/src/nsXMLHttpRequest.h
>+    aRv = SetRequestHeader(aHeader,aValue);

Space after ',', please.

>+++ b/dom/bindings/test/TestBindingHeader.h
>+++ b/dom/bindings/test/TestCodeGen.webidl
>+++ b/dom/bindings/test/TestExampleGen.webidl
>+++ b/dom/bindings/test/TestJSImplGen.webidl

These changes should be in part 2, right?  Also, in TestJSImplGen, please put receiveByteStringSequence() after the commented-out passStringSequence()?

r=me with that.  This is awesome.  :)

Comment 24

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

(In reply to Boris Zbarsky (:bz) from comment #22)
> >+    *rval = STRING_TO_JSVAL(jsStr);
> 
> And rval.set(JS::StringValue(jsStr));

rval.setString(jsStr); please.

Comment 25

[James Kitchener (:jkitch)]

Attachment: interdiff of code generation changes

Suggested changes have been made.

jsvalHandle wasn't defined, so you might want to check that my implementation is what was intended.

Comment 26

[James Kitchener (:jkitch)]

Attachment: part 1. WebIDL parser changes to support ByteString v4

rebased

Comment 27

[James Kitchener (:jkitch)]

Attachment: part 2: Code Generation changes & tests v4

Interdiff changes folded in.

Comment 28

[James Kitchener (:jkitch)]

Attachment: part 3: Convert XMLHttpRequest to use ByteString v3

Suggested changes have been made.

Comment 29

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

Comment on attachment 760194 [details] [diff] [review]
interdiff of code generation changes

Review of attachment 760194 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +3901,5 @@
>                  'result' :  "%s[%s]" % (result, index),
>                  'successCode': "break;",
>                  'jsvalRef': "tmp",
>                  'jsvalPtr': "tmp.address()",
> +                'jsvalHandle': "JS::MutableHandle<JS::Value>::fromMarkedLocation(tmp.address())",

You should just be "&tmp"

@@ +7250,5 @@
>    }
>  }"""
>  
> +        templateValues = {'jsvalRef': '*vp.address()', 'jsvalPtr': 'vp.address()',
> +                          'jsvalHandle' : 'JS::MutableHandle<JS::Value>::fromMarkedLocation(vp.address())',

&vp

@@ +7337,5 @@
>      def getBody(self):
>          successCode = ("*present = found;\n"
>                         "return true;")
>          templateValues = {'jsvalRef': '*vp.address()', 'jsvalPtr': 'vp.address()',
> +                          'jsvalHandle': 'JS::MutableHandle<JS::Value>::fromMarkedLocation(vp.address())',

&vp

@@ +7961,5 @@
>                                   "}\n"
>                                   "break;" % propDef),
>                  'jsvalRef': "temp",
>                  'jsvalPtr': "temp.address()",
> +                'jsvalHandle': "JS::MutableHandle<JS::Value>::fromMarkedLocation(temp.address())",

&temp

@@ +9576,5 @@
>                  'result' : result,
>                  'successCode' : "continue;" if arg.variadic else "break;",
>                  'jsvalRef' : "argv[%s]" % jsvalIndex,
>                  'jsvalPtr' : "&argv[%s]" % jsvalIndex,
> +                'jsvalHandle' : "JS::MutableHandle<JS::Value>::fromMarkedLocation(&argv[%s])" % jsvalIndex,

argv.handleAt(%s) should work, I think

Comment 30

[James Kitchener (:jkitch)]

Attachment: part 2: Code Generation changes & tests v5

jsvalHandle definitions have been corrected

Comment 31

[Boris Zbarsky [:bzbarsky]]

> jsvalHandle wasn't defined

Which revision are you basing this on?  It should be defined if your tree has the fix for bug 877281 (note that you need to pull inbound for that; it hasn't gotten merged to m-c yet).

Comment 32

[James Kitchener (:jkitch)]

My code is based off Mozilla-Central.  I didn't realise it hadn't landed yet.  A revised patch will be posted once it does.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Yeah , sorry; I expected it to have merged by now.  :(

Comment 34

[James Kitchener (:jkitch)]

Attachment: rebased interdiff of code generation changes

Now uses the changes from bug 877281

Comment 35

[James Kitchener (:jkitch)]

Attachment: rebased interdiff of code generation changes

Sorry, the last one was missing some changes.

Comment 36

[James Kitchener (:jkitch)]

Attachment: part 2: Code Generation changes & tests v6

Interdiff folded into a slightly modified version of attachment 759663 [details] [diff] [review]  (changed only enough for the patch to apply cleanly.  Changes to make it compile were in the interdiff).

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 760349 [details] [diff] [review]
rebased interdiff of code generation changes

>+++ b/dom/bindings/BindingUtils.cpp
>+// pval must not be null and must point to a rooted JS::Value

This comment should go; MutableHandle<Value> tells us all we need to know about pval.

>+ConvertJSValueToByteString(JSContext* cx, JS::Handle<JS::Value> v,
>+                       JS::MutableHandle<JS::Value> pval, bool nullable,
>+                       nsACString& result)

Please fix the indent.

>+      _snprintf(index, 21, "%d", i);

I forgot that Windows does weird naming here, right.  What you want is:

  PR_snprintf(index, sizeof(index), "%d", i);

and perhaps a static assert that sizeof(size_t) <= 8.

>+      // A UTF-16 character is up to 32 bits long.

Actually, a jschar is 16 bits in size, for what it's worth.

>+  JS_EncodeStringToBuffer(cx, s, result.BeginWriting(), uint32_t(length));
>+  result.SetLength(uint32_t(length));

Shouldn't need the casts here, since we made sure it's in the right range, right?

>+++ b/dom/bindings/BindingUtils.h
>+ConvertJSValueToByteString(JSContext* cx, JS::Handle<JS::Value> v,
>+                       JS::MutableHandle<JS::Value> pval, bool nullable,
>+                       nsACString& result);

Please fix the indent here too.

>+        rval.set(JSVAL_NULL);

I _think_ rval.setNull() should work here.

>+++ b/dom/bindings/Codegen.py
>+        # ByteString arguments cannot have a default value.

  assert defaultValue is None

r=me with those fixed; this looks fantastic!

Comment 38

[James Kitchener (:jkitch)]

Attachment: part 2: Code Generation changes & tests v7

Comment 39

[Boris Zbarsky [:bzbarsky]]

James, it looks like this is ready to go, right?  Do you want me to push the last patches here to try?

Comment 40

[James Kitchener (:jkitch)]

Yes, it should be ready.  

I can do the push myself, but I won't have access to the appropriate computer for another 7 hours.  Otherwise how exhaustive should the try run be?

Comment 41

[Boris Zbarsky [:bzbarsky]]

Another 7 hours is not a problem, so I'll leave that to you.  ;)

Doing a debug run on one platform is probably enough, I would think...

Comment 42

[James Kitchener (:jkitch)]

https://tbpl.mozilla.org/?tree=Try&rev=7fb933609efe

Green, other than a crash that appears to be a known problem.

Comment 43

[Boris Zbarsky [:bzbarsky]]

Yep, looks good.  Can you push to inbound yourself, or do you want me to?

Comment 44

[James Kitchener (:jkitch)]

Could you please do it?

I don't have that level of commit access.

Comment 45

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/864acd590539
   https://hg.mozilla.org/integration/mozilla-inbound/rev/2cb618e6b57c
   https://hg.mozilla.org/integration/mozilla-inbound/rev/708f6cd81acb

Thank you again for the patches!

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/864acd590539
https://hg.mozilla.org/mozilla-central/rev/2cb618e6b57c
https://hg.mozilla.org/mozilla-central/rev/708f6cd81acb

Comment 47

[Jeremie Patonnier :Jeremie]

Follow up for documentation: https://bugzilla.mozilla.org/show_bug.cgi?id=885275