Last Comment Bug 782888 - assertion "Shouldn't be trying to restyle non-elements directly" when setting presentation attribute on SVG text element
: assertion "Shouldn't be trying to restyle non-elements directly" when setting...
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla18
Assigned To: Cameron McCormack (:heycam) (away Jun 25 – Jul 10)
:
Mentors:
Depends on:
Blocks: 775304 788919
  Show dependency treegraph
 
Reported: 2012-08-15 00:58 PDT by Cameron McCormack (:heycam) (away Jun 25 – Jul 10)
Modified: 2012-10-07 15:00 PDT (History)
6 users (show)
cam: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
test (359 bytes, image/svg+xml)
2012-08-15 00:58 PDT, Cameron McCormack (:heycam) (away Jun 25 – Jul 10)
no flags Details
backout bug 775304 (4.07 KB, patch)
2012-09-28 09:43 PDT, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
bajaj.bhavana: approval‑mozilla‑aurora+
Details | Diff | Review
patch (3.93 KB, patch)
2012-10-04 19:17 PDT, Cameron McCormack (:heycam) (away Jun 25 – Jul 10)
bzbarsky: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review

Description Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2012-08-15 00:58:01 PDT
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.
Comment 1 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2012-08-15 01:59:45 PDT
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 2012-08-16 09:25:49 PDT
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 2012-08-16 09:29:51 PDT
I could be way-off here as the non-inherited thing seems to suggest that it shouldn't traverse children.
Comment 4 Robert Longson 2012-08-17 06:38:25 PDT
I tried backing out bug 775304 but it had no effect.
Comment 5 Robert Longson 2012-08-17 06:42:16 PDT
Sorry I got that completely wrong. Backing out bug 775304 fixes this issue.
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-08-29 00:48:15 PDT
> 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 [:bz] (Out June 25-July 6) 2012-08-29 01:00:44 PDT
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 [:bz] (Out June 25-July 6) 2012-08-31 23:23:53 PDT
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 2012-09-27 11:55:25 PDT
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 2012-09-27 11:59:51 PDT
I think we just want to back out bug 775304.
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-09-27 12:12:48 PDT
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] (Away Jun. 27 - Jul. 13) 2012-09-28 08:53:30 PDT
I should do it.
Comment 13 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-09-28 09:43:52 PDT
Created attachment 665953 [details] [diff] [review]
backout bug 775304
Comment 14 bhavana bajaj [:bajaj] 2012-09-28 15:05:33 PDT
Comment on attachment 665953 [details] [diff] [review]
backout bug 775304

Approving for backout in aurora based on comment 9
Comment 15 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-09-29 02:51:58 PDT
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) (away Jun 25 – Jul 10) 2012-10-03 23:23:11 PDT
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 [:bz] (Out June 25-July 6) 2012-10-04 08:09:50 PDT
> 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) (away Jun 25 – Jul 10) 2012-10-04 19:10:31 PDT
Stealing.
Comment 19 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2012-10-04 19:17:41 PDT
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.
Comment 20 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2012-10-04 19:27:14 PDT
Aurora try run: https://tbpl.mozilla.org/?tree=Try&rev=1f0337ada249
Comment 21 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-04 19:34:27 PDT
> 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 [:bz] (Out June 25-July 6) 2012-10-04 19:36:49 PDT
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) (away Jun 25 – Jul 10) 2012-10-04 19:38:18 PDT
(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) (away Jun 25 – Jul 10) 2012-10-04 19:48:21 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f40ba56870b
Comment 25 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-10-05 01:38:25 PDT
Thanks, Cameron.
Comment 26 Ed Morley [:emorley] 2012-10-05 03:58:19 PDT
https://hg.mozilla.org/mozilla-central/rev/7f40ba56870b
Comment 27 Alex Keybl [:akeybl] 2012-10-07 13:56:58 PDT
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) 2012-10-07 15:00:54 PDT
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

Note You need to log in before you can comment on or make changes to this bug.