Closed Bug 989965 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

28 Branch
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla31
Tracking Status
firefox28 --- wontfix
firefox29 + verified
firefox30 + verified
firefox31 --- verified

People

(Reporter: simon.lindholm10, Assigned: heycam)

References

Details

(Whiteboard: [firebug-p1])

Crash Data

Attachments

(2 files, 1 obsolete file)

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)
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)
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)
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8399800 - Flags: review?(bzbarsky)
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...
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?
(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.
Attached 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: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
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.