791347 - Support non-nsISupports refcounted natives and non-refcounted natives in new DOM bindings

Status: RESOLVED FIXED

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

Description

[Peter Van der Beken [:peterv]]

I'd like us to support non-nsISupports natives in the new bindings. I've come up with an approach that allows us to keep doing deferred releases by having each of these bindings register callbacks that XPConnect can call when it wants to do deferred releases (I called it deferred finalization, since we sometimes just destroy instead of releasing). For now we still need nsISupports for preserving wrappers, but I'm hopeful we can do away with that eventually too.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: v1

Olly, Andrew, could one of you take a look at the changes to support deferred releases in XPConnect and let me know what you think? Thanks!

Comment 2

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

Comment on attachment 661339 [details] [diff] [review]
v1

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

::: dom/bindings/Bindings.conf
@@ +51,5 @@
>  #                   Defaults to true.
> +#   * nativeOwnership: Describes how the native object is held. 4 possible
> +#                      types: worker object ("worker"), non-refcounted object
> +#                      ("owned"), refcounted non-nsISupports object
> +#                      ("refcounted") or nsISupports ("nsiupports"). Always

"upports"

::: dom/bindings/Codegen.py
@@ +633,5 @@
>      def generate_code(self):
>          # FIXME https://bugzilla.mozilla.org/show_bug.cgi?id=774279
>          # Using a real trace hook might enable us to deal with non-nsISupports
>          # wrappercached things here.
> +        assert self.descriptor.nativeOwnership is 'nsisupports'

==, I think; also elsewhere

@@ +683,5 @@
> +  bool done = newLen == 0;
> +  if (done) {
> +    delete pointers;
> +  }
> +  return done;""" % (smartPtr, smartPtr)

if (newLen == 0) {
  delete pointers;
  return true;
}
return false;

seems slightly clearer to me.

::: dom/bindings/Configuration.py
@@ +227,5 @@
>                              self.interface.identifier.name)
>          self.prefable = desc.get('prefable', False)
>  
> +        if self.workers:
> +            self.nativeOwnership = "worker"

Probably want to check that nothing is specified in the conf file.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +605,5 @@
> +        items->RemoveElementAt(count - 1);
> +        NS_RELEASE(wrapper);
> +
> +        if (slice > 0 && ++counter == slice) {
> +            return items->Length() == 0;

->IsEmpty()?

::: js/xpconnect/src/xpcprivate.h
@@ +707,5 @@
> +     * Infrastructure for classes that need to defer part of the finalization
> +     * until after the GC has run, for example for objects that we don't want to
> +     * destroy during the GC.
> +     */
> +    

Trailing whitespace.

@@ +736,5 @@
> +        DeferredFinalizeFunctions* item =
> +            mDeferredFinalizeFunctions.AppendElement();
> +        if (!item) {
> +            return false;
> +        }

nsAutoTArray is infallible by default, no?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 661339 [details] [diff] [review]
v1

>+++ b/dom/bindings/Bindings.conf
>+#   * nativeOwnership: Describes how the native object is held. 4 possible

I think this comment should describe what the binding object will do when it's finalized.  I assume the answers are "whatever is needed for workers" for "worker", "call delete" for "owned", "call Release" for the other two types, but it should be documented.

>+++ b/dom/bindings/Codegen.py
>+Take(*defer, self);""" % (smartPtr, smartPtr, smartPtr, smartPtr)

This might work better with a %(smartPtr)s version of replacement, where you'd only have { "smartPtr": smartPtr } at the end here.  Would also make the template string more readable, I think.

r=me with that.

Comment 4

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 661339 [details] [diff] [review]
v1

># HG changeset patch
># Parent b9c9cb7f26487806294cfca5938931e4ffba0b3c
># User Peter Van der Beken <[e-mail]>
>Fix for bug 791347 (Support non-nsISupports refcounted natives and non-refcounted natives in new DOM bindings).
>
>diff --git a/dom/bindings/BindingUtils.h b/dom/bindings/BindingUtils.h
>--- a/dom/bindings/BindingUtils.h
>+++ b/dom/bindings/BindingUtils.h
>@@ -1140,12 +1140,28 @@ XrayEnumerateProperties(JS::AutoIdVector
>                         jsid* attributeIds,
>                         JSPropertySpec* attributeSpecs,
>                         size_t attributeCount,
>                         Prefable<ConstantSpec>* constants,
>                         jsid* constantIds,
>                         ConstantSpec* constantSpecs,
>                         size_t constantCount);
> 
>+// Transfer reference in ptr to smartPtr.
>+template<class T>
>+inline void
>+Take(nsRefPtr<T>& smartPtr, T* ptr)
>+{
>+  smartPtr = dont_AddRef(ptr);
>+}

Uh, does new bindings code use yet another coding style :(
2 space indentation but JS/XPConnect naming convention.

>+++ b/dom/bindings/Bindings.conf
>@@ -44,16 +44,22 @@
> #                   dict). The keys are the property names as they appear in the
> #                   .webidl file and the values are the names as they should be
> #                   in the WebIDL.
> #   * wrapperCache: True if this object is a wrapper cache.  Objects that are
> #                   not can only be returned from a limited set of methods,
> #                   cannot be prefable, and must ensure that they disallow
> #                   XPConnect wrapping.  Always true for worker descriptors.
> #                   Defaults to true.
>+#   * nativeOwnership: Describes how the native object is held. 4 possible
>+#                      types: worker object ("worker"), non-refcounted object
>+#                      ("owned"), refcounted non-nsISupports object
>+#                      ("refcounted") or nsISupports ("nsiupports").
nsisupports.

Hmm, but do we really need the 'owned' case? 'owned' is just for cases when
some simple C++ object is accessed only by JS and doesn't keep anything else alive?
Couldn't refcounted non-nsISupports work too? In practice binding object would just be
the only one to addref/release, but would make the code less complicated.


>+
>+    /**
>+     * Infrastructure for classes that need to defer part of the finalization
>+     * until after the GC has run, for example for objects that we don't want to
>+     * destroy during the GC.
>+     */
>+    
>+    // Called once before the deferred finalization starts. Should hand off the
>+    // buffer with things to finalize in the return value.
>+    typedef void* (*DeferredFinalizeStartFunction)();
The name isn't very descriptive.
Maybe GetThingsForDeferredFinalization()


f+ but I'd prefer removing support for 'owned' unless it is really needed.

Comment 5

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

With 'owned' the C++ class needs MOZ_COUNT_CTOR/DTOR. It is easy to forget those.

Comment 6

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

So, either remove 'owned' or add something which forces sane memory logging for
'owned' objects (could be debug only).

Comment 7

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Also, as I mentioned on IRC, I'm a bit worried that there isn't any easy way to
keep 'owned' alive (in stack) while operating on them.

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: Force logging ctors/dtors for 'owned' classes

With the patch in bug 711628 this gives:

/Users/peterv/source/mozilla/media/DBG/dom/bindings/MediaStreamListBinding.cpp:504:3: error: no matching function for call to 'MustInheritFromNonRefcountedDOMObject'
  MustInheritFromNonRefcountedDOMObject(aObject);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../dist/include/mozilla/dom/BindingUtils.h:1185:1: note: candidate function not viable: no known conversion from 'mozilla::dom::MediaStreamList *' to 'mozilla::dom::NonRefcountedDOMObject *' for 1st argument; 
MustInheritFromNonRefcountedDOMObject(NonRefcountedDOMObject*)
^
1 error generated.

Inheriting from NonRefcountedDOMObject makes us log ctor and dtor for "NonRefcountedDOMObject", the class can still add ctor/dtor logging of its own. We could avoid double-logging in that case by adding a template param to signal 'already logging', but the complexity doesn't seem worth it.

Comment 9

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 661785 [details] [diff] [review]
Force logging ctors/dtors for 'owned' classes

Thanks.

Comment 10

[Andrew McCreight [:mccr8]]

Comment on attachment 661339 [details] [diff] [review]
v1

Olli's more familiar with this than me, so his f+ is probably enough.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Once this lands, we should consider restricting throwing in a ChromeOnly QueryInterface to the nsISupports cases, at the very least...

Comment 12

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6362581f442e

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6362581f442e

Should this have a test?

Comment 14

[Boris Zbarsky [:bzbarsky]]

It's not testable yet.  Once people write code using the new capabilities, they will be tested...