1180048 - ~5,000 instances of ' WARNING: zero axis length' emitted from dom/svg/nsSVGLength2.cpp on debug builds

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Eric Rahm [:erahm]]

>  5023 [NNNNN] WARNING: zero axis length: file dom/svg/nsSVGLength2.cpp, line 124

This is the #5 most verbose warning [1] during linux64 debug testing. I'm not sure it's particularly actionable (in the people who care about it probably aren't running debug builds sense), perhaps we should remove it or convert to some sort of user facing warning?

Devtools is the worst offender, followed by reftests:
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-2-bm114-tests1-linux64-build2.txt:3408
> mozilla-central_ubuntu64_vm-debug_test-reftest-e10s-2-bm120-tests1-linux64-build18.txt:328
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-1-bm113-tests1-linux64-build16.txt:328
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-3-bm67-tests1-linux64-build24.txt:320
> mozilla-central_ubuntu64_vm-debug_test-reftest-4-bm67-tests1-linux64-build13.txt:285
> mozilla-central_ubuntu64_vm-debug_test-reftest-e10s-1-bm51-tests1-linux64-build7.txt:152
> mozilla-central_ubuntu64_vm-debug_test-reftest-2-bm121-tests1-linux64-build9.txt:96
> mozilla-central_ubuntu64_vm-debug_test-reftest-1-bm51-tests1-linux64-build3.txt:56
> mozilla-central_ubuntu64_vm-debug_test-reftest-3-bm52-tests1-linux64-build3.txt:42
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-4-bm121-tests1-linux64-build7.txt:4
> mozilla-central_ubuntu64_vm-debug_test-crashtest-bm52-tests1-linux64-build5.txt:4

The most verbose tests are under |browser/devtools/animationinspector/test/| and |reftest/tests/layout/reftests/svg/|

[1] https://hg.mozilla.org/mozilla-central/annotate/f5e3bacfb60e/dom/svg/nsSVGLength2.cpp#l124

Comment 1

[Patrick Brosset <:pbro>]

What's the best way to investigate what SVG could be causing this in the devtools code?
The devtools toolbox UI makes use of a lot of SVG icons, could this be related?
The other SVG-related thing that comes to mind is the devtools "highlighter" (the thing that's drawn on top of elements in the page when you use the inspector) which also uses SVG.

Comment 2

[Robert Longson [:longsonr]]

This generally means that you're asking the system to size something as a percentage when its container has a size of zero.

Comment 3

[Cameron McCormack (:heycam)]

I tried doing a try run that logs the document URI for the element involved:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=da1407affac5

In the devtools logs it's mostly reporting chrome://browser/content/devtools/animationinspector/animation-inspector.xhtml which granted doesn't narrow things down much.

Comment 4

[Patrick Brosset <:pbro>]

Could be related to this SVG file:
https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/images/animation-fast-track.svg

Applied from this CSS file:
https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/animationinspector.css#317

Comment 5

[Robert Longson [:longsonr]]

Seems unlikely there's no percentages in the animation-fast-track.svg file.

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I took some time looking into this.

Simple STR:
1. Open about:home
2. Right-click the globe and inspect element
3. Select the animations side panel

The error is not logged when a panel other than the animation panel is selected.

The error is still logged:
- If animation-fast-track.svg is removed even though it is the *only* SVG in the panel.
- If no players are displayed and only the message "No animations were found for the current element" is shown.

It seems that whenever that particular panel is populated the warnings are shown.

Comment 7

[Eric Rahm [:erahm]]

This is now the #2 most verbose warning during testing.

Comment 8

[Cameron McCormack (:heycam)]

(In reply to Robert Longson from comment #2)
> This generally means that you're asking the system to size something as a
> percentage when its container has a size of zero.

Is this a legitimate thing to do (i.e., if we size some SVG image to zero width or height, should we be going down this path where we do try to resolve these percentage values), and is using a very small value in place of zero the right thing to do in response (which is what the code is doing now)?  If so, let's just remove the warning.

Comment 9

[Eric Rahm [:erahm]]

Robert, any thoughts re: comment 8?

Comment 10

[Robert Longson [:longsonr]]

We put in  very small value because the caller uses it as a denominator.

Comment 11

[Eric Rahm [:erahm]]

This is now #1. As per comment 8, Robert's confirmation that internally we do the right thing, lets just remove the warning.

Comment 12

[Eric Rahm [:erahm]]

Attachment: Stop warning about having a zero length axis

Comment 13

[Daniel Holbert [:dholbert]]

Looked into this a bit in GDB, to be sure it's not actually bringing up something that should concern us.

I found out the following:
 (1) The element that's being filtered is a generated-content node, for "#element-picker::before" in animation-inspector.xhtml. (generated from this CSS rule ::before http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/animationinspector.css?rev=06d8b833c6a2#76 )

 (2) The filter that's being applied is filter.svg, "invert", from here:
http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/filters.svg?raw=1
 This filter uses feComponentTransfer to invert colors.

 (3) I don't immediately see why this filter is being applied to this particular element, but it is. (And it seems like it's expected to be applied, based on a animationinspector.css rule that sets "filter" to none to turn off the filter when #element-picker is checked).

 (4) We end up tripping over this warning (in "FixAxisLength") when we're inside of  UserSpaceMetricsWithSize::GetAxisLength, and we trip over the warning with a 0-sized "size" variable.

 (5) The "size" is 0 because we're at the end of our first reflow, and our ::before-frame's mRect still has its initial 0,0 size.  In GetAxisLength(), our call to GetSize() (on NonSVGFrameUserSpaceMetrics) ends up calling  nsSVGIntegrationUtils::GetSVGCoordContextForNonSVGFrame, which returns our 0,0 frame size.

 (6) This is all happening during our call to ComputeEffectsRect, in nsIFrame::FinishAndStoreOverflow, at the end of the ::before frame's first reflow. So we end up incorrectly thinking we have a 0-sized effects rect.

 (7) ComputeEffectsRect actually takes our "aNewSize" as a parameter, but we don't use it because the filter doesn't have aUnits == SVG_UNIT_TYPE_OBJECTBOUNDINGBOX. (in nsSVGUtils::GetRelativeRect)

My takeaways:
 - The warning is probably mostly-harmless.
 - Our ComputeEffectsRect function may be returning a technically-bogus value here (since it's using an about-to-be-out-of-date frame rect-size, noted in (5) above.)
 - We should consider making this filter use primitiveUnits="objectBoundingBox" to work around all of this.

Comment 14

[Daniel Holbert [:dholbert]]

(Sorry, I meant "filters.svg", not "filter.svg", in comment 13. The MXR link is correct; my line before that URL is missing an "s".)

Comment 15

[Daniel Holbert [:dholbert]]

I filed bug 1189024 with a patch to use objectBoundingBox-sizing in devtools, to avoid this issue there. I'll bet that'll remove ~80% of the instances of this warning, based on the counts in comment 0.

With that, I'd tentatively lean against removing the warning, since it does provide a useful signal that something might be wrong. (or at least to keep it around with LAYOUT_WARNING(...))

Comment 16

[Eric Rahm [:erahm]]

(In reply to Daniel Holbert [:dholbert] from comment #15)
> With that, I'd tentatively lean against removing the warning, since it does
> provide a useful signal that something might be wrong. (or at least to keep
> it around with LAYOUT_WARNING(...))

Given the target audience (devtools devs, web devs) in this case, I'd say a debug-only, console-only warning doesn't make much sense. I can see how as a gecko dev working on layout it might be useful, so LAYOUT_WARNING is probably the way to go.

Comment 17

[Eric Rahm [:erahm]]

Attachment: Switch warning about having a zero length axis to a LAYOUT_WARNING

Switching to LAYOUT_WARNING reduces the verbosity of the zero length axis
warning during testing, but still allow opt-in logging.

Comment 18

[Robert Longson [:longsonr]]

Maybe we should check the frame state in nsSVGIntegrationUtils::GetSVGCoordContextForNonSVGFrame (check the first reflow bit) and return an empty rect at that point. That would be better wouldn't it?

Then we leave the warning (or ideally make it an assert) because if you suppress it, nobody will ever fix it.

Comment 19

[Daniel Holbert [:dholbert]]

(In reply to Robert Longson from comment #18)
> Maybe we should check the frame state in
> nsSVGIntegrationUtils::GetSVGCoordContextForNonSVGFrame (check the first
> reflow bit) and return an empty rect at that point. That would be better
> wouldn't it?

That would avoid this warning, but there's still the general problem that we'll be using a rect that's from *before* the current reflow, to set our overflow area *on* the current reflow. That's the more fundamental issue here.

> Then we leave the warning (or ideally make it an assert)
We can't make it an assertion, because we can legitimately hit this code-path, if we're *actually* applying a filter to a 0-size element.

> if you suppress it, nobody will ever fix it.

That's the theory; but part of the reason erahm's on his current NS_WARNING-killing-rampage is that we're *already* in a state where nobody ever fixes them, for many extremely spammy warnings. :)  So they cost us lots of noise in our logs [obscuring other potentially-important issues], while giving the tiny benefit that someone *might* notice them and investigate to see if something's wrong.  But no one ever does, in many cases.

LAYOUT_WARNING() seems like a good compromise; it lets layout developers opt-in to seeing this maybe-but-not-nessarily-worrisome warning (and take action as-needed/as-desired), while preventing it from filling up the terminals/logs of other folks.

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 8640699 [details] [diff] [review]
Switch warning about having a zero length axis to a LAYOUT_WARNING

># Parent  bc589dd18ad57ab24bd70070855c4c9568796cc5
>Bug 1180048 - Switch warning about having a zero length axis to a LAYOUT_WARNING. r=dholbert
>
>Switching to LAYOUT_WARNING reduces the verbosity of the zero length axis
>warning during testing, but still allow opt-in logging.

s/reduces the verbosity of/hides/?

"reduces the verbosity" sounds like we make it *less* spammy during testing, somehow. But really we're completely hiding it.

r=me with that clarified (or with the bonus commit-message lines just removed)

Thanks!

Comment 21

[Daniel Holbert [:dholbert]]

(I also realized that I don't think we ever really announced LAYOUT_WARNING() existence, or how to opt-in to seeing it, so I've done that here:
 https://groups.google.com/forum/#!topic/mozilla.dev.tech.layout/xsjIrxt9NLM
)

Comment 22

[Daniel Holbert [:dholbert]]

(er, here: https://groups.google.com/d/msg/mozilla.dev.tech.layout/xsjIrxt9NLM/aEQtVYaRBQAJ )

Comment 23

[Eric Rahm [:erahm]]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/4a09252567e2e07f599665c0ddcd718c9dc18656
changeset:  4a09252567e2e07f599665c0ddcd718c9dc18656
user:       Eric Rahm <[e-mail]>
date:       Wed Jul 29 16:06:14 2015 -0700
description:
Bug 1180048 - Switch warning about having a zero length axis to a LAYOUT_WARNING. r=dholbert

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4a09252567e2