989965 - Crash in nsCSSRuleProcessor::GetContentState with inIDOMUtils.getCSSStyleRules()

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Simon Lindholm]

Attachment: firebug-crash-test.html

As reported in https://code.google.com/p/fbug/issues/detail?id=7302, Firefox crashes when opening Firebug's HTML panel on a page with the following HTML (attached to the bug):

<style>
::-moz-placeholder { color: red; }
::-moz-placeholder:focus { color: green; }
</style>
<input type="text" placeholder="Hello world">


What Firebug does can be emulated by the following run from a scratchpad:

domUtils = Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils)
domUtils.getCSSStyleRules(content.document.body, ":-moz-placeholder")


Crash report: https://crash-stats.mozilla.com/report/index/98658806-be12-4524-a9ae-2f5572140331
Regression range:
Last good nightly: 2013-11-27
First bad nightly: 2013-11-28

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6ecf0c4dfcbe&tochange=3c44c1f43a67

So, pretty clearly bug 922669.

Comment 1

[Boris Zbarsky [:bzbarsky]]

In a debug build, it's pretty clear what goes wrong:

###!!! ASSERTION: aPseudoElement must be specified if the pseudo supports :hover and :active: '!(!aPseudoElement && nsCSSPseudoElements::PseudoElementSupportsUserActionState (aPseudoType))', file ../../../mozilla/layout/style/nsRuleProcessorData.h, line 498

and then we later crash when dereferencing that pointer.

In this case what happens is that the body itself of course has no ::-moz-placeholder styles.  So when we do this bit in nsComputedDOMStyle::GetStyleContextForElementNoFlush:

511         nsIFrame* frame = nsLayoutUtils::GetStyleFrame(aElement);
512         Element* pseudoElement = frame ? frame->GetPseudoElement(type) : nullptr;
513         sc = styleSet->ResolvePseudoElementStyle(aElement, type, parentContext,
514                                                  pseudoElement);

we're passing in a null pseudoElement.

We should probably just go ahead and return null if pseudoElement is null here, right?  At least for the cases when the pseudo is event-sensitive...

Comment 2

[Cameron McCormack (:heycam)]

Returning null there is a good safe option.  But for consistency with how we deal with getComputedStyle(elt, "::before") where the element does not have a ::before pseudo-element, should we instead be making StateSelectorMatches (or something further up the call stack) handle null by not matching any state pseudo-classes?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm.  Where does that happen right now?  I don't see any obvious handling for ::before in computed style code, so is it happening in ResolvePseudoElementStyle somewhere?

Comment 4

[Cameron McCormack (:heycam)]

I guess the difference is that it's not possible to have selectors like ::before:hover in the style sheet, so we'll never call into StateSelectorMatches, i.e. we'll never get into the body of this loop:

  https://hg.mozilla.org/mozilla-central/file/1417d180a1d8/layout/style/nsCSSRuleProcessor.cpp#l1678

Comment 5

[Cameron McCormack (:heycam)]

Attachment: patch

Comment 6

[Cameron McCormack (:heycam)]

Note that we can get this crash with a regular window.getComputedStyle; we don't need inDOMUtils.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

>+      // pseudo-element.  As |selector| has a user action state pseudo-class,

You mean "pseudo-element" at the end there?

r=me

Comment 8

[Boris Zbarsky [:bzbarsky]]

We may want to backport this to branches too...

Comment 9

[Cameron McCormack (:heycam)]

I mean pseudo-class, like :focus.  Though I think the right term is actually "user action pseudo-class", so I'll get rid of the "state".

Comment 10

[Boris Zbarsky [:bzbarsky]]

Right, but why does the selector containing a pseudo-class mean it doesn't match?  Or is the point that these pseudo-classes can only match when there is an actual pseudo-element, because without that the user can't interact with the pseudo-element?

Comment 11

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky [:bz] from comment #10)
> Or is the point that these pseudo-classes can only match when there
> is an actual pseudo-element, because without that the user can't interact
> with the pseudo-element?

Yeah that's right.  How about I adjust the comment to be clearer:

  // We can get here when calling getComputedStyle(aElt, aPseudo) if:                                
  //
  //   * aPseudo is a pseudo-element that supports a user action                                     
  //     pseudo-class, like "::-moz-placeholder";
  //   * there is a style rule that uses a pseudo-class on this                                      
  //     pseudo-element in the document, like ::-moz-placeholder:hover; and
  //   * aElt does not have such a pseudo-element.
  //
  // We know that the selector can't match, since there is no element for
  // the user action pseudo-class to match against.

Comment 12

[Cameron McCormack (:heycam)]

https://tbpl.mozilla.org/?tree=Try&rev=518598d3f0a0

Comment 13

[Boris Zbarsky [:bzbarsky]]

Yes, that sounds good.

Comment 14

[Cameron McCormack (:heycam)]

Attachment: patch (v2)

Turns out we also need to check whether the selector we're going to match against has any pseudo-classes.  Otherwise, a document like:

  <style>
  body::before { color: blue; }
  </style>
  <body>
  <script>
    alert(getComputedStyle(document.body, "::before").color);
  </script>

won't alert blue, as the ResolvePseudoElementStyle will just get default styles (as no rules will have matched) rather than include the body::before rule.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8400279 [details] [diff] [review]
patch (v2)

r=me

Comment 16

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d98b5699fa3

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2d98b5699fa3

Comment 18

[Cameron McCormack (:heycam)]

Comment on attachment 8400279 [details] [diff] [review]
patch (v2)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 922669
User impact if declined: Certain style sheets can (safely) crash the browser
Testing completed (on m-c, etc.): Patch is on m-c for several hours; manually tested that the crash is now avoided
Risk to taking this patch (and alternatives if risky): Low; it only causes certain selectors to not match
String or IDL/UUID changes made by this patch: N/A

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/84ea2659f9ea
https://hg.mozilla.org/releases/mozilla-beta/rev/b5185eeba69d

Comment 20

[Manuela Muntean [Away]]

Verified as fixed with Firefox 29 beta 5 and latest Nightly on Win 7 x64, Ubuntu 12.10 x86 and Mac OS X 10.9: the browser doesn't crash anymore when following steps from comment 0.

Comment 21

[Ada [:adalucinet]]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (X11; Linux i686; rv:30.0) Gecko/20100101 Firefox/30.0

Verified as fixed on Firefox 30 beta 1 (Build Id: 20140428174145) on Mac OS X 10.8.5, Windows 7 x64 and Ubuntu 13.10 x32 with steps from comment 0 - no crashes encountered.