Closed Bug 1180048 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: erahm, Assigned: erahm)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

>  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
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.
This generally means that you're asking the system to size something as a percentage when its container has a size of zero.
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.
Seems unlikely there's no percentages in the animation-fast-track.svg file.
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.
Summary: ~5,000 instances of ' WARNING: zero axis length' emitted from dom/svg/nsSVGLength2.cpp during linux64 debug testing → ~5,000 instances of ' WARNING: zero axis length' emitted from dom/svg/nsSVGLength2.cpp on debug builds
This is now the #2 most verbose warning during testing.
(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.
Robert, any thoughts re: comment 8?
Flags: needinfo?(longsonr)
We put in  very small value because the caller uses it as a denominator.
Flags: needinfo?(longsonr)
This is now #1. As per comment 8, Robert's confirmation that internally we do the right thing, lets just remove the warning.
Attachment #8639980 - Flags: review?(longsonr)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
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.
(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".)
Depends on: 1189024
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(...))
(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.
Switching to LAYOUT_WARNING reduces the verbosity of the zero length axis
warning during testing, but still allow opt-in logging.
Attachment #8640699 - Flags: review?(dholbert)
Attachment #8639980 - Attachment is obsolete: true
Attachment #8639980 - Flags: review?(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.
(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 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!
Attachment #8640699 - Flags: review?(dholbert) → review+
(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
)
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/4a09252567e2e07f599665c0ddcd718c9dc18656
changeset:  4a09252567e2e07f599665c0ddcd718c9dc18656
user:       Eric Rahm <erahm@mozilla.com>
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
https://hg.mozilla.org/mozilla-central/rev/4a09252567e2
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.