The default bug view has changed. See this FAQ.

Paris bindings codegen for sequence types

RESOLVED FIXED in mozilla15

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

unspecified
mozilla15
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 8 obsolete attachments)

29.99 KB, patch
peterv
: review+
Details | Diff | Splinter Review
28.82 KB, patch
Details | Diff | Splinter Review
17.00 KB, patch
peterv
: review+
Details | Diff | Splinter Review
9.09 KB, patch
peterv
: review+
Details | Diff | Splinter Review
It is time.
Depends on: 748637
Created attachment 618550 [details] [diff] [review]
part 1.  Refactor js-to-native conversion to make it not assume it's only happening on stack arguments.   changes in CastableObjectUnwrapper and CallbackObjectUnwrapper are
Attachment #618550 - Flags: review?(peterv)
Created attachment 618552 [details] [diff] [review]
Part 1 diff -b, for maybe more ease of reviewing
Created attachment 618556 [details] [diff] [review]
part 2.  Implement codegen for dealing with sequence arguments.
Attachment #618556 - Flags: review?(peterv)
Created attachment 618560 [details] [diff] [review]
part 3.  Make native-to-JS conversion not assume that returning after a successful conversion is OK.  That's true for return values, but not for general conversions.
Attachment #618560 - Flags: review?(peterv)
Created attachment 618562 [details] [diff] [review]
part 4.  Codegen for sequence return values.   big block in getRetvalDeclarationForType is just direct cut/paste from CGCallGenerator plus the addition of the sequence case.  The IDL parser changes were OKed by khuey; they're needed so that we don't have
Attachment #618562 - Flags: review?(peterv)
Created attachment 618812 [details] [diff] [review]
Support running code after successful wrapping

So I was working on a patch to support calling code when wrapping succeeded and this is what I came up with. It looks like it might be able to replace part 3? It allows passing code to run on successful wrapping (like "return true;") in the template values dict under the "success" key. The patch also supports declaring a jsval variable when needed (if filling in a local variable instead of vp).
Yes, that diff does everything I need in part 3.  I just needed to turn various "return true" into "continue" on demand.  ;)
Though I don't understand why the declarejsval bit, but I expect seeing a consumer would help.
One other note.  The code that part 2 leads to in the overload resolution case is kinda crappy:

      if (argv_start[1].isObject() && IsArrayLike(cx, &argv_start[1].toObject())) {
        JS::Value* argv = JS_ARGV(cx, vp);
        nsTArray< float > arg1;
        if (argv[1].isObject()) {
          JSObject* seq = &argv[1].toObject();
          if (!IsArrayLike(cx, seq)) {
            return Throw<false>(cx, NS_ERROR_XPC_BAD_CONVERT_JS);
          }

I have some thoughts on getting rid of those inner isObject and IsArrayLike tests, but they require rejiggering the overload resolution algorithm code a tad.  I'd rather do that as a followup if that's ok.
Created attachment 619221 [details] [diff] [review]
Part 2, now using JS_GetArrayLength
Attachment #619221 - Flags: review?(peterv)
Attachment #618556 - Attachment is obsolete: true
Attachment #618556 - Flags: review?(peterv)
Created attachment 619222 [details] [diff] [review]
Part 2, now using JS_GetArrayLength, and compiling
Attachment #619222 - Flags: review?(peterv)
Attachment #619221 - Attachment is obsolete: true
Attachment #619221 - Flags: review?(peterv)
Created attachment 619224 [details] [diff] [review]
Part 2, now using JS_GetArrayLength, and compiling for real
Attachment #619224 - Flags: review?(peterv)
Attachment #619222 - Attachment is obsolete: true
Attachment #619222 - Flags: review?(peterv)
Blocks: 749866
Blocks: 749864
Whiteboard: [need review]
Depends on: 752042
Created attachment 621325 [details] [diff] [review]
Part 4 now renumbered as part 3 and merged on top of the patch in
Attachment #621325 - Flags: review?(peterv)
Attachment #618562 - Attachment description: part 4. Codegen for sequence return values. big block in getRetvalDeclarationForType is just direct cut/paste from CGCallGenerator plus the addition of the sequence case. The IDL parser changes were OKed by khuey; they're needed so that we don't have → part 4. Codegen for sequence return values. big block in getRetvalDeclarationForType is just direct cut/paste from CGCallGenerator plus the addition of the sequence case. The IDL parser changes were OKed by khuey; they're needed so that we don't have
Attachment #618562 - Attachment is obsolete: true
Attachment #618562 - Flags: review?(peterv)
Comment on attachment 618560 [details] [diff] [review]
part 3.  Make native-to-JS conversion not assume that returning after a successful conversion is OK.  That's true for return values, but not for general conversions.

Not needed anymore; bug 752042 handles this.
Attachment #618560 - Attachment is obsolete: true
Attachment #618560 - Flags: review?(peterv)
Created attachment 621330 [details] [diff] [review]
Part 2 with IsArrayLike fixed to use compartments correctly
Attachment #619224 - Attachment is obsolete: true
Attachment #619224 - Flags: review?(peterv)
Attachment #621330 - Flags: review?(peterv)
Created attachment 621444 [details] [diff] [review]
Part 3 merged on top of latest diff in bug 752042
Attachment #621444 - Flags: review?(peterv)
Attachment #621325 - Attachment is obsolete: true
Attachment #621325 - Flags: review?(peterv)
Attachment #621444 - Attachment description: Merged on top of latest diff in → Part 3 merged on top of latest diff in bug 752042
Attachment #621330 - Attachment description: With IsArrayLike fixed to use compartments correctly → Part 2 with IsArrayLike fixed to use compartments correctly
Comment on attachment 618812 [details] [diff] [review]
Support running code after successful wrapping

This has been spun out to bug 752042.
Attachment #618812 - Attachment is obsolete: true
Blocks: 742185
Comment on attachment 618550 [details] [diff] [review]
part 1.  Refactor js-to-native conversion to make it not assume it's only happening on stack arguments.   changes in CastableObjectUnwrapper and CallbackObjectUnwrapper are

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

::: dom/bindings/Codegen.py
@@ +1122,5 @@
>  
>      def __str__(self):
>          if self.descriptor.workers:
> +            return string.Template(
> +                """${target} = ${source};"""

Don't need triple quotes here. (You could even reindent?)

@@ +1141,5 @@
> +if (NS_FAILED(rv) || !wrappedJS) {
> +  ${codeOnFailure}
> +}
> +
> +// Use a temp nsCOMPtr for the null-check, because ${target} might be NonNull

Should that be 'might be OwningNonNull'? Because if it might be NonNull this could leak?

@@ +1155,5 @@
> +    Get a template for converting a JS value to a native object based
> +    on the given type and descriptor.  If failureCode is given, then we're
> +    actually testing whether we can convert the argument to the
> +    desired type.  That means certain failures to convert need to be
> +    nonfatal and instead use failureCode.

I find this a bit vague and, looking at the code, confusing. Why doesn't callback, arraybuffer, string, enum or primitive unwrapping us failureCode? Because you don't need it? Might be good to document that here.

@@ +1248,5 @@
>          else:
> +            # Either external, or new-binding non-castable.  We always have a
> +            # holder for these, because we don't actually know whether we have
> +            # to addref when unwrapping or not.  So we just pass an
> +            # xpc_qsSelfRef to XPConnect and if we'll need a release it'll put

We're not passing a xpc_qsSelfRef but a nsCOMPtr.

@@ +1287,5 @@
> +            if type.nullable():
> +                templateBody += (
> +                    "\n"
> +                    "} else if (${val}.isNullOrUndefined()) {\n"
> +                    "  ${declName} = NULL;\n")

I don't think you need that final \n. Though it might be cleaner to have all of these not have a starting \n and have a trailing \n.

@@ +1292,5 @@
> +            templateBody += (
> +                "\n"
> +                "} else {\n"
> +                "  return Throw<%s>(cx, NS_ERROR_XPC_BAD_CONVERT_JS);\n"
> +                "}\n" % toStringBool(not descriptor.workers))

But most other stuff doesn't have a trailing \n, so do we really need one here?

@@ +1485,5 @@
>      def define(self):
> +        return instantiateJSToNativeConversionTemplate(
> +            getJSToNativeConversionTemplate(self.argument.type,
> +                                            self.descriptor),
> +            self.replacementVariables).define()

Given that you removed the last consumer of unindenter please remove it too.

::: dom/bindings/Utils.h
@@ +526,5 @@
> +  {}
> +
> +  operator T&() {
> +    MOZ_ASSERT(inited);
> +    MOZ_ASSERT(ptr);

Maybe add the message "NonNull<...> was set to null" or something like that for the second assert? I think that's what it's checking?
Attachment #618550 - Flags: review?(peterv) → review+
Comment on attachment 621330 [details] [diff] [review]
Part 2 with IsArrayLike fixed to use compartments correctly

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

::: dom/bindings/BindingUtils.h
@@ +155,5 @@
>  
>    // XXXbz need to detect platform objects (including listbinding
>    // ones) with indexGetters here!
> +  return JS_IsArrayObject(cx, obj) || JS_IsTypedArrayObject(obj, cx) ||
> +    JS_IsArrayBufferObject(obj, cx);

Nit: I'd line up JS_IsArrayBufferObject with JS_IsArrayObject.

::: dom/bindings/Codegen.py
@@ +1231,5 @@
> +            templateBody += (
> +                "\n"
> +                "} else {\n"
> +                "  return Throw<%s>(cx, NS_ERROR_XPC_BAD_CONVERT_JS);\n"
> +                "}\n" % toStringBool(not isWorker))

I had some comments on this in part 1.

@@ +1290,5 @@
> +                    {
> +                        "val" : "temp",
> +                        "declName" : "*arr.AppendElement()"
> +                        }
> +                    ))).define()

Should this use instantiateJSToNativeConversionTemplate?

@@ +1318,5 @@
>          # Allow null pointers for nullable types and old-binding classes
>          argIsPointer = type.nullable() or type.unroll().inner.isExternal()
>  
>          # Non-worker callbacks have to hold a strong ref to the thing being
> +        # passed down.  So do sequences.

Sequences and non-worker callbacks...

@@ +1433,2 @@
>          template += (
>              # XXXbz We don't know whether we're on workers, so play it safe

We know now through descriptorProvider, no?

@@ +2153,5 @@
>  
>              # XXXbz Now we're supposed to check for distinguishingArg being
>              # an array or a platform object that supports indexed
>              # properties... skip that last for now.  It's a bit of a pain.
> +            pickFirstSignature("%s.isObject() && IsArrayLike(cx, &%s.toObject())" %

Heh.
Attachment #621330 - Flags: review?(peterv) → review+
Comment on attachment 621330 [details] [diff] [review]
Part 2 with IsArrayLike fixed to use compartments correctly

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

::: dom/bindings/Codegen.py
@@ +1404,5 @@
>                      "  ${holderName} = tmp;\n"
>                      "}\n")
>  
>              # And store our tmp, before it goes out of scope.
>              templateBody += "${declName} = tmp;"

I forgot to ask: does this do an additional AddRef (with a Release on ${holderName} when it goes out of scope) if forceOwningType is True? Might be nice to avoid that, even though this code should go away eventually.
Attachment #621444 - Flags: review?(peterv) → review+
(In reply to Peter Van der Beken [:peterv] from comment #18)
> Don't need triple quotes here. (You could even reindent?)

Done the first.  Reindenting involves ugliness or running over 80 chars, so I left it as it is for now.

> > +// Use a temp nsCOMPtr for the null-check, because ${target} might be NonNull
> 
> Should that be 'might be OwningNonNull'? Because if it might be NonNull this
> could leak?

Fixed comment to read OwningNonNull.  If ${target} were actually NonNull, the generated C++ just wouldn't compile.  This comment now says:

  // Use a temp nsCOMPtr for the null-check, because ${target} might be
  // OwningNonNull, not an nsCOMPtr.

> Why doesn't callback, arraybuffer, string, enum or primitive unwrapping us failureCode?

For callback, that's just a bug.  Fixed by just passing our failureCode as the codeOnFailure arg to CallbackObjectUnwrapper.

For arraybuffer it's likewise a bug, fixed in the patches in bug 749864.  So I'd prefer to just leave it as-is here and follow up in that bug.

For string and enum and primitive there is no way to get a non-fatal failure (i.e. failure due to the object being the wrong sort of thing).  Those can only fail due to JS exceptions being thrown or out of memory conditions.  Documented that by changing the last sentence of the comment to say:

    That means that failures to convert due to the JS value being the wrong type of
    value need to use failureCode instead of throwing exceptions.  Failures to
    convert that are due to JS exceptions (from toString or valueOf methods) or
    out of memory conditions need to throw exceptions no matter what
    failureCode is.

> We're not passing a xpc_qsSelfRef but a nsCOMPtr.

Comment fixed.

> Though it might be cleaner to have all of these not have a starting \n and have a
> trailing \n.

Did that.

> But most other stuff doesn't have a trailing \n, so do we really need one
> here?

No, we do not.  Fixed.

> Given that you removed the last consumer of unindenter please remove it too.

Done.

> Maybe add the message "NonNull<...> was set to null" or something like that
> for the second assert? I think that's what it's checking?

Done.
(In reply to Peter Van der Beken [:peterv] from comment #19)
> Nit: I'd line up JS_IsArrayBufferObject with JS_IsArrayObject.

Actually, he JS_IsArrayBuffer object check is just bogus: arraybuffers are not arraylike.  I've removed that bit.

> Should this use instantiateJSToNativeConversionTemplate?

No.  instantiateJSToNativeConversionTemplate is for cases when you want to unwrap into a holder on the stack; it sets up the stack variables to unwrap into and whatnot.  In our case we most definitely do not want to unwrap onto the stack; we want to do it into the array.

> Sequences and non-worker callbacks...

Done.

> We know now through descriptorProvider, no?

Yes.  The patch in bug 749864 fixes this; I'd rather not churn it here and bitrot that patch.

> does this do an additional AddRef (with a Release on ${holderName} when it goes out of
> scope) if forceOwningType is True?

It will if ${holderName} is actually non-null (so the non-castable-native case).  I'm not sure how to fix this short of a null-check on ${holderName}.  On the other hand, the non-castable-native case is already pretty slow, no?  Leaving this as-is for now, but happy to do a followup if you want me to.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ab9330f8337
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ba14e6312ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/a23d7ab14732
Flags: in-testsuite?
Whiteboard: [need review]
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/7ab9330f8337
https://hg.mozilla.org/mozilla-central/rev/7ba14e6312ea
https://hg.mozilla.org/mozilla-central/rev/a23d7ab14732
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Added decent tests in https://hg.mozilla.org/integration/mozilla-inbound/rev/e0d8fa7fe174
Flags: in-testsuite? → in-testsuite+
Blocks: 742214
Blocks: 742202
You need to log in before you can comment on or make changes to this bug.