846710 - nsISVGPoint / DOMSVGTranslatePoint refcounting and cycle collection handling seems to be broken

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

both classes have Addref/Release and refcnt and participate cycle collection, yet
DOMSVGTranslatePoint inherit nsISVGPoint.
Unless proved not to cause problems, I think this should be fixed also in FF21.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Oh, bug 821955 is already in FF20.

Comment 2

[Andrew McCreight [:mccr8]]

It would be nice if we could somehow warn if people are shadowing mRefCnt.

Comment 3

[Alex Keybl [:akeybl]]

Sending over to David since this was a regression from bug 821955.

smaug - can you please provide more info about the theoretical user impact here (since this hasn't yet been demonstrated in the wild)? It's not clear why this should track yet.

Comment 4

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Alex Keybl [:akeybl] from comment #3)
> smaug - can you please provide more info about the theoretical user impact
> here (since this hasn't yet been demonstrated in the wild)? It's not clear
> why this should track yet.
Leaks, and especially such leaks which tend to cause higher cycle collections times and high CC
times bad for user experience.
But I'm not familiar with this SVG code, so perhaps someone can prove that the problem doesn't
actually cause leaks.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 720230 [details] [diff] [review]
Patch

You should unlink mElement

Comment 7

[Andrew McCreight [:mccr8]]

Comment on attachment 720230 [details] [diff] [review]
Patch

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

::: content/svg/content/src/SVGSVGElement.cpp
@@ +61,5 @@
> +                                                  nsISVGPoint)
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mElement)
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
> +
> +NS_IMPL_ADDREF_INHERITED(DOMSVGTranslatePoint, nsISVGPoint)

Olli, should this need to use NS_IMPL_CYCLE_COLLECTING_ADDREF and NS_IMPL_CYCLE_COLLECTING_RELEASE, because you are defining a new participant?  With this, you end up using the Upcast of the parent, but maybe that doesn't matter.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Why would it need NS_IMPL_CYCLE_COLLECTING_ADDREF/RELEASE?
We use NS_IMPL_ADDREF/RELEASE_INHERITED elsewhere which ends up
then calling mRefCnt.decr(base_nsISupports);

Comment 9

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/383fecb8fa17

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/383fecb8fa17

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Given there's a patch up and no one is claiming this does *not* cause terrible leaks, let's go ahead with tracking & uplift if low risk enough patch.  Getting this in before tomorrow would give us more time on Beta to evaluate so please nominate if this looks good on central.

Comment 12

[David Zbarsky (:dzbarsky)]

Comment on attachment 720230 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 821955
User impact if declined: May cause leaks
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): Low risk
String or UUID changes made by this patch: None

Comment 13

[David Zbarsky (:dzbarsky)]

(In reply to David Zbarsky (:dzbarsky) from comment #12)
> Comment on attachment 720230 [details] [diff] [review]

> User impact if declined: May cause leaks

Leaks, and especially such leaks which tend to cause higher cycle collections times and high CC
times bad for user experience.

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to David Zbarsky (:dzbarsky) from comment #13)
> (In reply to David Zbarsky (:dzbarsky) from comment #12)
> > Comment on attachment 720230 [details] [diff] [review]
> 
> > User impact if declined: May cause leaks
> 
> Leaks, and especially such leaks which tend to cause higher cycle
> collections times and high CC
> times bad for user experience.

David - do you think we'd catch these potential leaks with QA testing prior to shipping this release?  Is there any behaviour QA can do to try and catch this early? Or test this patch out on nightly?

Comment 15

[David Zbarsky (:dzbarsky)]

These leaks only occur when specific SVG features are used, so it would depend how extensive the testing is.  Unfortunately, I'm not sure how to construct a testcase that would leak.  Smaug might know.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

To find the cases when there might be some leaks require code inspection, I expect.

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 720230 [details] [diff] [review]
Patch

Given the late timeframe and that the regressing issue has been present for a couple of months now we'll let this go for FF20 and try to get more testing/code review around this over the remaining course of FF21

Comment 18

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/99f0362b3591