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

RESOLVED FIXED in Firefox 17

Status

()

Core
SVG
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

({regression})

Trunk
mozilla18
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox17+ fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

5 years ago
Created attachment 652032 [details]
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.
(Assignee)

Comment 1

5 years ago
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

5 years ago
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

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

Comment 4

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

Comment 5

5 years ago
Sorry I got that completely wrong. Backing out bug 775304 fixes this issue.
Blocks: 775304

Updated

5 years ago
Keywords: regression
> 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.
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(!).
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....
tracking-firefox17: --- → ?
Blocks: 788919
tracking-firefox17: ? → +
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.
Assignee: nobody → bzbarsky

Comment 10

5 years ago
I think we just want to back out bug 775304.
Certainly for branches that's what we want to do.  See also bug 788919.

Jonathan, do you want to do it, or should I?
I should do it.
Created attachment 665953 [details] [diff] [review]
backout bug 775304
Attachment #665953 - Flags: approval-mozilla-aurora?
Assignee: bzbarsky → jwatt
Comment on attachment 665953 [details] [diff] [review]
backout bug 775304

Approving for backout in aurora based on comment 9
Attachment #665953 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
(Assignee)

Comment 16

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

Comment 18

5 years ago
Stealing.
Assignee: jwatt → cam
Status: NEW → ASSIGNED
(Assignee)

Comment 19

5 years ago
Created attachment 668284 [details] [diff] [review]
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.
Attachment #668284 - Flags: review?(bzbarsky)
Attachment #668284 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 20

5 years ago
Aurora try run: https://tbpl.mozilla.org/?tree=Try&rev=1f0337ada249
> 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 on attachment 668284 [details] [diff] [review]
patch

I'd prefer we test "aContent && !aContent->IsElement()".  r=me with that.
Attachment #668284 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 23

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

Comment 24

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f40ba56870b
(Assignee)

Updated

5 years ago
Flags: in-testsuite+
Thanks, Cameron.
https://hg.mozilla.org/mozilla-central/rev/7f40ba56870b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18

Updated

5 years ago
tracking-firefox17: + → -

Updated

5 years ago
tracking-firefox17: - → +
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.
Attachment #668284 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
status-firefox17: --- → fixed
You need to log in before you can comment on or make changes to this bug.