611922 - "ABORT: aRelevantLinkVisited should only be set when we have a separate style"

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase (uses a relative image url)

###!!! ABORT: aRelevantLinkVisited should only be set when we have a separate style: 'aRulesIfVisited || !aRelevantLinkVisited', file layout/style/nsStyleContext.cpp, line 179

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Jesse Ruderman]

Bug 575675 shows another way to trigger this assertion (with a chrome document).

Comment 3

[Jesse Ruderman]

Usually aborts within 5 seconds.

Comment 4

[Boris Zbarsky [:bzbarsky]]

We understand bug 575675 now.  It has to do with the lack of :visited rules in the chrome preference stylesheet....  That doesn't seem likely to be relevant here.

What's happening here is that we assert when we're reparenting the style for the <span> as part of style reresolution.  At this point, the <span>'s parent is an <a> which has no attributes.  So presumably this is the reresolve for the href attribute removal.  But since it has no attributes, it shouldn't in fact be visited.  And we have |mLinkState = eLinkState_NotLink| for that <a>.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Ah, the key, of course, is that nsStyleSet::ReparentStyleContext uses the RelevantLinkVisited() value of the old style context (aStyleContext) for the new one.  But in this case, the visitedness actually changed.  I should be able to write a simpler testcase for this...

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Simpler, self-contained testcase

The timeout bit stinks, but async visited state lookup doesn't block onload (and ideally shouldn't be detectable at all), and I needed to make sure the script ran after we'd decided the link is visited.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I think this should only happen when going from the visited state to the not-link state, since that's the case in which the <a>'s new style context won't have an if-visited style.  In all other cases, this code in nsStyleSet::GetContext makes sure we have an if-visited rulenode to pass to FindChildWithRules:

  nsStyleContext *parentIfVisited =
    aParentContext ? aParentContext->GetStyleIfVisited() : nsnull;
  if (parentIfVisited) {
    if (!aVisitedRuleNode) {
      aVisitedRuleNode = aRuleNode;
    }

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

The test still shows the problem for me even with the 100ms timeout.  The worst that will happen with that timer is that the test won't catch a regression, so at least there shouldn't be any randomorange issues here...  I really wish the visited state lookup blocked onload.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: And with the test actually waiting so it can fail

Comment 10

[Boris Zbarsky [:bzbarsky]]

David, I'm not sure how serious this abort situation is; should I be considering this for 2.0?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Though... two things.

1)  Maybe we need to do this on any link state change.  In particular, when going from "unvisited" to "visited", I'm not sure we end up with the right visited bit in the style context....

2)  Maybe we can just make reparenting smarter?  e.g. can it use the new parent's "is relevant link visited" bit unless it's a style context for a link or something?

Comment 12

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

(In reply to comment #11)
> Though... two things.
> 
> 1)  Maybe we need to do this on any link state change.  In particular, when
> going from "unvisited" to "visited", I'm not sure we end up with the right
> visited bit in the style context....

I think we should if we at least reparent, given nsStyleSet::GetContext, at least with the fix below.

But what about the opposite case of the one you're patching in the bug:  the case where we go from not-a-link to is-a-link?

> 2)  Maybe we can just make reparenting smarter?  e.g. can it use the new
> parent's "is relevant link visited" bit unless it's a style context for a link
> or something?

Er, yes, I agree that's a bug in nsStyleSet::ReparentStyleContext.

Would that also fix this bug?  If so, it certainly seems like a better fix.

Comment 13

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

Comment on attachment 501279 [details] [diff] [review]
And with the test actually waiting so it can fail

Marking review- to elicit response.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Yeah, response is coming up.  I just had no power most of today, and need to think about the answers to you questions.

Comment 15

[Bob Clary [:bc] (inactive)]

FYI, I see this consistently testing crash urls on www.ankama-shop.com and www.battlefieldheroes.com

Comment 16

[Boris Zbarsky [:bzbarsky]]

> But what about the opposite case of the one you're patching in the bug:  the
> case where we go from not-a-link to is-a-link?

Comment 7 addresses this.

> Would that also fix this bug? 

Sure seems to.  I'll post a patch to this effect.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: When reparenting style contexts, use the visitedness of our new parent unless we're the style context for a link. In that situation, assume that our visitedness did not change.

Comment 18

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

Comment on attachment 529763 [details] [diff] [review]
When reparenting style contexts, use the visitedness of our new parent unless we're the style context for a link.  In that situation, assume that our visitedness did not change.

r=dbaron

Comment 19

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/projects/cedar/rev/bd9646fd621c

Comment 20

[Daniel Holbert [:dholbert]]

http://hg.mozilla.org/mozilla-central/rev/bd9646fd621c