1114804 - Make ISupports DeferredFinalize use the standard bindings finalizer code

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

We have some weird crashes in this code in bug 997908, and I don't quite trust how it handles the ownership of the array, so I think we should just make the ISupports deferred finalize stuff use the same code that non-ISupports uses.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Make ISupports use the standard deferred finalizer code.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Make ISupports use the standard deferred finalizer code.

One slight change I made is that CycleCollectedJSRuntime::FinalizeDeferredThings returns early when there's nothing in the array. It used to be that ISupports would always get added, even if there weren't any, but now we just won't in that case, so we end up hitting an assert about doing things when there's nothing to do.

try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cd1f9777841e

Comment 3

[Peter Van der Beken [:peterv]]

Comment on attachment 8540733 [details] [diff] [review]
Make ISupports use the standard deferred finalizer code.

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

Shouldn't this move DeferredFinalizerImpl to xpcom/base?

Comment 4

[Andrew McCreight [:mccr8]]

Comment on attachment 8540733 [details] [diff] [review]
Make ISupports use the standard deferred finalizer code.

Yeah, that's a good idea.

Comment 5

[Andrew McCreight [:mccr8]]

Comment on attachment 8540733 [details] [diff] [review]
Make ISupports use the standard deferred finalizer code.

Well, given that I'm changing the code here, I'll add a second patch to this bug for moving the code into XPCOM, so I think this can get reviewed as is, unless you want to see how those changes would look, too.  I'll probably create a new file for these in XPCOM, then include them in BindingUtils.h.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: Make ISupports use the standard deferred finalizer code.

Bug 1096328 removed the SmartPtr template parameter that I was
using. Rather than adding it back in, or using nsRefPtr<nsISupports>,
I added in a new case for nsISupports to the SmartPtr conditional type
definition.  Otherwise the patch should be the same as before.

try run just in case:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fc71868abf8

Comment 7

[Andrew McCreight [:mccr8]]

(In reply to Peter Van der Beken [:peterv] from comment #3)
> Shouldn't this move DeferredFinalizerImpl to xpcom/base?

So, now that I'm looking at this, the way it is right now this will require dragging all of the IsRefcounted machinery over into XPCOM, too.  Do you think I should go ahead and do that, or just leave it in BindingUtils.h?

Comment 8

[Peter Van der Beken [:peterv]]

Comment on attachment 8570204 [details] [diff] [review]
Make ISupports use the standard deferred finalizer code.

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

::: dom/bindings/BindingUtils.h
@@ +2864,5 @@
> +  typedef typename Conditional<IsSame<T, nsISupports>::value,
> +                               nsCOMPtr<T>,
> +                               typename Conditional<IsRefcounted<T>::value,
> +                                                    nsRefPtr<T>,
> +                                                    nsAutoPtr<T>>::Type>::Type SmartPtr;

Can we statically assert that if IsSame<T, nsISupports>::value is false then IsBaseOf<nsISupports, T>::value is also false?

Comment 9

[Peter Van der Beken [:peterv]]

(In reply to Andrew McCreight [:mccr8] from comment #7)
> So, now that I'm looking at this, the way it is right now this will require
> dragging all of the IsRefcounted machinery over into XPCOM, too.  Do you
> think I should go ahead and do that, or just leave it in BindingUtils.h?

Ugh :-(. Maybe it's not worth it after all.

Comment 10

[Andrew McCreight [:mccr8]]

(In reply to Peter Van der Beken [:peterv] from comment #8)
> Can we statically assert that if IsSame<T, nsISupports>::value is false then
> IsBaseOf<nsISupports, T>::value is also false?

That's not true, is it?  If T != nsISupports, it could still have nsISupports as a base class.

Comment 11

[Peter Van der Beken [:peterv]]

(In reply to Andrew McCreight [:mccr8] from comment #10)
> That's not true, is it?  If T != nsISupports, it could still have
> nsISupports as a base class.

Er, how can we end up with a DeferredFinalizerImpl with a T that has nsISupports as a base class but T != nsISupports? I don't think that can happen, but if it does we definitely want to avoid that. DeferredFinalizer casts to nsISupports* if IsBaseOf<nsISupports, T>::value, if there's another path to a DeferredFinalizerImpl it should surely do the same!

Comment 12

[Andrew McCreight [:mccr8]]

(In reply to Peter Van der Beken [:peterv] from comment #11)
> Er, how can we end up with a DeferredFinalizerImpl with a T that has
> nsISupports as a base class but T != nsISupports? I don't think that can
> happen, but if it does we definitely want to avoid that. DeferredFinalizer
> casts to nsISupports* if IsBaseOf<nsISupports, T>::value, if there's another
> path to a DeferredFinalizerImpl it should surely do the same!

Oh, I see what you mean now.  I wasn't thinking about how DeferredFinalizerImpl is only instantiated through DeferredFinalizer.

Comment 13

[Andrew McCreight [:mccr8]]

Landed with the static_assert.

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

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 15

[Steven Michaud [:smichaud] (Retired)]

This seems to have fixed bug 997908 (see bug 997908 comment #239).  So should we consider uplifting it to aurora and beta?

Comment 16

[Andrew McCreight [:mccr8]]

Comment on attachment 8570204 [details] [diff] [review]
Make ISupports use the standard deferred finalizer code.

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: This fixes a long-standing Mac topcrash, bug 997908.  It would be nice to get this out a little earlier, but it wouldn't be the end of the world if it waited another 5 weeks.
[Describe test coverage new/current, TreeHerder]: this code runs all the time
[Risks and why]: shouldn't be too bad. It just makes us use some existing code in more places.
[String/UUID change made/needed]: none

Comment 17

[Andrew McCreight [:mccr8]]

(In reply to Steven Michaud from comment #15)
> This seems to have fixed bug 997908 (see bug 997908 comment #239).  So
> should we consider uplifting it to aurora and beta?

The final build of beta is tomorrow or something, so I think it is better to skip beta.

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8570204 [details] [diff] [review]
Make ISupports use the standard deferred finalizer code.

Fix a top crash, taking it (with pleasure :)

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4c8396eda749

Comment 20

[Andrew McCreight [:mccr8]]

Ryan asked about backporting this to b2g, but given that bug 1096328 landed on 38 and would require a backport be changed a bit, I'm inclined to not land it on those older branches without any evidence that they are being affected by this.  Of course, it is a mystery to me why this is only showing up in our crash stats on OSX.

Comment 21

[Steven Michaud [:smichaud] (Retired)]

> it is a mystery to me why this is only showing up in our crash stats on OSX

Bug 997908 also showed up on Windows, in fairly large numbers.  But it wasn't a topcrasher there -- the OS X numbers were proportionately much larger.