1242048 - Make SVG webidl implementation support expandos properly without having to remove [Constant] from them.

Status: NEW

(Core :: SVG, defect)

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch

Comment 2

[Peter Van der Beken [:peterv]]

Why do we want to do this instead of following the spec (using SameObject and making the tearoff tables hold a strong reference)?

Comment 3

[Robert Longson [:longsonr]]

What change do I need to make for a strong reference. If you could just show me how to do say one case e.g. this one...

already_AddRefed<DOMSVGPreserveAspectRatio>
DOMSVGAnimatedPreserveAspectRatio::BaseVal()
{
  RefPtr<DOMSVGPreserveAspectRatio> domBaseVal =
    sBaseSVGPAspectRatioTearoffTable.GetTearoff(mVal);
  if (!domBaseVal) {
    domBaseVal = new DOMSVGPreserveAspectRatio(mVal, mSVGElement, true);
    sBaseSVGPAspectRatioTearoffTable.AddTearoff(mVal, domBaseVal);
  }

  return domBaseVal.forget();
}

Comment 4

[Robert Longson [:longsonr]]

Jonathan, do you know what Peter wants here? The strong reference bit, not the replacing [Constant] with [SameObject] bit which is trivial.

Comment 5

[Jonathan Watt [:jwatt]]

(In reply to Peter Van der Beken [:peterv] from comment #2)
> Why do we want to do this instead of following the spec (using SameObject
> and making the tearoff tables hold a strong reference)?

See point #3 in the long comment at the top of https://dxr.mozilla.org/mozilla-central/source/dom/svg/DOMSVGAnimatedLengthList.h

Basically we want to free memory when possible because some of these SVG DOM objects can use a lot.

Comment 6

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #4)
> Jonathan, do you know what Peter wants here? The strong reference bit, not
> the replacing [Constant] with [SameObject] bit which is trivial.

I assume that manking something as [SameObject] makes the bindings code do all the magic to make sure the object is kept around, the same object is always returned, and that we don't need to have any logic outside the generated bindings code to do that. Assuming that's true, it seems the logical consequence would be that nsSVGAttrTearoffTable and its consumers would go away.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> I assume that manking something as [SameObject] makes the bindings code do all the magic to make sure the
> object is kept around

No, it does not.  The responsibility is on the callee to ensure that the same object is in fact returned each time.  [SameObject] is basically documentation of otherwise hard to guess behavior invariants of the callee, no more, no less.

If you want the bindings to keep stuff alive for you, use [Cached].  But note that this will only keep things alive from a JS perspective; any C++ consumers of the API would still see whatever the C++ does.

Comment 8

[Jonathan Watt [:jwatt]]

If [SameObject] is only documentation then perhaps we can use that even though (as noted by the linked source comment in comment 5) we may return a different object if JS can't tell that that's the case. Either that or it sounds like we should remove [Constant] and replace it with a comment noting why we don't use [SameObject]. Peter/Boris would need to make a call on which of these things we should do.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> If [SameObject] is only documentation 

Well, it's documentation in the sense that it tells consumers (like web authors and our JIT) what to expect.  In particular our JIT _will_ perform optimizations based on assumptions it makes based on [SameObject] annotations.  And per spec, if something is annotated [SameObject] it MUST in fact return the same object each time.  If we're not doing that, and its observable (is it?), then we're not following the spec.

Comment 10

[Jonathan Watt [:jwatt]]

These annotations are only in a draft, not a final spec yet, and I'm a bit surprised to see they've been added (last time I checked we were the only implementation that would return the same object(-ish)). I would take issue with adding these annotations if it would rigidly force our implementation to keep large objects around until the element dies. Often scripts in the wild only look at these type of objects once then never again.

Our current implementation has the tearoffs hold a weak ref. That way the tearoff will return the same object every time unless all scripts have released all references, in which case script isn't going to care if a different object is returned in future. Technically I think the difference is still observable via something like path.pathSegList.toString() (it's not that, but it was something like that) where the address of the object is encoded, but I don't think that technicality should trump the memory wins of being able to eagerly release these objects.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Sounds like we should push back on the spec draft, then.  The problem is coming up with a counterproposal that doesn't make GC explicitly observable.

Comment 12

[Peter Van der Beken [:peterv]]

(In reply to Robert Longson from comment #3)
> What change do I need to make for a strong reference. If you could just show
> me how to do say one case e.g. this one...

You'd have to make nsSVGAttrTearoffTable hold a RefPtr<TearoffType> instead of TearoffType*. Then  you'd have to make everything that holds a SVGAnimatedPreserveAspectRatio traverse the RefPtr<TearoffType> from the CC traversal code (probably easiest to add a Traverse member to SVGAnimatedPreserveAspectRatio which looks up the DOMSVGPreserveAspectRatio in the sBaseSVGPAspectRatioTearoffTable and calls ImplCycleCollectionTraverse on it).

(In reply to Jonathan Watt [:jwatt] from comment #10)
> Our current implementation has the tearoffs hold a weak ref. That way the
> tearoff will return the same object every time unless all scripts have
> released all references, in which case script isn't going to care if a
> different object is returned in future.

It might set expandos on them and care about those.

Comment 13

[Boris Zbarsky [:bzbarsky]]

It might set expandos on them and care about those.

Or use the tearoff as a weakmap key.

Comment 14

[Robert Longson [:longsonr]]

Attachment: 1242048.txt

Assume that GetTearoff2 will eventually be renamed and replace GetTearoff

What do I pass to ImplCycleCollectionTraverse?
What is supposed to call Traverse?
Do I need to change anything else of SVGAnimatedPreserveAspectRatio.cpp or callers of its methods?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9061187 [details] [diff] [review]
1242048.txt

So the goal here is to hold a strong ref to the thing in the tearoff table, effectively?

If so, I think the answers to the questions in comment 14 are:

1) Expand out the NS_SVG_VAL_IMPL_CYCLE_COLLECTION_WRAPPERCACHED macro in SVGAnimatedPreserveAspectRatio.cpp

2) Between NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN and NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END add something like:
```
  DOMSVGPreserveAspectRatio* tearoff = nullptr;
  if (tmp->mIsAnimate) {
    tearoff = sAnimSVGPAspectRatioTearoffTable.GetWeak(tmp); // Or something equivalent that returns the ptr
  } else {
    ...
  }
  if (tearoff) {
    ImplCycleCollectionTraverse(cb, tearoff, "description goes here", 0);
  }
```
This could probably go into a TraverseTearoffs method; then you'd call tmp->TraverseTearoffs(cb) in the CC macro bits or something.

3) In the unlink bits of the expanded macro, remove the hashtable entry.

I think that's it...

Comment 16

[Robert Longson [:longsonr]]

Attachment: updated patch

So the goal here is to hold a strong ref to the thing in the tearoff table,
effectively?

The goal is to have the webidls be able to maintain their [Constant] modifiers that the SVG spec says they should have. I think the consequence for our implementation is what you're saying above though. I.e. the answer to your question is yes.

That gets me to here, although this doesn't compile because the ImplCycleCollectionTraverse functions don't match anything.

a) is it right to have 2 ImplCycleCollectionTraverse calls, i.e. one for DOMSVGAnimatedPreserveAspectRatio and one for DOMSVGPreserveAspectRatio

b) Do the classes have to inherit/implement some other class so that this compiles?

c) if we remove tearoffs from the hashtables in the cycle collector we don't remove in the destructors, right?

Comment 17

[Robert Longson [:longsonr]]

Also regarding 3) I don't need to call ImplCycleCollectionUnlink anywhere do I?

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9061234 [details] [diff] [review]
updated patch

> because the ImplCycleCollectionTraverse functions don't match anything

Er, right, for a raw pointer you just want CycleCollectionNoteChild, not ImplCycleCollectionTraverse.

> a) is it right to have 2 ImplCycleCollectionTraverse calls, i.e. one for DOMSVGAnimatedPreserveAspectRatio and one for DOMSVGPreserveAspectRatio

No, you should just have a ImplCycleCollectionTraverse or CycleCollectionNoteChild call for edges in the ownership graph that you are traversing.  The DOMSVGAnimatedPreserveAspectRatio has an edge to the DOMSVGPreserveAspectRatio but not to itself, right?

> b) Do the classes have to inherit/implement some other class so that this compiles?

I think the compilation issue should be solved with CycleCollectionNoteChild.

> c) if we remove tearoffs from the hashtables in the cycle collector we don't remove in the destructors, right?

It's safer to do it in both.  Unlink will only happen if the object is in fact part of an ownership cycle, and it's not obvious to me whether that's guaranteed to happen 100% of the time here, and will continue to be guaranteed as the code changes.

> Also regarding 3) I don't need to call ImplCycleCollectionUnlink anywhere do I?

Correct.  

You should probably document (or point to the docs in SVGElement.h) why you're traversing but not unlinking mSVGElement.

I'm a little confused with what the story is for sSVGAnimatedPAspectRatioTearoffTable: we're unlinking that but not traversing it?

Comment 19

[Robert Longson [:longsonr]]

Attachment: Hopefully this is what's required. Need to do the other attributes.

Comment 20

[Robert Longson [:longsonr]]

Attachment: Bug 1242048 - Partially implement suggested solution

Comment 21

[Robert Longson [:longsonr]]

This appears to traverse but it doesn't work because all of the newly added tests fail. Have I got something wrong with the test or is there something else I need to do to the code?

Note that If I take out all the CC/GC calls from the mochitest then it passes.

Comment 22

[Robert Longson [:longsonr]]

So far I've figured out how to preserve expandos and leak, or not preserve expandos and not leak. If I addref the pointer being added to the tearoff then the expando is preserved but it leaks even if I release it in the unlink/destructor.

Comment 23

[Robert Longson [:longsonr]]

Attachment: Bug 1242048 - boolean only

This patch doesn't work and I don't know why. In its current state it doesn't leak though.

Comment 24

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 25

[Suhaib Mujahid [:suhaib]]

Sorry, there was a problem with the detection of inactive users. I'm reverting the change.