Closed Bug 796850 Opened 7 years ago Closed 7 years ago

Implement ByteString in the WebIDL bindings

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: Ms2ger, Assigned: jkitch)

References

(Blocks 1 open bug, )

Details

(Keywords: doc-bug-filed, Whiteboard: [mentor=bz][lang=python][lang=c++])

Attachments

(5 files, 14 obsolete files)

1.68 KB, text/plain
Details
16.31 KB, patch
Details | Diff | Splinter Review
25.83 KB, patch
Details | Diff | Splinter Review
14.41 KB, patch
bzbarsky
: review+
bzbarsky
: feedback+
Details | Diff | Splinter Review
28.72 KB, patch
Details | Diff | Splinter Review
Andrea, do you happen to be interested? :)
Assignee: nobody → amarchesini
I am interested... but I cannot work on this right now.
Assignee: amarchesini → nobody
Whiteboard: [mentor=bz][lang=python][lang=c++]
Assignee: nobody → jkitch.bug
Attached patch WIP (obsolete) — Splinter Review
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.
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"
Attachment #745629 - Attachment is obsolete: true
Attachment #747968 - Flags: feedback?(bzbarsky)
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?
Attachment #747979 - Flags: feedback?(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!
Attachment #747968 - Flags: feedback?(bzbarsky) → feedback+
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.  :(
Attachment #747979 - Flags: feedback?(bzbarsky) → feedback+
Keywords: dev-doc-needed
Attachment #747968 - Attachment is obsolete: true
Attachment #757094 - Flags: review?(bzbarsky)
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).
Attachment #747979 - Attachment is obsolete: true
Attachment #757095 - Flags: review?(bzbarsky)
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.
Attachment #757096 - Flags: review?(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.
Attachment #757094 - Flags: review?(bzbarsky)
Attachment #757094 - Flags: review?(Ms2ger)
Attachment #757094 - Flags: review+
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.
Attachment #757095 - Flags: review?(bzbarsky) → review-
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....
(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 on attachment 757094 [details] [diff] [review]
part 1. WebIDL parser changes to support ByteString v2

r- for the dom/imptests parts.
Attachment #757094 - Flags: review?(Ms2ger) → review-
(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.
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
James, does comment 16 make sense?
Flags: needinfo?(jkitch.bug)
Corrections have been made
Attachment #757094 - Attachment is obsolete: true
The suggested change fixed the problems.

Default Optional and nsCString version is attached.
Attachment #757095 - Attachment is obsolete: true
Attachment #759661 - Flags: review?(bzbarsky)
Flags: needinfo?(jkitch.bug)
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.
Attachment #757096 - Attachment is obsolete: true
Attachment #757096 - Flags: review?(bzbarsky)
Attachment #759663 - Flags: review?(bzbarsky)
Note: you'll need to merge part 1 to the changes from bug 829248.
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.
Attachment #759661 - Flags: review?(bzbarsky) → review+
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.  :)
Attachment #759663 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #22)
> >+    *rval = STRING_TO_JSVAL(jsStr);
> 
> And rval.set(JS::StringValue(jsStr));

rval.setString(jsStr); please.
Suggested changes have been made.

jsvalHandle wasn't defined, so you might want to check that my implementation is what was intended.
Attachment #760194 - Flags: feedback?(bzbarsky)
rebased
Attachment #759660 - Attachment is obsolete: true
Interdiff changes folded in.
Attachment #759661 - Attachment is obsolete: true
Suggested changes have been made.
Attachment #759663 - Attachment is obsolete: true
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
jsvalHandle definitions have been corrected
Attachment #760197 - Attachment is obsolete: true
> 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).
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.
Attachment #760194 - Flags: feedback?(bzbarsky)
Yeah , sorry; I expected it to have merged by now.  :(
Depends on: 877281
Now uses the changes from bug 877281
Attachment #760194 - Attachment is obsolete: true
Attachment #760348 - Flags: feedback?(bzbarsky)
Sorry, the last one was missing some changes.
Attachment #760348 - Attachment is obsolete: true
Attachment #760348 - Flags: feedback?(bzbarsky)
Attachment #760349 - Flags: feedback?(bzbarsky)
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).
Attachment #760207 - Attachment is obsolete: true
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!
Attachment #760349 - Flags: review+
Attachment #760349 - Flags: feedback?(bzbarsky)
Attachment #760349 - Flags: feedback+
Attachment #760350 - Attachment is obsolete: true
James, it looks like this is ready to go, right?  Do you want me to push the last patches here to try?
Flags: needinfo?(jkitch.bug)
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?
Flags: needinfo?(jkitch.bug)
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...
https://tbpl.mozilla.org/?tree=Try&rev=7fb933609efe

Green, other than a crash that appears to be a known problem.
Yep, looks good.  Can you push to inbound yourself, or do you want me to?
Flags: needinfo?(jkitch.bug)
Could you please do it?

I don't have that level of commit access.
Flags: needinfo?(jkitch.bug)
Depends on: 885275
Depends on: 901682
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.