774970 - Allow new-binding this unwrapping to deal with old-binding objects in some cases

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

We're going to need this for Node and Element and such, while we do the incremental conversion, to deal with code that does Node.prototype.appendChild.call(old_binding_element, newChild).

Comment 1

[Boris Zbarsky [:bzbarsky]]

Oh, I say "limited" because I have no problem with it only working for some classes, and classes will need to opt in to it.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Add the ability to generate code for dealing with an XPConnec 'this' object in some cases.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I think this should do the trick, both for "this" and for arguments.  Hard to test without doing the Node conversion....  Example of generated code if I flag TestInterface with this flag (for the passSelf method in the test IDL):

  mozilla::dom::TestInterface* self;
  {
    nsresult rv = UnwrapObject<prototypes::id::TestInterface, mozilla::dom::TestInterface>(cx, obj, self);
    if (NS_FAILED(rv)) {
      mozilla::dom::TestInterface *objPtr;
      xpc_qsSelfRef objRef;
      JS::Value val = JS::ObjectValue(*obj);
      nsresult rv = xpc_qsUnwrapArg<mozilla::dom::TestInterface>(cx, val, &objPtr, &objRef.ptr, &val);
      if (NS_FAILED(rv)) {
       return Throw<true>(cx, rv);
      }
      // We should be castable!
      MOZ_ASSERT(!objRef.ptr);
      self = objPtr;
    }
  }

....

  NonNull<mozilla::dom::TestInterface> arg0;
  if (argv[0].isObject()) {
    {
      nsresult rv = UnwrapObject<prototypes::id::TestInterface, mozilla::dom::TestInterface>(cx, &argv[0].toObject(), arg0);
      if (NS_FAILED(rv)) {
        mozilla::dom::TestInterface *objPtr;
        xpc_qsSelfRef objRef;
        JS::Value val = JS::ObjectValue(*&argv[0].toObject());
        nsresult rv = xpc_qsUnwrapArg<mozilla::dom::TestInterface>(cx, val, &objPtr, &objRef.ptr, &val);
        if (NS_FAILED(rv)) {
          return Throw<true>(cx, rv);
        }
        // We should be castable!
        MOZ_ASSERT(!objRef.ptr);
        arg0 = objPtr;
	// We should have an object, too!
	MOZ_ASSERT(objPtr);
      }
    }
  } else {
     return Throw<false>(cx, NS_ERROR_XPC_BAD_CONVERT_JS);
  }

Comment 4

[Boris Zbarsky [:bzbarsky]]

Er, the "we should have an object too" bits in the above paste were hand-added, incorrectly.  The patch code is right, though.  ;)

Comment 5

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

Comment on attachment 644641 [details] [diff] [review]
Add the ability to generate code for dealing with an XPConnec 'this' object in some cases.

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

'XPConnect'

::: dom/bindings/Codegen.py
@@ +1449,5 @@
>                                "codeOnFailure" : CGIndenter(CGGeneric(codeOnFailure), 4).define() }
> +        if descriptor.hasXPConnectImpls:
> +            # We don't use xpc_qsUnwrapThis because it will always throw on
> +            # unwrap failure, whereas we want to control whether we throw or
> +            # not.

Not strictly true (xpc_qsUnwrapThis has a failureFatal argument), but it would be annoying.

@@ +1451,5 @@
> +            # We don't use xpc_qsUnwrapThis because it will always throw on
> +            # unwrap failure, whereas we want to control whether we throw or
> +            # not.
> +            self.substitution["codeOnFailure"] = CGIndenter(CGGeneric(string.Template(
> +                "${type} *objPtr;\n"

* to the left in this neck of the woods

Comment 6

[Boris Zbarsky [:bzbarsky]]

> xpc_qsUnwrapThis has a failureFatal argument

Which doesn't clearly communicate back what exactly happened (returns NS_OK even if unwrapping failed).

Comment 7

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/41b8acc38356

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/41b8acc38356

Comment 9

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

Do we want to turn this on for EventTarget?

Comment 10

[Boris Zbarsky [:bzbarsky]]

That's a good question.  I guess if we did we could add some tests for it and make sure it actually works...

On the other hand, if we don't need it for compat, I'm not sure we need the extra code.