Painting SVG-as-image causes AddRef/Release churn

RESOLVED FIXED in mozilla38

Status

()

Core
SVG
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: smaug, Assigned: jwatt)

Tracking

Trunk
mozilla38
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

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

Comment 1

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

Updated

4 years ago
Blocks: 741760
(Assignee)

Updated

4 years ago
OS: Linux → All
Hardware: x86_64 → All
Version: 34 Branch → Trunk
(Assignee)

Comment 2

4 years ago
Created attachment 8532785 [details] [diff] [review]
patch
Attachment #8532785 - Flags: review?(dholbert)
Comment on attachment 8532785 [details] [diff] [review]
patch

># User Jonathan Watt <jwatt@jwatt.org>
>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.
Attachment #8532785 - Flags: review?(dholbert) → review+
FWIW Regarding 2) there are two pointer overloadings of GetAnimValue nsSVGELement* and nsIFrame* so nullptr wouldn't compile.
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.)
(Reporter)

Comment 6

4 years ago
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.
Attachment #8532785 - Flags: feedback+
(Assignee)

Comment 8

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

Comment 12

3 years ago
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
https://hg.mozilla.org/mozilla-central/rev/24dbc8542602
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.