817256 - DOMSVGTransform holds weak ref to matrix

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[David Zbarsky (:dzbarsky)]

(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?
>

Comment 1

[David Zbarsky (:dzbarsky)]

Hmm, this seems to work:
http://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E%0A%3Cscript%3E%0Atransform%20%3D%20document.createElementNS%28%22http%3A%2F%2Fwww.w3.org%2F2000%2Fsvg%22%2C%20%22svg%22%29.createSVGTransform%28%29%3B%0Atransform.matrix.expando%20%3D%201%3B%0Aw%28transform.matrix.expando%29%3B%0A%3C%2Fscript%3E

Does this mean that something is keeping the matrix alive?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yes, like the JS object.  Now if you triggered a GC between the expando set and the read...

Comment 3

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 4

[Robert Longson [: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.

Comment 5

[David Zbarsky (:dzbarsky)]

(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.

Comment 6

[Robert Longson [:longsonr]]

It's not a weak ref any more though is it?

Comment 7

[David Zbarsky (:dzbarsky)]

I fixed the first comment.  But DOMSVGTransformList still holds weak refs to its items.

Comment 8

[Robert Longson [:longsonr]]

OK, I misunderstood what that comment was referring to.

Comment 9

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6aa0940b3199

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6aa0940b3199