Closed Bug 817256 Opened 9 years ago Closed 9 years ago

DOMSVGTransform holds weak ref to matrix

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: dzbarsky, Assigned: dzbarsky)

References

Details

Attachments

(1 file)

(In reply to Boris Zbarsky (:bz) from comment #18)
> could you file a followup bug on the fact that this code is
> totally broken if the user script sets expandos on the matrix?
>
QA Contact: dzbarsky
Assignee: nobody → dzbarsky
QA Contact: dzbarsky
Component: DOM → SVG
Yes, like the JS object.  Now if you triggered a GC between the expando set and the read...
Attached patch PatchSplinter Review
Attachment #700890 - Flags: review?(longsonr)
Comment on attachment 700890 [details] [diff] [review]
Patch

> 
> // Make sure we clear the weak ref in the owning transform (if there is one)
> // upon unlink.

Remove the comment above since it's now wrong.

> NS_IMPL_CYCLE_COLLECTION_CLASS(DOMSVGMatrix)
> NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(DOMSVGMatrix)
>-  if (tmp->mTransform) {
>-    tmp->mTransform->ClearMatrixTearoff(tmp);
>-  }


> // We could use NS_IMPL_CYCLE_COLLECTION_1, except that in Unlink() we need to
> // clear our list's weak ref to us to be safe. (The other option would be to
> // not unlink and rely on the breaking of the other edges in the cycle, as
> // NS_SVG_VAL_IMPL_CYCLE_COLLECTION does.)

This comment is wrong too and needs rewriting or removing.
Attachment #700890 - Flags: review?(longsonr) → review+
(In reply to Robert Longson from comment #4)
> Comment on attachment 700890 [details] [diff] [review]

> 
> 
> > // We could use NS_IMPL_CYCLE_COLLECTION_1, except that in Unlink() we need to
> > // clear our list's weak ref to us to be safe. (The other option would be to
> > // not unlink and rely on the breaking of the other edges in the cycle, as
> > // NS_SVG_VAL_IMPL_CYCLE_COLLECTION does.)
> 
> This comment is wrong too and needs rewriting or removing.

Why is this wrong?  We're still unlinking the list.
It's not a weak ref any more though is it?
I fixed the first comment.  But DOMSVGTransformList still holds weak refs to its items.
OK, I misunderstood what that comment was referring to.
https://hg.mozilla.org/mozilla-central/rev/6aa0940b3199
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Depends on: 831673
Depends on: 831668
You need to log in before you can comment on or make changes to this bug.