782888 - assertion "Shouldn't be trying to restyle non-elements directly" when setting presentation attribute on SVG text element

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Cameron McCormack (:heycam)]

Attachment: test

With the attached document:

###!!! ASSERTION: Shouldn't be trying to restyle non-elements directly: '!aContent || aContent->IsElement()', file /Volumes/Data/moz/central/layout/base/nsStyleChangeList.cpp, line 65
###!!! ASSERTION: aFrame's content should be an element: 'aFrame->GetContent()->IsElement()', file /Volumes/Data/moz/central/layout/svg/base/src/nsSVGEffects.cpp, line 480

The assertions don't appear if fill is not set to a url() value.

Comment 1

[Cameron McCormack (:heycam)]

We get to:

http://hg.mozilla.org/mozilla-central/file/tip/layout/base/nsFrameManager.cpp#l1287

with aFrame being the nsSVGGlyphFrame and content being the nsTextNode.  Then in CaptureChange we reach the NS_ASSERTION call since we've got an nsTextNode and not an Element.  CalcStyleDifference returns (NS_STYLE_HINT_REFLOW | nsChangeHint_UpdateEffects) in this case.  Then the NS_UpdateHint call at line 986 returns true because aMinChange is 0 and we added some bits.

But if we don't have a url() value involved, then when we still do get into CaptureChange with aFrame being the nsSVGGlyphFrame and aContent being the nsTextNode, but we don't get to

http://hg.mozilla.org/mozilla-central/file/tip/layout/base/nsFrameManager.cpp#l988

because the NS_UpdateHint call is NS_UpdateHint(aMinChange, nsChangeHint_RepaintFrame), and aMinChange is already nsChangeHint_RepaintFrame.

Boris, could you help me interpret what this all means?

Comment 2

[Robert Longson [:longsonr]]

Was this caused by bug 775304? Worth trying backing that out locally to see whether it fixes this.

If nsChangeHint_UpdateEffects is a non-inherited hint then it seems to me that ReResolveStyleContext will traverse the text children of the nsSVGTextFrame when it previously would not.

Comment 3

[Robert Longson [:longsonr]]

I could be way-off here as the non-inherited thing seems to suggest that it shouldn't traverse children.

Comment 4

[Robert Longson [:longsonr]]

I tried backing out bug 775304 but it had no effect.

Comment 5

[Robert Longson [:longsonr]]

Sorry I got that completely wrong. Backing out bug 775304 fixes this issue.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> with aFrame being the nsSVGGlyphFrame and content being the nsTextNode.

That's fine so far.

> CalcStyleDifference returns (NS_STYLE_HINT_REFLOW | nsChangeHint_UpdateEffects)

OK, how did that happen if aMinChange is 0?

We're talking about style for a text node.  There can't be any rules matching it, so it has initial values of all reset properties, and the same values as its parent element for inherited properties.  Which means we should have computed a change of "(NS_STYLE_HINT_REFLOW | nsChangeHint_UpdateEffects)" for the parent and passed that along as aMinChange.  I know there's weirdness around UpdateEffects, but the reflow part really should have worked correctly.

What you see without url() value is how this should normally work.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I just stepped through this, and I don't see any NS_STYLE_HINT_REFLOW.  Just nsChangeHint_UpdateEffects.  That would be due to us going from a paint server to a color for fill, which of course inherits to the text(!).

Comment 8

[Boris Zbarsky [:bzbarsky]]

So the point is that fill inherits, right?  But it doesn't actually apply to text, does it?

I wonder whether it would make sense to force it to none on textnodes or something....

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

We're about a week away from merging 17 to the Beta channel and since SVG is increasingly important as we move forward with Web Apps it's time this issue gets an assignee if it's going to be tracked for 17's release.  Starting with Boris, if you can't get to this please assign to a suitable replacement.

Comment 10

[Robert Longson [:longsonr]]

I think we just want to back out bug 775304.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Certainly for branches that's what we want to do.  See also bug 788919.

Jonathan, do you want to do it, or should I?

Comment 12

[Jonathan Watt [:jwatt]]

I should do it.

Comment 13

[Jonathan Watt [:jwatt]]

Attachment: backout bug 775304

Comment 14

[bhavana bajaj [:bajaj]]

Comment on attachment 665953 [details] [diff] [review]
backout bug 775304

Approving for backout in aurora based on comment 9

Comment 15

[Jonathan Watt [:jwatt]]

Pushed https://hg.mozilla.org/releases/mozilla-aurora/rev/1697327817af but then had to back it out again because reftests/svg/smil/anim-filter-primitive-size-01.svg fails for some reason.

Comment 16

[Cameron McCormack (:heycam)]

Forcing fill to none on text nodes would break some of my bug 655877 changes, where simple enough nsTextFrames get their foreground color from the fill property.

Is the issue here that fill and stroke changes can result in eChangeHint_UpdateEffects, and this hint is one of the bits in nsChangeHint_Hints_NotHandledForDescendants?  And that there are no other inherited properties that cause inherited change hints?  Do we need to have a different mechanism to update the nsSVGEffects::{Fill,Stroke}Property frame properties?

Or what about just not propagating eChangeHint_UpdateEffects into text nodes?  With something like:

  if ((ourChange & eChangeHint_UpdateEffects) &&
      aContent->IsNodeOfType(eTEXT)) {
    ourChange = ourChange & ~eChangeHint_UpdateEffects;
  }

just after the call to CalcStyleDifference in CaptureChange.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> Is the issue here that fill and stroke changes can result in eChangeHint_UpdateEffects,
> and this hint is one of the bits in nsChangeHint_Hints_NotHandledForDescendants? 

Yes.

> Or what about just not propagating eChangeHint_UpdateEffects into text nodes?

I could live with that, I guess.

Comment 18

[Cameron McCormack (:heycam)]

Stealing.

Comment 19

[Cameron McCormack (:heycam)]

Attachment: patch

Here's a patch to the comment 16 suggestion.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 775304
User impact if declined: I don't think the assertion failure causes any issues.
Testing completed (on m-c, etc.): Try run in progress https://tbpl.mozilla.org/?tree=Try&rev=2385a0b0ccc8
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.

Comment 20

[Cameron McCormack (:heycam)]

Aurora try run: https://tbpl.mozilla.org/?tree=Try&rev=1f0337ada249

Comment 21

[Boris Zbarsky [:bzbarsky]]

> I don't think the assertion failure causes any issues.

You're wrong. See stacks in bug 788919 where we end up calling AsElement() on a textnode then passing the result to selector-matching (in an opt build; in a debug build we fail a fatal assert on the AsElement() call).

Comment 22

[Boris Zbarsky [:bzbarsky]]

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

I'd prefer we test "aContent && !aContent->IsElement()".  r=me with that.

Comment 23

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky (:bz) from comment #21)
> You're wrong. See stacks in bug 788919 where we end up calling AsElement()
> on a textnode then passing the result to selector-matching (in an opt build;
> in a debug build we fail a fatal assert on the AsElement() call).

Ouch, OK.

(In reply to Boris Zbarsky (:bz) from comment #22)
> I'd prefer we test "aContent && !aContent->IsElement()".  r=me with that.

Sure, thanks.

Comment 24

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f40ba56870b

Comment 25

[Jonathan Watt [:jwatt]]

Thanks, Cameron.

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7f40ba56870b

Comment 27

[Alex Keybl [:akeybl]]

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

[Triage Comment]
Given comment 21, approving for Aurora 17. Please land early Monday to make the next merge.

Comment 28

[Phil Ringnalda (:philor)]

Pretty sure the November 5 of "away until November 5" comes after early Monday, so I massaged it into applying to aurora and landed it in https://hg.mozilla.org/releases/mozilla-aurora/rev/fb1d0219a055