1043181 - Crash [@ nsCSSValue::GetStringValue(nsAString_internal&) ] with @counter-style

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Sebastian Zartner [:sebo]]

Attachment: atCounterStyle.html

Example crash report:
https://crash-stats.mozilla.com/report/index/58a6e548-c794-49a8-b374-0fdae2140724

Test case:
1. Install Firebug 2.0.2[1]
2. Open Firebug on the attached files
3. Switch to the CSS panel
4. Reload the page

Sebastian

[1] https://addons.mozilla.org/firefox/addon/firebug/versions/2.0.2

Comment 1

[Sebastian Zartner [:sebo]]

Attachment: atCounterStyle.css

Comment 2

[Cameron McCormack (:heycam)]

The nsCSSCounterStyleRule copy constructor (which is called under CSSStyleSheet::EnsureUniqueInner) doesn't copy over its mValues array, which is where the descriptor values are stored.  Later we assume that the symbols descriptor nsCSSValue is a list, when it's null.  (Also mGeneration isn't initialized.)

Comment 3

[Cameron McCormack (:heycam)]

Attachment: patch

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Probably better to use ArrayLength(mValues) rather than eCSSCounterDesc_COUNT.  And given that you're there, I'd prefer ++i over i++ when it doesn't matter, since it's the conceptually simpler operation.

r=dbaron with that

I suppose it doesn't matter whether the generation is initialized to 0 or aCopy.mGeneration, so this is probably fine (and maybe slightly better just because it's more normal).

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, we perhaps need a test_rule_cloning to go along with test_value_cloning.  Though it would be a good bit of work to write.

Comment 6

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #4)
> I suppose it doesn't matter whether the generation is initialized to 0 or
> aCopy.mGeneration, so this is probably fine (and maybe slightly better just
> because it's more normal).

Yeah, the mGeneration value should be 0 at the time it is cloned, so I agree it shouldn't matter.

Comment 7

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ff70a5507f7

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6ff70a5507f7

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

Why not uplift this patch to 33 where @counter-style landed?

Comment 10

[Sebastian Zartner [:sebo]]

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

I can confirm that it's working now. Thanks for the fast patch!

> Why not uplift this patch to 33 where @counter-style landed?
I agree with that. I tried to set the appropriate tracking flags. (Please correct them if I did that wrong.)

Sebastian

Comment 11

[Cameron McCormack (:heycam)]

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

Approval Request Comment
[Feature/regressing bug #]: bug 966166
[User impact if declined]: certain CSS rules can cause a crash (at least when inspecting the rules using Firebug, possibly also in the normal course of viewing a page)
[Describe test coverage new/current, TBPL]: no test, but manual testing shows the crash is now avoided
[Risks and why]: low; the patch just initializes some variables that weren't previously
[String/UUID change made/needed]: N/A

Comment 12

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a55b8ef060a8

Comment 13

[Catalin Varga [QA][:VarCat]]

I couldn't reproduce the crash using the steps from the bug, also based on Socorro it  seems that the latest build that reproduce the crash was from 20140716183446. Marking the bug as Verified. If someone can still reproduce this bug please reopen it.

Comment 14

[Sebastian Zartner [:sebo]]

FWIW it's also working for me now using Firefox 32.0.2 + Firebug 2.0.2/2.0.4 on Win8.1.

Sebastian