Closed Bug 1114804 Opened 5 years ago Closed 5 years ago

Make ISupports DeferredFinalize use the standard bindings finalizer code

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
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
Attachment #8540414 - Attachment is obsolete: true
Attachment #8540733 - Flags: review?(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 on attachment 8540733 [details] [diff] [review]
Make ISupports use the standard deferred finalizer code.

Yeah, that's a good idea.
Attachment #8540733 - Flags: review?(peterv)
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.
Attachment #8540733 - Flags: review?(peterv)
Attachment #8540733 - Flags: review?(peterv) → review+
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
Attachment #8540733 - Attachment is obsolete: true
Attachment #8570204 - Flags: review?(peterv)
Blocks: 1137517
(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?
Flags: needinfo?(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?
Attachment #8570204 - Flags: review?(peterv) → review+
(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.
Flags: needinfo?(peterv)
(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.
(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!
(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.
https://hg.mozilla.org/mozilla-central/rev/654836515933
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
This seems to have fixed bug 997908 (see bug 997908 comment #239).  So should we consider uplifting it to aurora and beta?
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
Attachment #8570204 - Flags: approval-mozilla-aurora?
(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 on attachment 8570204 [details] [diff] [review]
Make ISupports use the standard deferred finalizer code.

Fix a top crash, taking it (with pleasure :)
Attachment #8570204 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
> 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.
Depends on: 1162024
Depends on: 1170045
You need to log in before you can comment on or make changes to this bug.