558943 - [FIX]"ASSERTION: Shouldn't be trying to restyle non-elements directly" with :first-line

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: Shouldn't be trying to restyle non-elements directly: '!aContent || aContent->IsNodeOfType(nsINode::eELEMENT)', file /Users/jruderman/mozilla-central/layout/base/nsStyleChangeList.cpp, line 96

The node in question appears to be a text node.

Comment 1

[Jesse Ruderman]

Attachment: stack trace for the assertion

Comment 2

[Boris Zbarsky [:bzbarsky]]

Hmm.  So the issue is that the style-if-visited colors are different for the before and after style contexts for the text child of the link:

(gdb) p/x otherVis->GetStyleColor()->mColor
$37 = 0xff8b1a55
(gdb) p/x thisVis->GetStyleColor()->mColor
$38 = 0xffee0000

The former is the default visited color.  The latter is the default link color.

Comment 3

[Boris Zbarsky [:bzbarsky]]

|this| and |thisVis| have the same mCachedInheritedData->mColorData.

They also have the same mRuleNode, of course: the root.

They also have the same mParent, which looks wrong to me.  The mParent does have a style-if-visited style context, which seems like it would be the right thing to parent them to.

Maybe our style tree verification code should assert that any given style-if-visited either has the if-visited style of the parent as the parent or has a different rulenode than the main style context?

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK, so this looks like a bug in the interaction of nsStyleSet::ReparentStyleContext and nsStyleSet::GetContext.  When we're originally reparenting the kids of the ::first-line, we end up in ReparentStyleContext for the text, of course.  At that point, our new parent context is the new style context for the <a>.  This of course has a style if visited.  So do we, since our old parent had one too.

Now we land in GetContext.  aIsLink is true (as passed in from ReparentStyleContext), so we set parentIfVisited to aParentContext, which is wrong.  It should be aParentContext->GetStyleIfVisited() in this case.

It's not clear to me yet whether ReparentStyleContext should be passing PR_FALSE or whether it should pass something depending on the old context or whether GetContext needs some changes or what.

David, do you want to take a look?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Style tree check to catch this a little earlier

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Even better style tree check

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix the other caller too

Comment 9

[Boris Zbarsky [:bzbarsky]]

Oh, and I think this blocks bug 494117, since I want to use ReparentStyleContext there.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: And handle the visited rulenode better

Not doing this triggers asserts on some tests once I move to using ReparentStyleContext more.

Comment 11

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

Comment on attachment 445738 [details] [diff] [review]
Even better style tree check

I think you can actually make this even stronger:

if (childStyleIfVisited &&
    !((childStyleIfVisited->GetRuleNode() != aContext->GetRuleNode() &&
       childStyleIfVisited->GetParent() == aContext->GetParent()) ||
      childStyleIfVisited->GetParent() == aContext->GetParent()->GetStyleIfVisited())) {
  NS_ERROR(...);
  etc.
}

though it's worth adding a comment that the "ruleNode != ruleNode &&" depends on the pref style sheet having different rules for :link and :visited.

r=dbaron with that if you agree

Comment 12

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

Comment on attachment 447703 [details] [diff] [review]
And handle the visited rulenode better

r=dbaron, although maybe it's worth putting the parent equality test into a method on nsStyleContext?  It would also have to null-check mStyleIfVisited, but I think that's ok.  (Then the third sentence of the big comment could go there too.)

Comment 13

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

(The method could be nsStyleContext::IsLinkContext(), perhaps?)

Comment 14

[Boris Zbarsky [:bzbarsky]]

> I think you can actually make this even stronger:

I made it even stronger than that:

  // Since we have different rules for :link and :visited in our ua/user sheets,
  // we know that either childStyleIfVisited has a different rulenode than
  // aContext (in which case it has :visited rules applied and its parent must
  // be aContext->GetParent()), or it has the same rulenode and then its parent
  // must be aContext->GetParent()->GetStyleIfVisited().
  if (childStyleIfVisited &&
      !((childStyleIfVisited->GetRuleNode() != aContext->GetRuleNode() &&
         childStyleIfVisited->GetParent() == aContext->GetParent()) ||
        (childStyleIfVisited->GetRuleNode() == aContext->GetRuleNode() &&
         childStyleIfVisited->GetParent() ==
         aContext->GetParent()->GetStyleIfVisited()))) {

etc.  Though I'm a little torn about moving that outermost ! in all the way using deMorgan; might be more readable... hard to say.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> (The method could be nsStyleContext::IsLinkContext(), perhaps?)

Good idea.  Done.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/4a0fcf9b58ff for the assertions, but the strengthening in comment 14 is wrong.  See bug 570866.

Pushed http://hg.mozilla.org/mozilla-central/rev/e88b028a0483 for the main patch.