Last Comment Bug 611922 - "ABORT: aRelevantLinkVisited should only be set when we have a separate style"
: "ABORT: aRelevantLinkVisited should only be set when we have a separate style"
Status: RESOLVED FIXED
fixed-in-cedar
: assertion, testcase
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: x86 Mac OS X
: P1 critical (vote)
: mozilla6
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on:
Blocks: 343943 532972
  Show dependency treegraph
 
Reported: 2010-11-12 19:11 PST by Jesse Ruderman
Modified: 2011-05-21 17:35 PDT (History)
5 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (uses a relative image url) (647 bytes, text/html)
2010-11-12 19:11 PST, Jesse Ruderman
no flags Details
stack trace (7.52 KB, text/plain)
2010-11-12 19:11 PST, Jesse Ruderman
no flags Details
Simpler, self-contained testcase (232 bytes, text/html)
2011-01-04 23:06 PST, Boris Zbarsky [:bz]
no flags Details
Proposed fix (2.96 KB, patch)
2011-01-05 00:18 PST, Boris Zbarsky [:bz]
no flags Details | Diff | Review
And with the test actually waiting so it can fail (3.06 KB, patch)
2011-01-05 00:24 PST, Boris Zbarsky [:bz]
dbaron: review-
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. (2.76 KB, patch)
2011-05-03 11:07 PDT, Boris Zbarsky [:bz]
dbaron: review+
Details | Diff | Review

Description Jesse Ruderman 2010-11-12 19:11:10 PST
Created attachment 490318 [details]
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 2010-11-12 19:11:28 PST
Created attachment 490319 [details]
stack trace
Comment 2 Jesse Ruderman 2010-11-12 19:11:40 PST
Bug 575675 shows another way to trigger this assertion (with a chrome document).
Comment 3 Jesse Ruderman 2010-11-12 19:13:01 PST
Usually aborts within 5 seconds.
Comment 4 Boris Zbarsky [:bz] 2011-01-04 22:51:01 PST
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 [:bz] 2011-01-04 22:55:42 PST
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 [:bz] 2011-01-04 23:06:24 PST
Created attachment 501268 [details]
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 [:bz] 2011-01-04 23:12:42 PST
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 [:bz] 2011-01-05 00:18:54 PST
Created attachment 501278 [details] [diff] [review]
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 [:bz] 2011-01-05 00:24:01 PST
Created attachment 501279 [details] [diff] [review]
And with the test actually waiting so it can fail
Comment 10 Boris Zbarsky [:bz] 2011-01-05 00:25:08 PST
David, I'm not sure how serious this abort situation is; should I be considering this for 2.0?
Comment 11 Boris Zbarsky [:bz] 2011-01-05 00:46:33 PST
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-7 (review requests must explain patch) 2011-01-11 13:59:33 PST
(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-7 (review requests must explain patch) 2011-01-12 10:08:19 PST
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 [:bz] 2011-01-12 14:48:25 PST
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:] 2011-03-17 05:14:33 PDT
FYI, I see this consistently testing crash urls on www.ankama-shop.com and www.battlefieldheroes.com
Comment 16 Boris Zbarsky [:bz] 2011-05-03 11:01:05 PDT
> 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 [:bz] 2011-05-03 11:07:08 PDT
Created 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.
Comment 18 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-05-19 10:27:14 PDT
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 [:bz] 2011-05-20 19:46:11 PDT
http://hg.mozilla.org/projects/cedar/rev/bd9646fd621c
Comment 20 Daniel Holbert [:dholbert] 2011-05-21 17:35:28 PDT
http://hg.mozilla.org/mozilla-central/rev/bd9646fd621c

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