507067 - SVG: GetAnimVal / GetBaseVal is wrong for em/ex units in display:none

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Test case to demonstrate problem

SVG 1.1, section 4.3 describes the 'value' member of an SVGLength as:

> The value as an floating point value, in user units.

Currently in our implementation, when an SVGAnimatedLength is animated, the value attribute is not in user units but in the specified units.

The attached testcase passes on Opera 10b2. We fail because we seem to return values in em's rather than user units.

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Cross-browser animVal units test

Related to this bug, this test case shows what units are reported by different browsers when requesting the animVal of a currently animated SVGAnimatedLength.

In summary:

 Opera 10b2: user units
 Batik 1.7: the units of the animation function if they agree
 Gecko: the units of the base value
 WebKit: didn't work for me (just returned an odd value in the units of the animation function)

It also tests the behaviour of frozen animation values that use relative values. So the font-size on the parent node is changed once the animation is frozen. The circle should jump to the left (I think). This seems to work in Opera and Gecko but not Batik.

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I looked into this a bit more, it turns out the test is failing because we don't have a frame when we go to get the font-size. We're hitting the warning here:

  http://mxr.mozilla.org/mozilla-central/source/layout/svg/base/src/nsSVGUtils.cpp#272

If I just change the test so that parent <div> containing the SVG is 'visibility: hidden' instead of 'display: none' the test passes.

Comment 3

[Robert Longson [:longsonr]]

SVG's treatment of display: none has always been a pain. See bug 376027, this bug should probably be a dup of that.

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Initial patch

Here is an initial attempt to fix it. With this applied the test case passes.

I'm not sure if this is the right approach. roc talked about moving the GetStyleContextForContext from nsInspectorCSSUtils to nsLayoutUtils. I'm not quite sure how this should look yet.

Also, regarding the patch as it stands, there's a fair bit of duplicated code for getting the style context and pres context in both GetFontSize and GetFontXHeight which could be factored out. There are also lots of other tweaks available too to tidy things up. We have all sorts of overloads in nsSVGLength2 to take either an nsIContent or nsIFrame and it seems like we could cut out a lot of code if we just took an nsIContent but I think the nsIFrame path is necessary because it's faster and is used a lot in rendering which calls nsSVGLength2::GetAnimValue(nsIFrame*). Does that sound right?

Comment 5

[Robert Longson [:longsonr]]

Seems like the right approach. Does this patch fix bug 417019?

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to comment #5)
> Seems like the right approach. Does this patch fix bug 417019?

Seems to. I can repro that bug with FF3.5 but I can't on trunk with this patch applied. I've yet to check a trunk build without this patch however.

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: patch v1b

Updated patch to tidy up code a little and add test case.

I have verified that the test case passes with this test. I also verified that this fixes bug 417019.

Given that bug 417019 is fixed by this, I wonder if this should be marked as a dupe of that bug?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can you move GetContextForContent and nsInspectorCSSUtils::GetStyleContextForContent to nsLayoutUtils? Otherwise looks good.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Although actually, I think GetContextForContent should just return an already_AddRefed<nsStyleContext> (return null on failure). nsSVGUtils::GetFontSize need only take the style context, you can get the prescontext from the style context (and it's only two dereferences).

Comment 11

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: patch v1c

Addressed roc's review comments. 2 quick questions though:

>   * @param aPseudo the pseudo style to query or nsnull.

I'm not sure how to describe this parameter as I don't understand the style system.

>      aPresShell->FlushPendingNotifications(Flush_Style);

You said earlier that we should probably have a version of this that doesn't flush the style system. Is that still necessary, and if so, is it sufficient just to take an extra bool parameter to indicate if we should flush or not? Once again, I don't understand the style system to know how this should work.

Also, I haven't yet run all the reftests with these latest changes.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> >   * @param aPseudo the pseudo style to query or nsnull.
> 
> I'm not sure how to describe this parameter as I don't understand the style
> system.

A pseudo is something like "hover" --- a CSS pseudo-class that matches elements in certain states.

> >      aPresShell->FlushPendingNotifications(Flush_Style);
> 
> You said earlier that we should probably have a version of this that doesn't
> flush the style system. Is that still necessary, and if so, is it sufficient
> just to take an extra bool parameter to indicate if we should flush or not?
> Once again, I don't understand the style system to know how this should work.

I think we probably should have this function not flush, and document that in nsLayoutUtils.h, so that callers are responsible for flushing as necessary. Same for the new SVGUtils methods.

Comment 13

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: patch v1d

Moved flushing out of nsLayoutUtils::GetStyleContextForContent as per comment 12.

I'm not sure which of the call sites should manually perform the flush however. In this patch I have:

nsCanvasRenderingContext2D::SetFont [manual flush, added]
http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/nsCanvasRenderingContext2D.cpp#1931

nsCanvasRenderingContext2D::DrawOrMeasureText [manual flush, added]
http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/nsCanvasRenderingContext2D.cpp#2274

nsComputedDOMStyle::GetPropertyCSSValue [manual flush, already done earlier in the calling method]
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsComputedDOMStyle.cpp#383

nsInspectorCSSUtils::GetRuleNodeForContent [manual flush, added]
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsInspectorCSSUtils.cpp#162

nsSVGUtils::GetFontSize [no flush]
http://mxr.mozilla.org/mozilla-central/source/layout/svg/base/src/nsSVGUtils.cpp#268

nsSVGUtils::GetFontXHeight [no flush]
http://mxr.mozilla.org/mozilla-central/source/layout/svg/base/src/nsSVGUtils.cpp#287

I'm a bit unsure if that's right.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 393104 [details] [diff] [review]
patch v1d

+  nsStyleContext* styleContext = aFrame->GetStyleContext();
+  if (!styleContext)
+      return nsnull;

This can't actually be null.

I guess dbaron should sr here.

Comment 15

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: patch v1e

Removed the two lines from roc's review.

Comment 16

[Brian Birtles (:birtles, Feb 26~ parental leave)]

This one's not quite ready to land after all. I'm getting some test failures on tryserver:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1249625917.1249632866.19857.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1249625917.1249633478.26727.gz

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #9)
> Can you move GetContextForContent and
> nsInspectorCSSUtils::GetStyleContextForContent to nsLayoutUtils? Otherwise
> looks good.

Hmmm... I'd been planning to move it back to nsComputedDOMStyle (which I did yesterday as part of bug 371655).

I guess I'm ok with nsLayoutUtils too; but there's no need for GetStyleContextForFrame to be static method; it can just be a file-scope static.


What's the rationale for GetStyleContextForContent not flushing?

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, nsLayoutUtils.cpp uses 2-space indent; some of the moved code uses 4-space and should be changed.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> What's the rationale for GetStyleContextForContent not flushing?

In SMIL we'll call it a lot and we don't want to flush every time.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #17)
> I guess I'm ok with nsLayoutUtils too; but there's no need for
> GetStyleContextForFrame to be static method; it can just be a file-scope
> static.

Though I think I prefer it in nsComputedDOMStyle; it's implementing something that's really pretty odd, and that we only want when specs ask for behavior specifically related to the computed style.

(In reply to comment #19)
> > What's the rationale for GetStyleContextForContent not flushing?
> 
> In SMIL we'll call it a lot and we don't want to flush every time.

I'm not sure I'm comfortable with that, really; if you don't flush, its behavior is quite inconsistent between content that happens to have a frame and content that doesn't.

I still need to understand better what it's needed for here, though.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #20)
> (In reply to comment #17)
> > I guess I'm ok with nsLayoutUtils too; but there's no need for
> > GetStyleContextForFrame to be static method; it can just be a file-scope
> > static.
> 
> Though I think I prefer it in nsComputedDOMStyle; it's implementing something
> that's really pretty odd, and that we only want when specs ask for behavior
> specifically related to the computed style.

Why do you think it's pretty odd?

> (In reply to comment #19)
> > > What's the rationale for GetStyleContextForContent not flushing?
> > 
> > In SMIL we'll call it a lot and we don't want to flush every time.
> 
> I'm not sure I'm comfortable with that, really; if you don't flush, its
> behavior is quite inconsistent between content that happens to have a frame and
> content that doesn't.

I think of it as being like GetStyleContext for frames, which doesn't flush.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think I convinced roc yesterday that we should leave the flushing in, and only try to optimize if it shows up in profiles, since it makes sense for this method to flush, the only reason for removing it was to pull the flush "out of the loop", and flushing when there isn't anything to flush ought to be cheap enough compared to the rest of what GetStyleContextForContent does.

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, if you merge to trunk and leave GetStyleContextForContent in nsComputedDOMStyle (where it now is), this patch will get a good bit smaller.  Any chance you could do that and then re-request review?

Comment 24

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: patch v1h

Address review comments:
* Leave GetStyleContextForContent in nsComputedDOMStyle

Comment 25

[Brian Birtles (:birtles, Feb 26~ parental leave)]

This patch causes a crash for the crashtest for bug 386566.

http://mxr.mozilla.org/mozilla-central/source/layout/svg/crashtests/386566-1.svg

I don't really understand all this but judging by the comments on bug 386566 and  bug 386475 it appears to be related to the interaction with resolving em/ex units and flushing layout.

Stacktrace:
 gklayout.dll!nsCachedStyleData::GetStyleSVG()  Line 149 + 0xc bytes
 gklayout.dll!nsStyleContext::GetStyleSVG()  Line 149 + 0x14 bytes
 gklayout.dll!nsIFrame::GetStyleSVG()  Line 149 + 0x3e bytes
 gklayout.dll!nsSVGGeometryFrame::HasStroke(gfxContext * aContext=0x0012e97c)  Line 239 + 0x8 bytes
 gklayout.dll!nsSVGPathGeometryFrame::UpdateCoveredRegion()  Line 269 + 0xf bytes
 gklayout.dll!nsSVGOuterSVGFrame::UpdateAndInvalidateCoveredRegion(nsIFrame * aFrame=0x05f79dec)  Line 647
 gklayout.dll!nsSVGUtils::UpdateGraphic(nsISVGChildFrame * aSVGFrame=0x05f79e1c)  Line 698 + 0xc bytes
 gklayout.dll!nsSVGPathGeometryFrame::NotifyRedrawUnsuspended()  Line 323 + 0x20 bytes
 gklayout.dll!nsSVGOuterSVGFrame::UnsuspendRedraw()  Line 702
 ...

Comment 26

[Robert Longson [:longsonr]]

The call stack in the crash case is nsSVGRectFrame calls nsSVGRectElement to get the covered region. If this flushes it may destroy the nsSVGRectFrame and when the call stack unwinds it crashes.

nsSVGUtils::GetFontSize(nsIContent *aContent)
cannot call anything that flushes. 

roc must try harder in comment 22 ;-)

Comment 27

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Thanks Robert. I think I just arrived at the same conclusion (except that it
took me an hour to get there).

For my own sake, here's the stacktrace of where the frame is being reconstructed.

 gklayout.dll!nsCSSFrameConstructor::RecreateFramesForContent(nsIContent * aContent=0x061f0e58, int aAsyncInsert=0)  Line 9091
 gklayout.dll!nsCSSFrameConstructor::ProcessRestyledFrames(nsStyleChangeList & aChangeList={...})  Line 7762
 gklayout.dll!nsCSSFrameConstructor::RestyleElement(nsIContent * aContent=0x061f0e58, nsIFrame * aPrimaryFrame=0x061ece7c, nsChangeHint aMinHint=0)  Line 7846
 gklayout.dll!nsCSSFrameConstructor::ProcessOneRestyle(nsIContent * aContent=0x061f0e58, nsReStyleHint aRestyleHint=eReStyle_Self, nsChangeHint aChangeHint=0)  Line 11531
 gklayout.dll!nsCSSFrameConstructor::ProcessPendingRestyles()  Line 11640
 gklayout.dll!PresShell::FlushPendingNotifications(mozFlushType aType=Flush_Style)  Line 4851
 gklayout.dll!nsComputedDOMStyle::GetStyleContextForContent(nsIContent * aContent=0x061f0e58, nsIAtom * aPseudo=0x00000000, nsIPresShell * aPresShell=0x061c83d8)  Line 365
 gklayout.dll!nsSVGUtils::GetFontSize(nsIContent * aContent=0x061f0e58)  Line 272 + 0x11 bytes
 gklayout.dll!nsSVGLength2::GetEmLength(nsSVGElement * aSVGElement=0x061f0e58)  Line 126 + 0xc bytes
 gklayout.dll!nsSVGLength2::GetUnitScaleFactor(nsSVGElement * aSVGElement=0x061f0e58, unsigned char aUnitType='')  Line 251 + 0x9 bytes
 gklayout.dll!nsSVGLength2::GetAnimValue(nsSVGElement * aSVGElement=0x061f0e58)  Line 82 + 0x14 bytes
 gklayout.dll!nsSVGElement::GetAnimatedLengthValues(float * aFirst=0x0012e868, ...)  Line 1238 + 0x21 bytes
 gklayout.dll!nsSVGRectElement::ConstructPath(gfxContext * aCtx=0x0012e97c)  Line 174 + 0x26 bytes
 gklayout.dll!nsSVGPathGeometryFrame::GeneratePath(gfxContext * aContext=0x0012e97c, const gfxMatrix * aOverrideTransform=0x00000000)  Line 490
 gklayout.dll!nsSVGPathGeometryFrame::UpdateCoveredRegion()  Line 253
 ...

When we get back to UpdateCoveredRegion the frame is invalid.

Comment 28

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: patch v1i

Updated to add version of GetStyleContextForContent that doesn't flush.

roc: Not sure if you wanted me to carry your review over from last time or if you wanted to have another look at these changes.

I've just uploaded to TryServer. Will check later for any other failing tests.

Comment 29

[Brian Birtles (:birtles, Feb 26~ parental leave)]

All clear this time on TryServer.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 394227 [details] [diff] [review]
patch v1i

>+      nsIDocument* currentDoc = aContent->GetCurrentDoc();
>+      if (!currentDoc)
>+        return nsnull;
>+
>+      nsIPresShell *presShell = currentDoc->GetPrimaryShell();
>+      if (!presShell)
>+        return nsnull;
>+
>+      aPresShell = presShell;

Could you factor this (twice) into a:

static nsIPresShell*
GetPresShellForContent(nsIContent *aContent)
{
  ...
}

and
  aPresShell = GetPresShellForContent(aContent);

>diff --git a/layout/svg/base/src/nsSVGUtils.cpp b/layout/svg/base/src/nsSVGUtils.cpp

>+float
>+nsSVGUtils::GetFontSize(nsStyleContext *aStyleContext)
>+{
>+  if (!aStyleContext) {
>+    NS_WARNING("NULL style context in GetFontSize");
>+    return 1.0f;
>+  }
>+

You don't need to check aStyleContext for being null at runtime; one caller has a frame, which can't be constructed without a style context, and the other caller already null-checked.  So this should just be:

NS_ABORT_IF_FALSE(aStyleContext, "null style context");

>+  nsPresContext *presContext = aStyleContext->PresContext();
>+  if (!presContext) {
>+    NS_WARNING("NULL pres context in GetFontSize");
>+    return 1.0f;
>+  }

Same here; it's not possible to create a style context without a pres context; that's what the lack of "Get" on the getter tells you.  So this should just be

NS_ABORT_IF_FALSE(presContext, "null pres context");


>+float
>+nsSVGUtils::GetFontXHeight(nsStyleContext *aStyleContext)
>+{

Same two comments as above.

sr=dbaron with that

Comment 31

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: patch v1j (address dbaron's sr comments)

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'll land this later today.

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/mozilla-central/rev/ae3a432b4653

Comment 34

[Robert Longson [:longsonr]]

Comment on attachment 395293 [details] [diff] [review]
patch v1j (address dbaron's sr comments)

Has baked for a while. Fixes issues with use getting the wrong sizes when printing. Includes tests.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't think this really needs to be in 1.9.2, it's a pretty obscure bug.