Closed Bug 791347 Opened 8 years ago Closed 8 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: peterv, Assigned: peterv)

References

Details

Attachments

(2 files)

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.
Attached patch v1Splinter Review
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!
Attachment #661339 - Flags: review?(bzbarsky)
Attachment #661339 - Flags: feedback?(continuation)
Attachment #661339 - Flags: feedback?(bugs)
Blocks: 711628
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 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.
Attachment #661339 - Flags: review?(bzbarsky) → review+
Comment on attachment 661339 [details] [diff] [review]
v1

># HG changeset patch
># Parent b9c9cb7f26487806294cfca5938931e4ffba0b3c
># User Peter Van der Beken <peterv@propagandism.org>
>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.
Attachment #661339 - Flags: feedback?(bugs) → feedback+
With 'owned' the C++ class needs MOZ_COUNT_CTOR/DTOR. It is easy to forget those.
So, either remove 'owned' or add something which forces sane memory logging for
'owned' objects (could be debug only).
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.
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.
Attachment #661785 - Flags: review?(bugs)
Comment on attachment 661785 [details] [diff] [review]
Force logging ctors/dtors for 'owned' classes

Thanks.
Attachment #661785 - Flags: review?(bugs) → review+
Comment on attachment 661339 [details] [diff] [review]
v1

Olli's more familiar with this than me, so his f+ is probably enough.
Attachment #661339 - Flags: feedback?(continuation)
Once this lands, we should consider restricting throwing in a ChromeOnly QueryInterface to the nsISupports cases, at the very least...
https://hg.mozilla.org/mozilla-central/rev/6362581f442e

Should this have a test?
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
It's not testable yet.  Once people write code using the new capabilities, they will be tested...
Depends on: 811057
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.