857439 - Sort out bindings for 'owned' objects

Status: RESOLVED FIXED

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

Description

[David Zbarsky (:dzbarsky)]

(In reply to Boris Zbarsky (:bz) from comment #14)
> Comment on attachment 732560 [details] [diff] [review]
> Delete owned objects if wrapping fails
> 
> Why do we not need to change the wrappercached case?  Is it not possible to
> have an owned wrappercached object?
> 
> I've been thinking about this some more, and I'm not sure our setup for
> doing 'owned' is sane at all.  In particular, we don't enforce anywhere that
> 'owned' interfaces don't inherit from non-'owned' ones or vice versa... 
> It's a giant footgun.
> 
> So my temptation is that the way we should do this is as follows:
> 
> 1)  Disallow 'owned' on interfaces that either inherit from something or have
>     something inheriting from them.
> 2)  Once we do that, we know exactly which binding to use, so we don't have
> to
>     call WrapObject on the object itself at all, and can just call the
>     binding's Wrap().
> 3)  The binding's Wrap() can explicitly tell us whether it took ownership.
> 
> Or something.
> 
> I realize that's a bunch of changes.  I'm just not happy trying to audit the
> code we have now to make sure we're properly handling deletion in all the
> places where it should happen but not any other places...
> 
> No matter what, we should do step 1, because otherwise reasoning about this
> becomes insane, imo.
> And also, I suggest spinning the whole "make 'owned' sane" bit into a
> separate bug.  And checking with Peter how he thinks it should work, perhaps.

Comment 1

[David Zbarsky (:dzbarsky)]

Attachment: Part 1: Disallow owned interfaces from having parents or children

Comment 2

[David Zbarsky (:dzbarsky)]

Attachment: Part 2: Clean up example codegen a bit

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 739360 [details] [diff] [review]
Part 1: Disallow owned interfaces from having parents or children

Two things:

1)  descriptor.interface.hasChildInterfaces()
2)  Make it an exception that says what you did wrong, not an assert.

r=me

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 739361 [details] [diff] [review]
Part 2: Clean up example codegen a bit

Yes!

Comment 5

[David Zbarsky (:dzbarsky)]

Attachment: Part 3: Fix codegen

Comment 6

[David Zbarsky (:dzbarsky)]

Attachment: Part 4: Fix existing owned bindings

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 739405 [details] [diff] [review]
Part 3: Fix codegen

>+%s""" % (AssertInheritanceChain(self.descriptor),
>                     CreateBindingJSObject(self.descriptor, "global"),

Fix indent here and on the next line?

The problem is, the ordering this thing produces is wrong.  We want to set *aTookOwnership after the CreateBindingJSObject, and before InitUnforgeableProperties bits, no?  Otherwise we could create the object, fail to set an unforgeable, bail out, and double-delete.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 739406 [details] [diff] [review]
Part 4: Fix existing owned bindings

r=me but that TextMetrics.h thing is not in the tree yet... ;)

Comment 9

[David Zbarsky (:dzbarsky)]

Attachment: Part 3: Fix codegen

You're right, we just don't have any owned objects with unforgeable props.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 739413 [details] [diff] [review]
Part 3: Fix codegen

There we go!

r=me

Comment 11

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8674d8a06e67
https://hg.mozilla.org/integration/mozilla-inbound/rev/cef5ac2e149a
https://hg.mozilla.org/integration/mozilla-inbound/rev/c39abce27c56
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a2610c6d785
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e65c02b49e3

Comment 12

[Peter Van der Beken [:peterv]]

Comment on attachment 739413 [details] [diff] [review]
Part 3: Fix codegen

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

Given that this disallows owned wrappercached bindings, how is the value of tookOwnership ever different from whether the return value of WrapObject is non-null?

::: dom/bindings/BindingUtils.h
@@ +666,5 @@
> +  }
> +
> +  // We can end up here in all sorts of compartments, per above.  Make
> +  // sure to JS_WrapValue!
> +  *vp = JS::ObjectValue(*obj);

So if WrapObject failed we dereference null here?

Comment 13

[David Zbarsky (:dzbarsky)]

(In reply to Peter Van der Beken [:peterv] from comment #12)
> Comment on attachment 739413 [details] [diff] [review]
> Part 3: Fix codegen
> 
> Review of attachment 739413 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Given that this disallows owned wrappercached bindings, how is the value of
> tookOwnership ever different from whether the return value of WrapObject is
> non-null?
> 

If WrapObject fails when defining unforgeable properties it returns null but the JSObject took ownership.

> ::: dom/bindings/BindingUtils.h
> @@ +666,5 @@
> > +  }
> > +
> > +  // We can end up here in all sorts of compartments, per above.  Make
> > +  // sure to JS_WrapValue!
> > +  *vp = JS::ObjectValue(*obj);
> 
> So if WrapObject failed we dereference null here?

Doesn't WrapNewBindingNonWrapperCachedObject have the same problem?

Comment 14

[Peter Van der Beken [:peterv]]

> Doesn't WrapNewBindingNonWrapperCachedObject have the same problem?

Sure.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Ugh.  Sorry I missed the lack of null-check on the WrapObject return value!  We should add that, both places.  And in the new code assert that if WrapObject returned non-null then tookOwnership is true....

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8674d8a06e67
https://hg.mozilla.org/mozilla-central/rev/cef5ac2e149a
https://hg.mozilla.org/mozilla-central/rev/c39abce27c56
https://hg.mozilla.org/mozilla-central/rev/3a2610c6d785
https://hg.mozilla.org/mozilla-central/rev/6e65c02b49e3

Comment 17

[David Zbarsky (:dzbarsky)]

Attachment: Add null-checks and assert

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 739764 [details] [diff] [review]
Add null-checks and assert

r=me

Comment 19

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bccba6d0be91

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bccba6d0be91