Closed Bug 558943 Opened 10 years ago Closed 10 years ago

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

Categories

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

x86
macOS
defect

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: jruderman, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(4 files, 3 obsolete files)

Attached file 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.
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.
|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?
Blocks: 147777
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?
Component: Layout → Style System (CSS)
QA Contact: layout → style-system
blocking2.0: --- → ?
Attachment #445736 - Attachment is obsolete: true
Attachment #445738 - Flags: review?(dbaron)
Attached patch Proposed fix (obsolete) — Splinter Review
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #447460 - Flags: review?(dbaron)
Attached patch Fix the other caller too (obsolete) — Splinter Review
Attachment #447460 - Attachment is obsolete: true
Attachment #447461 - Flags: review?(dbaron)
Attachment #447460 - Flags: review?(dbaron)
Oh, and I think this blocks bug 494117, since I want to use ReparentStyleContext there.
Blocks: 494117
Summary: "ASSERTION: Shouldn't be trying to restyle non-elements directly" with :first-line → [FIx]"ASSERTION: Shouldn't be trying to restyle non-elements directly" with :first-line
Summary: [FIx]"ASSERTION: Shouldn't be trying to restyle non-elements directly" with :first-line → [FIX]"ASSERTION: Shouldn't be trying to restyle non-elements directly" with :first-line
Not doing this triggers asserts on some tests once I move to using ReparentStyleContext more.
Attachment #447461 - Attachment is obsolete: true
Attachment #447703 - Flags: review?(dbaron)
Attachment #447461 - Flags: review?(dbaron)
blocking2.0: ? → final+
Priority: -- → P1
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
Attachment #445738 - Flags: review?(dbaron) → review+
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.)
Attachment #447703 - Flags: review?(dbaron) → review+
(The method could be nsStyleContext::IsLinkContext(), perhaps?)
> 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.
> (The method could be nsStyleContext::IsLinkContext(), perhaps?)

Good idea.  Done.
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.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Depends on: 570866
Flags: in-testsuite+
Resolution: --- → FIXED
Depends on: 571105
You need to log in before you can comment on or make changes to this bug.