1086284 - Painting SVG-as-image causes AddRef/Release churn

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

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

Things under SVGDocumentWrapper::GetWidthOrHeight (Width/Heigth/AnimVal) ends up Addrefing/Releasing svg element during paintlist creation. (At least) that causes the svg-as-image elements to show up in the CC graph all the time.

Comment 1

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

If my testing is right, fixing this would indeed hide svg-as-image documents from CC graph, and
cut down the base CC graph size (when there is just one tab open) about 50%.

(In my test I just made the method to return some dummy value in aResult.)

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8532785 [details] [diff] [review]
patch

># User Jonathan Watt <[e-mail]>
>Bug 1086284 - Avoid using COM when determining an SVG-as-an-image's intrinsic size so that it doesn't end up in the GC graph. r=dholbert

s/COM/refcounting/ (I don't think any nsCOMPtrs are involved here -- only nsRefPtrs)

Also, s/GC graph/CC graph/

>diff --git a/dom/svg/SVGSVGElement.cpp b/dom/svg/SVGSVGElement.cpp
>+float
>+SVGSVGElement::GetIntrinsicWidth() const
>+{
>+  if (mLengthAttributes[ATTR_WIDTH].GetSpecifiedUnitType() ==
>+        nsIDOMSVGLength::SVG_LENGTHTYPE_PERCENTAGE) {
>+    return UnspecifiedNaN<float>();
>+  }

This can be condensed to:
 if (mLengthAttributes[ATTR_WIDTH].IsPercentage()) {

Same in GetIntrinsicHeight.

>+  // context is only needed for percentage resolution
>+  SVGSVGElement* unused = nullptr;
>+  return std::max(mLengthAttributes[ATTR_WIDTH].GetAnimValue(unused), 0.f);
>+}

Three things:
(1) Clarify comment to:
  // Context is only needed for percentage resolution. We already know we
  // don't have a percentage, so no context is needed; hence, nullptr.

(2) I'm not sure why you're splitting out "unused = nullptr" into a variable -- you could just directly pass nullptr, right? Presumably you're using a variable to be more explicit about what the "nullptr" represents?

If so, probably better to name it "context" instead of "unused". ("unused" is no clearer than "nullptr", IMO -- "context = nullptr; foo(context);" seems much clearer than "unused = nullptr; foo(unused)" [with the comment explaining why we pass a null context])

(3) Just an observation: the zero-clamping here (with std::max) *does* represent a behavior-change from what our code currently does here, but I think it's a good change. Up until now, we've considered SVG Images as being able to have e.g. negative intrinsic widths, and client code has just been responsible for handling that, but from a quick GDB session, it looks like client code does not handle it. (e.g. nsImageLoadingContent::GetNaturalWidth foolishly converts the returned int32_t into an uint32_t, without any range-checking)  So, it's probably good to just treat negative sizes as 0.

>+++ b/dom/svg/SVGSVGElement.h
>@@ -253,16 +253,19 @@ public:
>   already_AddRefed<SVGTransform> CreateSVGTransform();
>   already_AddRefed<SVGTransform> CreateSVGTransformFromMatrix(SVGMatrix& matrix);
>   using nsINode::GetElementById; // This does what we want
>   already_AddRefed<SVGAnimatedRect> ViewBox();
>   already_AddRefed<DOMSVGAnimatedPreserveAspectRatio> PreserveAspectRatio();
>   uint16_t ZoomAndPan();
>   void SetZoomAndPan(uint16_t aZoomAndPan, ErrorResult& rv);
> 
>+  float GetIntrinsicWidth() const;
>+  float GetIntrinsicHeight() const;
>+

Move these up higher. (You're adding these at the bottom of the "// WebIDL" section, which is not the right place for them. They belong higher up, probably right above "// WebIDL", or at least somewhere in the "public helpers" section above that.)

Also, please add a comment for these -- since they're public methods exposed to everything that knows about SVGSVGElement. (mentioning the behavior w/ percent values, in particular)

>+++ b/image/src/SVGDocumentWrapper.cpp
>@@ -71,44 +72,24 @@ SVGDocumentWrapper::DestroyViewer()
> bool
> SVGDocumentWrapper::GetWidthOrHeight(Dimension aDimension,
>                                      int32_t& aResult)
> {
[...]
>+  float length = (aDimension == eWidth) ? rootElem->GetIntrinsicWidth()
>+                                        : rootElem->GetIntrinsicHeight();

Optional: Since you're making GetWidthOrHeight much shorter now, I wonder if we should just do away with it entirely, and fold it into VectorImage::GetWidth() and VectorImage::GetHeight()?  (That would remove the need for this ternary condition here, for example.)  This could be in a followup bug, too.

r=me with the above addressed.

Comment 4

[Robert Longson [:longsonr]]

FWIW Regarding 2) there are two pointer overloadings of GetAnimValue nsSVGELement* and nsIFrame* so nullptr wouldn't compile.

Comment 5

[Daniel Holbert [:dholbert]]

Ah, thanks -- that part of the patch makes a bit more sense, given that. (I do still think it'd be a bit more declarative/readable to name it "context" instead of "unused", but not a big deal.)

Comment 6

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

Comment on attachment 8532785 [details] [diff] [review]
patch

This does remove all the svg-img documents we use in FF UI from the CC graph.
Thanks.

Comment 7

[Jonathan Watt [:jwatt]]

Comment on attachment 8532785 [details] [diff] [review]
patch

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

Comment 8

[Jonathan Watt [:jwatt]]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> Optional: Since you're making GetWidthOrHeight much shorter now, I wonder if
> we should just do away with it entirely, and fold it into
> VectorImage::GetWidth() and VectorImage::GetHeight()?  (That would remove
> the need for this ternary condition here, for example.)  This could be in a
> followup bug, too.

Bug 1112533.

Comment 9

[Carsten Book [:Tomcat]]

sorry back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=4718023&repo=mozilla-inbound

Comment 10

[Daniel Holbert [:dholbert]]

The failures look like we're resolving the "height" & "width" attributes on <svg> to 0 (in an image), (for the purposes of image intrinsic size determination) when they have "em" units.  This is probably because the 'context' pointer is used for "em" resolution *as well as* percent resolution.

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8532785 [details] [diff] [review]
patch

>+float
>+SVGSVGElement::GetIntrinsicWidth() const
>+{
>+  if (mLengthAttributes[ATTR_WIDTH].GetSpecifiedUnitType() ==
>+        nsIDOMSVGLength::SVG_LENGTHTYPE_PERCENTAGE) {
>+    return UnspecifiedNaN<float>();
>+  }
>+  // context is only needed for percentage resolution
>+  SVGSVGElement* unused = nullptr;
>+  return std::max(mLengthAttributes[ATTR_WIDTH].GetAnimValue(unused), 0.f);

Can't we just pass in "this" as the context-pointer here?

Comment 12

[Jonathan Watt [:jwatt]]

Yeah, turns out we can. I added a comment to the source to note why that's okay.

https://hg.mozilla.org/integration/mozilla-inbound/rev/24dbc8542602

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/24dbc8542602