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

VERIFIED FIXED in Firefox 29

Status

()

defect
--
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: simon.lindholm10, Assigned: heycam)

Tracking

28 Branch
mozilla31
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox28 wontfix, firefox29+ verified, firefox30+ verified, firefox31 verified)

Details

(Whiteboard: [firebug-p1], crash signature)

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

5 years ago
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.
Whiteboard: [firebug-p1]
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...
Flags: needinfo?(cam)
Assignee

Comment 2

5 years ago
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?
Flags: needinfo?(cam)
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?
Flags: needinfo?(cam)
Assignee

Comment 4

5 years ago
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
Flags: needinfo?(cam)
Assignee

Comment 5

5 years ago
Posted patch patch (obsolete) — Splinter Review
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8399800 - Flags: review?(bzbarsky)
Assignee

Comment 6

5 years ago
Note that we can get this crash with a regular window.getComputedStyle; we don't need inDOMUtils.
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
Attachment #8399800 - Flags: review?(bzbarsky) → review+
We may want to backport this to branches too...
Assignee

Comment 9

5 years ago
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".
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?
Assignee

Comment 11

5 years ago
(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.
Yes, that sounds good.
Assignee

Comment 14

5 years ago
Posted patch patch (v2)Splinter Review
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.
Attachment #8399800 - Attachment is obsolete: true
Attachment #8400279 - Flags: review?(bzbarsky)
Comment on attachment 8400279 [details] [diff] [review]
patch (v2)

r=me
Attachment #8400279 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/2d98b5699fa3
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Assignee

Comment 18

5 years ago
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
Attachment #8400279 - Flags: approval-mozilla-beta?
Attachment #8400279 - Flags: approval-mozilla-aurora?
Attachment #8400279 - Flags: approval-mozilla-beta?
Attachment #8400279 - Flags: approval-mozilla-beta+
Attachment #8400279 - Flags: approval-mozilla-aurora?
Attachment #8400279 - Flags: approval-mozilla-aurora+
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.
Status: RESOLVED → VERIFIED
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.
You need to log in before you can comment on or make changes to this bug.