stack overflow when large numbers of CSS rules match a single node [@ nsStyleSet::AddImportantRules]

RESOLVED FIXED in mozilla1.9.1a1

Status

()

Core
CSS Parsing and Computation
--
critical
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: tempeml4bugzilla, Assigned: dbaron)

Tracking

({crash, testcase})

Trunk
mozilla1.9.1a1
crash, testcase
Points:
---
Bug Flags:
blocking1.9.1 -
wanted1.9.1 -
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(crash signature, URL)

Attachments

(6 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
User-Agent:       Mozilla/4.0 (compatible; MSIE 7.0; Windows NT 5.1; .NET CLR 1.1.4322; FDM; .NET CLR 3.0.04506.30; .NET CLR 2.0.50727; .NET CLR 3.0.04506.648; .NET CLR 3.5.21022)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a1pre) Gecko/2008061303 Minefield/3.1a1pre

While testing for a limit to CSS declarations, I had generated a CSS file containing over 50k unique declarations to a single ID, plus one declaration for another ID at the end of the CSS file.  File Size was 1,000,016 bytes (977kb).  Loading the page causes a crash in both the latest/fully updated FF2 and Minefield at the time of posting.

Reproducible: Always

Steps to Reproduce:
1.Visit Page.
2.Crash.
3.
Actual Results:  
Browser crashes.

Expected Results:  
The page should have rendered and presented two lines containing "Test" and two buttons to obtain the color styles.  "I Color" button is for the 50k declarations. That button should return the limit (I hope there isn't one) or a value of color:#999980 (or equivalent) as the last declared color for id="i". "Z Color" was to test if there was a filesize issue as well and should have returned Green or equivalent.

Fresh and unconfigured installations, as per W3C testing standards for CSS.
I have (Hopefully) attached the HTML page and the CSS file.  If you want the test itself to work (The buttons to produce results) You'll need to put Prototype.js in the same directory as the html file.  Prototype.js not included for obvious security reasons.

This test was originally designed for IE, which returns a filesize issue (Z was undefined) with a unique declaration to a single id limit of 4096 declarations (in which the file was shortened to that + Z and Z was defined as green).

Minefield Report:
Add-ons: {972ce4c6-7e08-4474-a285-3208198ce6fd}:3.1a1pre
BuildID: 2008061303
CrashTime: 1213413996
Email: 
InstallTime: 1213411680
ProductName: Firefox
SecondsSinceLastCrash: 2267
StartupTime: 1213412996
Theme: classic/1.0
UserID: 18a0ee35-f60a-474a-aafb-307af8ec1994
Vendor: Mozilla
Version: 3.1a1pre
(Reporter)

Comment 1

10 years ago
Created attachment 325063 [details]
The HTML Test that causes the crash.

Needs the CSS file as well.
Prototype.js not included, but useful if you want to extract results from the buttons.
(Reporter)

Comment 2

10 years ago
Comment on attachment 325063 [details]
The HTML Test that causes the crash.

Basically needs
http://rapidshare.com/files/122296773/CSSFSLimit.css.html because it's too large for bugzilla (977kb)
Please type about:crashes and provide the crash IDs from this cras
Keywords: crash

Comment 4

10 years ago
Confirmed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008061305 Minefield/3.0pre ID:2008061305

Crash report: http://crash-stats.mozilla.com/report/pending/6c861878-39fa-11dd-80a9-0013211cbf8a
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 5

10 years ago
Created attachment 325088 [details]
Crash info (call stack, etc)

Updated

10 years ago
Summary: An excessively large CSS file crashes the browser. Either filesize or number of declarations is an issue. → An excessively large CSS file crashes the browser. Either filesize or number of declarations is an issue. Stack overflow [@ gklayout!nsStyleSet::AddImportantRules]
confirming a bug in Firefox:General is usually a very bad idea but thanks for the crash id.

-> Core:CSS

Stack overflow: 
0  	xul.dll  	nsStyleSet::AddImportantRules  	 mozilla/layout/style/nsStyleSet.cpp:434
1 	xul.dll 	nsStyleSet::AddImportantRules 	mozilla/layout/style/nsStyleSet.cpp:434
2 	xul.dll 	nsStyleSet::AddImportantRules 	mozilla/layout/style/nsStyleSet.cpp:434
3 	xul.dll 	nsStyleSet::AddImportantRules 	mozilla/layout/style/nsStyleSet.cpp:434
4 	xul.dll 	nsStyleSet::AddImportantRules 	mozilla/layout/style/nsStyleSet.cpp:434
5 	xul.dll 	nsStyleSet::AddImportantRules 	mozilla/layout/style/nsStyleSet.cpp:434
Component: General → Style System (CSS)
Product: Firefox → Core
QA Contact: general → style-system
Summary: An excessively large CSS file crashes the browser. Either filesize or number of declarations is an issue. Stack overflow [@ gklayout!nsStyleSet::AddImportantRules] → An excessively large CSS file crashes the browser. Either filesize or number of declarations is an issue. Stack overflow [@ nsStyleSet::AddImportantRules]
Version: unspecified → Trunk

Comment 7

10 years ago
Thanks Matthias, I forgot to change the component :/ sorry.
Adding the reporter testcase in URL (one single HTML file ~1Mo).
(Assignee)

Updated

10 years ago
Assignee: nobody → dbaron
Flags: wanted1.9.0.x?
Flags: blocking1.9.1?
OS: Windows XP → All
Hardware: PC → All
Target Milestone: --- → mozilla1.9.1
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

10 years ago
Summary: An excessively large CSS file crashes the browser. Either filesize or number of declarations is an issue. Stack overflow [@ nsStyleSet::AddImportantRules] → stack overflow when large numbers of CSS rules match a single node [@ nsStyleSet::AddImportantRules]
(Assignee)

Comment 8

10 years ago
Created attachment 325111 [details] [diff] [review]
patch 1: make AddImportantRules, etc., use iteration and its own stack

needs more tests
(Assignee)

Comment 9

10 years ago
Created attachment 325112 [details] [diff] [review]
patch 2: remove nsRuleList

patch 3 is not written yet (it will use a destroy queue to destroy rule nodes)
(Assignee)

Comment 10

10 years ago
Created attachment 325113 [details] [diff] [review]
patch 4: test for this bug
(Assignee)

Comment 11

10 years ago
Created attachment 325143 [details] [diff] [review]
patch 3: use a queue to destroy rule node children
(Assignee)

Comment 12

10 years ago
Created attachment 325334 [details] [diff] [review]
patch 1: make AddImportantRules, etc., use iteration and its own stack

with test for cascading added
Attachment #325111 - Attachment is obsolete: true
Attachment #325334 - Flags: superreview?(bzbarsky)
Attachment #325334 - Flags: review?(fantasai.bugs)
(Assignee)

Updated

10 years ago
Attachment #325113 - Attachment description: patch 4: test → patch 4: test for this bug
(Assignee)

Updated

10 years ago
Attachment #325112 - Flags: superreview?(bzbarsky)
Attachment #325112 - Flags: review?(bzbarsky)
(Assignee)

Comment 13

10 years ago
Comment on attachment 325113 [details] [diff] [review]
patch 4: test for this bug

This test is pretty slow; not sure if there's anything we ought to do about that.  I think I do want to test a bit more than currently causes the bug.
Attachment #325113 - Flags: superreview?(bzbarsky)
Attachment #325113 - Flags: review?(bzbarsky)
(Assignee)

Updated

10 years ago
Attachment #325143 - Flags: superreview?(bzbarsky)
Attachment #325143 - Flags: review?(bzbarsky)
(Assignee)

Comment 14

10 years ago
For a quick overview:  the case of a very large number of rules matching the same element triggered stack overflows in two different ways:

 patch 1 fixes the one in AddImportantRules

 patch 3 fixes the one in rule node destruction

 patch 2 is preparatory cleanup for patch 3, and patch 4 is a test for this bug

Comment 15

10 years ago
Comment on attachment 325334 [details] [diff] [review]
patch 1: make AddImportantRules, etc., use iteration and its own stack

I don't really understand why the zIndex keeps being incremented in your tests rather than just using two fixed numbers. But it looks correct. r=fantasai on code and tests if you document your do_test function (state what it does, what each argument means, and what values are valid)
Attachment #325334 - Flags: review?(fantasai.bugs) → review+
Flags: wanted1.9.1-
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Comment on attachment 325334 [details] [diff] [review]
patch 1: make AddImportantRules, etc., use iteration and its own stack

> nsStyleSet::AddImportantRules(nsRuleNode* aCurrLevelNode,
>                               nsRuleNode* aLastPrevLevelNode)

The changes here seem OK, but do we want to use some sort of auto-array to avoid heap-allocation if there aren't too many important rules?

> nsStyleSet::AssertNoCSSRules(nsRuleNode* aCurrLevelNode,
>   if (!aCurrLevelNode || aCurrLevelNode == aLastPrevLevelNode)

The second check can go away just like in the other two functions, right?

I guess a crashtest for this would just be too big, right?

sr=bzbarsky
Attachment #325334 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 325112 [details] [diff] [review]
patch 2: remove nsRuleList

r+sr=bzbarsky
Attachment #325112 - Flags: superreview?(bzbarsky)
Attachment #325112 - Flags: superreview+
Attachment #325112 - Flags: review?(bzbarsky)
Attachment #325112 - Flags: review+
Comment on attachment 325113 [details] [diff] [review]
patch 4: test for this bug

Ah, here's the crashtest!  ;)
Attachment #325113 - Flags: superreview?(bzbarsky)
Attachment #325113 - Flags: superreview+
Attachment #325113 - Flags: review?(bzbarsky)
Attachment #325113 - Flags: review+
Comment on attachment 325143 [details] [diff] [review]
patch 3: use a queue to destroy rule node children

>+  // aDestroyQueueTail is for internal use only.
>+  NS_HIDDEN_(void) Destroy(nsRuleNode ***aDestroyQueueTail = nsnull);

Just create a private overload instead of trying to enforce hiding via comments?

Also, this fixes situations in which we have a bunch of rulenodes all of which have at least enough kids to hash and the depth is too deep, right?  Can we add a crashtest for this too?

r+sr=bzbarsky with those.
Attachment #325143 - Flags: superreview?(bzbarsky)
Attachment #325143 - Flags: superreview+
Attachment #325143 - Flags: review?(bzbarsky)
Attachment #325143 - Flags: review+
(Assignee)

Comment 20

10 years ago
(In reply to comment #19)
> (From update of attachment 325143 [details] [diff] [review])
> >+  // aDestroyQueueTail is for internal use only.
> >+  NS_HIDDEN_(void) Destroy(nsRuleNode ***aDestroyQueueTail = nsnull);
> 
> Just create a private overload instead of trying to enforce hiding via
> comments?

No need for an overload; I'll call the internal one DestroyInternal and drop the default param.  Good point, though.

> Also, this fixes situations in which we have a bunch of rulenodes all of which
> have at least enough kids to hash and the depth is too deep, right?  Can we add
> a crashtest for this too?

Let's see if this crashtest survives first; it's slow enough, and that one would be a few orders of magnitude slower (if we don't want to make too many assumptions about the switch-to-hash constant).
(Assignee)

Comment 21

10 years ago
I had to revert the crashtest because it was too slow, but the rest is landed:

http://hg.mozilla.org/mozilla-central/index.cgi/rev/a00c120d4a6e
http://hg.mozilla.org/mozilla-central/index.cgi/rev/5b6fa5bcaccd
http://hg.mozilla.org/mozilla-central/index.cgi/rev/571dbbcf60bf
http://hg.mozilla.org/mozilla-central/index.cgi/rev/6a513bc88338
http://hg.mozilla.org/mozilla-central/index.cgi/rev/4c0aae446b52
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: mozilla1.9.1 → mozilla1.9.1a1

Updated

10 years ago
Duplicate of this bug: 448131
Flags: wanted1.9.0.x? → wanted1.9.0.x+
dbaron: Is this likely the same as these other crashes with the same top frame?

  bp-321475b0-a418-11dd-88cb-001cc45a2ce4
  bp-dc729691-a41a-11dd-8d7b-001321b13766
  bp-8706a799-a420-11dd-8427-001cc45a2c28
  bp-16860311-a446-11dd-b6e9-001cc45a2ce4
  bp-44f8b86e-a456-11dd-bcfc-001321b13766
  bp-102f5707-a46c-11dd-89dc-001a4bd43ed6

If so that would make this crash a 3.0.3 topcrash. Currently number 8. Can you see if your patch applies to 1.9.0 and requested approval1.9.0.5 on it?
Flags: blocking1.9.0.5?
Blocking on the assumption that this is, in fact, the fix for the topcrash.
Flags: blocking1.9.0.5? → blocking1.9.0.5+
(Assignee)

Comment 25

10 years ago
This would not fix any of the crashes in comment 23.  This is a stack overflow.
Renominating based on comment 25.
Flags: blocking1.9.0.5+ → blocking1.9.0.5?
Flags: blocking1.9.0.5?
For what it's worth, we've seen this stack a handful of times on the 1.9.0 branch. It might be worth fixing there, though I doubt it.
(Assignee)

Comment 28

10 years ago
Please reread comment 25 more carefully.
(Assignee)

Comment 29

10 years ago
I filed bug 466024 on that crash so you stop commenting here.
David, I've read comment 25 very carefully and filed bug 466021 prior to commenting here. Please re-read my comment 27 which says that I've seen a handful of *this* stack (a stack overflow) on the 1.9.0 branch.

Specifically:
  bp-6555beec-cb64-4459-84af-927e20081120
  bp-663220b0-d9bb-4bcc-8f86-cd4520081119
(Assignee)

Comment 31

10 years ago
Yep, sorry.
Flags: wanted1.9.0.x+
(Assignee)

Updated

9 years ago
Duplicate of this bug: 536811
Crash Signature: [@ nsStyleSet::AddImportantRules]
You need to log in before you can comment on or make changes to this bug.