946312 - Crashes in StateSelectorMatches

Status: RESOLVED DUPLICATE of bug 945048

(Core :: CSS Parsing and Computation, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

https://crash-stats.mozilla.com/query/?product=Firefox&version=ALL%3AALL&range_value=4&range_unit=weeks&date=12%2F04%2F2013+17%3A00%3A00&query_search=signature&query_type=contains&query=StateSelectorMatches&reason=&release_channels=&build_id=&process_type=any&hang_type=any

These start with the 2013-11-28-03-02-01-mozilla-central build, and are in code introduced by:

http://hg.mozilla.org/mozilla-central/rev/8e580ff4d7ca

 Bug 922669 - Part 6: Split out user action pseudo-class matching from SelectorMatches. r=bz
author	Cameron McCormack <[e-mail]>
	Thu Nov 28 17:46:39 2013 +1100 (at Thu Nov 28 17:46:39 2013 +1100)

They occur on Windows and Linux in low numbers, but are currently the #6 topcrasher on the Mac.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

> but are currently the #6 topcrasher on the Mac.

They're the #6 Mac topcrasher on the 28 branch.

Comment 2

[bhavana bajaj [:bajaj]]

Passing this on to :cam for help with investigation as this may be a fallout from Bug 922669. Also this seems to be across platform.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Bhavana, did you mean to pass this to Cameron McCormack <[e-mail]>? :-)

Comment 4

[Steven Michaud [:smichaud] (Retired)]

All these crashes happen here:

http://hg.mozilla.org/mozilla-central/annotate/84a5a5800bd3/layout/style/nsCSSRuleProcessor.cpp#l1671

They're probably null dereferences (aElement seems to be null).

Comment 5

[Cameron McCormack (:heycam)]

Thanks Steven, looking.

Comment 6

[Cameron McCormack (:heycam)]

I can't reproduce locally yet, but:  Boris, in bug 945048 I removed the check for null pdata->mPseudoElement thinking that it wasn't needed now that bogus selectors like ::-moz-selection:hover don't parse.  But I think we're crashing here because have a set of rules like:

  ::-moz-progress-bar:hover { }
  ::-moz-selection { }

and when we are calling ProbePseudoElementStyle for the ::-moz-selection, we're passing in null as the element (as expected), but we end up finding the ::moz-progress-bar:hover rule and checking to see if it should contribute to the style returned by ProbePseudoElementStyle.  So I think it makes sense to restore those checks.  Does that sound right to you?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> but we end up finding the ::moz-progress-bar:hover rule

That shouldn't happen.  Those should be in separate rulehashes, no?

The stacks in breakpad with this signature seem to be pretty confused (like those methods don't actually call each other, mostly), but here's my question: is this just a duplicate of bug 945048, maybe?  I see no instances of this crash with a 2013-12-03 build or newer....

In particular, bug 945048 was a crash in StateSelectorMatches when we get to the GetContentStateForVisitedHandling call, but the rule there used :active.  If you had a testcase as for that bug but using :hover, then you'd crash on the line where this bug says we're crashing, right?

Comment 8

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky [:bz] from comment #7)
> That shouldn't happen.  Those should be in separate rulehashes, no?

Yeah, they should be.

> The stacks in breakpad with this signature seem to be pretty confused (like
> those methods don't actually call each other, mostly), but here's my
> question: is this just a duplicate of bug 945048, maybe?  I see no instances
> of this crash with a 2013-12-03 build or newer....

Oh, I misread the most recent build ID on crash-stats and looked locally at 2013-12-03 to see whether the bug 945048 fix is in there.

> In particular, bug 945048 was a crash in StateSelectorMatches when we get to
> the GetContentStateForVisitedHandling call, but the rule there used :active.
> If you had a testcase as for that bug but using :hover, then you'd crash on
> the line where this bug says we're crashing, right?

Yes :hover and :active should have exactly the same crashing-or-not behaviour.

So yeah, if there are no more crashes by tomorrow, let's dupe it to bug 945048.