Last Comment Bug 748267 - Paris bindings codegen for sequence types
: Paris bindings codegen for sequence types
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
:
Mentors:
Depends on: 748637 752042
Blocks: 742185 742202 742214 748266 749864 749866
  Show dependency treegraph
 
Reported: 2012-04-24 01:16 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2012-06-13 08:46 PDT (History)
2 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
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 (29.99 KB, patch)
2012-04-25 22:18 PDT, Boris Zbarsky [:bz] (still a bit busy)
peterv: review+
Details | Diff | Splinter Review
Part 1 diff -b, for maybe more ease of reviewing (28.82 KB, patch)
2012-04-25 22:20 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
part 2. Implement codegen for dealing with sequence arguments. (16.43 KB, patch)
2012-04-25 22:25 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter 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. (8.49 KB, patch)
2012-04-25 22:27 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter 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 (8.86 KB, patch)
2012-04-25 22:38 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Support running code after successful wrapping (10.17 KB, patch)
2012-04-26 14:48 PDT, Peter Van der Beken [:peterv]
no flags Details | Diff | Splinter Review
Part 2, now using JS_GetArrayLength (16.39 KB, patch)
2012-04-27 17:25 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Part 2, now using JS_GetArrayLength, and compiling (16.41 KB, patch)
2012-04-27 17:31 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Part 2, now using JS_GetArrayLength, and compiling for real (16.41 KB, patch)
2012-04-27 17:36 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Part 4 now renumbered as part 3 and merged on top of the patch in (9.00 KB, patch)
2012-05-05 11:18 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Part 2 with IsArrayLike fixed to use compartments correctly (17.00 KB, patch)
2012-05-05 11:56 PDT, Boris Zbarsky [:bz] (still a bit busy)
peterv: review+
Details | Diff | Splinter Review
Part 3 merged on top of latest diff in bug 752042 (9.09 KB, patch)
2012-05-06 12:03 PDT, Boris Zbarsky [:bz] (still a bit busy)
peterv: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2012-04-24 01:16:13 PDT
It is time.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2012-04-25 22:18:33 PDT
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
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2012-04-25 22:20:21 PDT
Created attachment 618552 [details] [diff] [review]
Part 1 diff -b, for maybe more ease of reviewing
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2012-04-25 22:25:15 PDT
Created attachment 618556 [details] [diff] [review]
part 2.  Implement codegen for dealing with sequence arguments.
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2012-04-25 22:27:16 PDT
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.
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2012-04-25 22:38:14 PDT
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
Comment 6 Peter Van der Beken [:peterv] 2012-04-26 14:48:15 PDT
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).
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2012-04-26 23:07:40 PDT
Yes, that diff does everything I need in part 3.  I just needed to turn various "return true" into "continue" on demand.  ;)
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2012-04-26 23:11:19 PDT
Though I don't understand why the declarejsval bit, but I expect seeing a consumer would help.
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2012-04-26 23:16:28 PDT
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.
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2012-04-27 17:25:47 PDT
Created attachment 619221 [details] [diff] [review]
Part 2, now using JS_GetArrayLength
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2012-04-27 17:31:43 PDT
Created attachment 619222 [details] [diff] [review]
Part 2, now using JS_GetArrayLength, and compiling
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2012-04-27 17:36:36 PDT
Created attachment 619224 [details] [diff] [review]
Part 2, now using JS_GetArrayLength, and compiling for real
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2012-05-05 11:18:25 PDT
Created attachment 621325 [details] [diff] [review]
Part 4 now renumbered as part 3 and merged on top of the patch in
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2012-05-05 11:19:10 PDT
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.
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2012-05-05 11:56:16 PDT
Created attachment 621330 [details] [diff] [review]
Part 2 with IsArrayLike fixed to use compartments correctly
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2012-05-06 12:03:27 PDT
Created attachment 621444 [details] [diff] [review]
Part 3 merged on top of latest diff in bug 752042
Comment 17 Boris Zbarsky [:bz] (still a bit busy) 2012-05-06 12:05:18 PDT
Comment on attachment 618812 [details] [diff] [review]
Support running code after successful wrapping

This has been spun out to bug 752042.
Comment 18 Peter Van der Beken [:peterv] 2012-05-15 01:27:19 PDT
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?
Comment 19 Peter Van der Beken [:peterv] 2012-05-15 02:31:26 PDT
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.
Comment 20 Peter Van der Beken [:peterv] 2012-05-15 02:36:18 PDT
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.
Comment 21 Boris Zbarsky [:bz] (still a bit busy) 2012-05-15 10:24:39 PDT
(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.
Comment 22 Boris Zbarsky [:bz] (still a bit busy) 2012-05-15 11:21:57 PDT
(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.
Comment 25 Boris Zbarsky [:bz] (still a bit busy) 2012-05-29 20:57:25 PDT
Added decent tests in https://hg.mozilla.org/integration/mozilla-inbound/rev/e0d8fa7fe174

Note You need to log in before you can comment on or make changes to this bug.