Closed Bug 439184 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla1.9.1a1

People

(Reporter: tempeml4bugzilla, Assigned: dbaron)

References

()

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(6 files, 1 obsolete file)

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
Needs the CSS file as well.
Prototype.js not included, but useful if you want to extract results from the buttons.
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
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
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
Thanks Matthias, I forgot to change the component :/ sorry.
Adding the reporter testcase in URL (one single HTML file ~1Mo).
Assignee: nobody → dbaron
Flags: wanted1.9.0.x?
Flags: blocking1.9.1?
OS: Windows XP → All
Hardware: PC → All
Target Milestone: --- → mozilla1.9.1
Status: NEW → ASSIGNED
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]
patch 3 is not written yet (it will use a destroy queue to destroy rule nodes)
with test for cascading added
Attachment #325111 - Attachment is obsolete: true
Attachment #325334 - Flags: superreview?(bzbarsky)
Attachment #325334 - Flags: review?(fantasai.bugs)
Attachment #325113 - Attachment description: patch 4: test → patch 4: test for this bug
Attachment #325112 - Flags: superreview?(bzbarsky)
Attachment #325112 - Flags: review?(bzbarsky)
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)
Attachment #325143 - Flags: superreview?(bzbarsky)
Attachment #325143 - Flags: review?(bzbarsky)
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 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+
(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).
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+
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.
Please reread comment 25 more carefully.
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
Flags: wanted1.9.0.x+
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.