945048 - crash in nsINode::GetBoolFlag(nsINode::BooleanFlag)

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Guilherme Lima]

This bug was filed from the Socorro interface and is 
report bp-a903b6d3-9ab6-461e-ad58-14d592131201.
=============================================================
I'm constantly crashing trying to load any lyrics page on http://letras.mus.br/

other crash reports:
https://crash-stats.mozilla.com/report/index/b09821dd-2816-441c-8c43-6a40b2131201
https://crash-stats.mozilla.com/report/index/32c314fe-2ca8-4faa-84f4-ec02a2131201
https://crash-stats.mozilla.com/report/index/ef658389-ce29-4135-a19b-e1a462131130

It started crashing for me with the build from Oct 29th. I don't have the exactly data because I don't visit the site very often.

Comment 1

[Simon Montagu :smontagu]

I see this starting in regression range http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6ecf0c4dfcbe&tochange=3c44c1f43a67. There is pseudo-element stuff in the crash-stats stack, so I suspect bug 922669 or one of its friends and relations.

Comment 2

[Guilherme Lima]

Ah, yeah, I meant Nov 29th of course.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Fwiw, all three of those crashes look like null-derefs.

Comment 4

[Cameron McCormack (:heycam)]

It looks like the issue is that the page has a style rule with a |button::-moz-focus-inner:active| selector, and in nsButtonFrameRenderer::ReResolveStyle, we don't pass in a content node for the pseudo-element to ProbePseudoStyle, since there isn't one (these are extra style contexts just used for rendering).  Then in ContentEnumFunc, we pass the null pseudo-element into StateSelectorMatches.  I think we should just assume that the state never matches here by returning if pdata->mPseudoElement is null.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Makes sense to me.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Wait, why did "button::-moz-focus-inner:active" parse at all?

Comment 7

[Cameron McCormack (:heycam)]

Oh, great question.  The flag I added in nsCSSPseudoElementList.h must not be working?

Comment 8

[Cameron McCormack (:heycam)]

I'm not calling nsCSSPseudoElements::PseudoElementSupportsUserActionState anywhere in nsCSSParser.cpp. :/

Comment 9

[Cameron McCormack (:heycam)]

Attachment: patch

Comment 10

[Cameron McCormack (:heycam)]

https://tbpl.mozilla.org/?tree=Try&rev=8b4e3f5d3ead

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8340830 [details] [diff] [review]
patch

>+      if (!pdata->mPseudoElement ||

Do we still need this now that we've fixed the parser bug?

>+  if (!aData->mPseudoElement) {

Likewise.

r=me modulo those.

Comment 12

[Cameron McCormack (:heycam)]

No we shouldn't strictly need those; all of the times we're calling ProbePseudoElementStyle or ResolvePseudoElementStyle for pseudo-elements with the flag allowing the pseudo-classes after them we're passing in an element.

Comment 13

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/54a8c5aae115

Comment 14

[Cameron McCormack (:heycam)]

Thanks Guilherme for the prompt bug report!

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/54a8c5aae115