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

RESOLVED FIXED in Firefox 21

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: smaug, Assigned: dzbarsky)

Tracking

unspecified
mozilla22
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox20+ wontfix, firefox21+ fixed, firefox22+ fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
tracking-firefox21: --- → ?
tracking-firefox22: --- → ?
(Reporter)

Comment 1

6 years ago
Oh, bug 821955 is already in FF20.
tracking-firefox20: --- → ?
It would be nice if we could somehow warn if people are shadowing mRefCnt.

Comment 3

6 years ago
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.
Assignee: nobody → dzbarsky
Flags: needinfo?(bugs)
(Assignee)

Comment 4

6 years ago
Created attachment 720230 [details] [diff] [review]
Patch
Attachment #720230 - Flags: review?(bugs)
(Reporter)

Comment 5

6 years ago
(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.
Flags: needinfo?(bugs)
(Reporter)

Comment 6

6 years ago
Comment on attachment 720230 [details] [diff] [review]
Patch

You should unlink mElement
Attachment #720230 - Flags: review?(bugs) → review+
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.
(Reporter)

Comment 8

6 years ago
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);
https://hg.mozilla.org/mozilla-central/rev/383fecb8fa17
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
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.
status-firefox20: --- → affected
status-firefox21: --- → affected
status-firefox22: --- → affected
tracking-firefox20: ? → +
tracking-firefox21: ? → +
tracking-firefox22: ? → +

Updated

6 years ago
status-firefox22: affected → fixed
(Assignee)

Comment 12

6 years ago
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
Attachment #720230 - Flags: approval-mozilla-beta?
Attachment #720230 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 13

6 years ago
(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.
(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?
(Assignee)

Comment 15

6 years ago
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.
Flags: needinfo?(bugs)
(Reporter)

Comment 16

6 years ago
To find the cases when there might be some leaks require code inspection, I expect.
Flags: needinfo?(bugs)
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
Attachment #720230 - Flags: approval-mozilla-beta?
Attachment #720230 - Flags: approval-mozilla-beta-
Attachment #720230 - Flags: approval-mozilla-aurora?
Attachment #720230 - Flags: approval-mozilla-aurora+
status-firefox20: affected → wontfix
status-firefox21: affected → fixed
You need to log in before you can comment on or make changes to this bug.