Bug 27748 - crash w/ stack overflow when same CSS file loaded repeatedly
Summary: crash w/ stack overflow when same CSS file loaded repeatedly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Dirk Pranke
URL: http://www.jordanzad.com/jordan/index...
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-27 18:10 PDT by Dirk Pranke
Modified: 2009-08-03 13:01 PDT (History)
3 users (show)

See Also:


Attachments
test case - html file that loads a large CSS file repeatedly (602 bytes, text/html)
2009-07-27 19:19 PDT, Dirk Pranke
no flags Details
test case - CSS file loaded by HTML file (244.14 KB, text/css)
2009-07-27 19:20 PDT, Dirk Pranke
no flags Details
patch for crash (297.53 KB, patch)
2009-07-27 20:16 PDT, Dirk Pranke
sam: review-
Details | Formatted Diff | Diff
revied patch w/ sam's comments - data file reduced from 50k lines to 30k (179.88 KB, patch)
2009-07-28 19:13 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
third time's the charm - generate the list of rules dynamically (4.43 KB, patch)
2009-07-29 16:42 PDT, Dirk Pranke
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2009-07-27 18:10:52 PDT
originally reported as chromium bug #16755: https://crbug.com/16755

in CSSStyleSelector.h, ~CSSRuleData() deletes the next pointer in the linked list before itself, causing a frame to be stuffed onto the stack for every member of the list. For really long lists, this can cause a stack overflow.

Example (in the wild): http://www.jordanzad.com/jordan/index.php
This web page appears to load the same CSS file over and over again (probably due to a buggy code generator). 

The easiest fix is to change CSSRuleDataSet to delete the list iteratively. This fixes the size of the stack at one frame.

I have a simpler version as a test case that I will upload momentarily. I am currently playing around with it a bit to understand when CSSRuleData() elements are created, and when CSSRuleDataSets() are created, to see if there are better ways to handle this long-term.
Comment 1 Dirk Pranke 2009-07-27 19:19:43 PDT
Created attachment 33593 [details]
test case - html file that loads a large CSS file repeatedly
Comment 2 Dirk Pranke 2009-07-27 19:20:04 PDT
Created attachment 33594 [details]
test case - CSS file loaded by HTML file
Comment 3 Dirk Pranke 2009-07-27 19:21:13 PDT
test: download the two files, rename the CSS file to "resources/large-linked-file-crash.css", load the HTML file. Wait for crash :)
Comment 4 Dirk Pranke 2009-07-27 19:23:53 PDT
the XHTML DOCTYPE seems to perturb the loading of the page in such a way that the linked CSS file gets loaded repeatedly. I don't understand why this is. However, if you remove the DOCTYPE, you will instead get the crash when you navigate to another page (causing the destructor to run).

Note that CSSRuleData and CSSRuleDataSet don't need to be in CSSStyleSelector.h; I'll file a separate bug to move them into the .cpp file. I'm not clear that CSSRuleDataSet even needs to exist; you can probably replace it with a std::list.
Comment 5 Dirk Pranke 2009-07-27 19:46:37 PDT
cleanup bug filed as bug 27753.
Comment 6 Dirk Pranke 2009-07-27 19:59:34 PDT
Note that the file only appears to cause a crash on windows; not sure why Mac Safari isn't crashing.
Comment 7 Dirk Pranke 2009-07-27 20:16:23 PDT
Created attachment 33597 [details]
patch for crash
Comment 8 Sam Weinig 2009-07-27 20:52:03 PDT
Comment on attachment 33597 [details]
patch for crash

> -        ~CSSRuleDataList() { delete m_first; }
> +        ~CSSRuleDataList() { 

{ should be on the next line.

> +          CSSRuleData* ptr;
> +          CSSRuleData* next;
> +          ptr = m_first;
> +          while (ptr) {
> +            next = ptr->next();
> +            delete ptr;
> +            ptr = next;
> +          }
> +        }

This should use 4 spaces for indentation.

You have also marked the tests as executable which is not correct.  Is there no programatic way to test this using JS?

r-.
Comment 9 Darin Fisher (:fishd, Google) 2009-07-27 22:17:59 PDT
It seems like CSSStyleSheet.insertRule should be usable here to avoid a large'ish data file:
http://www.w3.org/TR/DOM-Level-2-Style/css.html#CSS-CSSStyleSheet

Create an empty <style> tag, and then use document.styleSheets[0].insertRule("a {}", 0) a bunch of times?
Comment 10 Dirk Pranke 2009-07-28 12:21:07 PDT
(In reply to comment #8)
> (From update of attachment 33597 [details])
> > -        ~CSSRuleDataList() { delete m_first; }
> > +        ~CSSRuleDataList() { 
> 
> { should be on the next line.
> 
> > +          CSSRuleData* ptr;
> > +          CSSRuleData* next;
> > +          ptr = m_first;
> > +          while (ptr) {
> > +            next = ptr->next();
> > +            delete ptr;
> > +            ptr = next;
> > +          }
> > +        }
> 
> This should use 4 spaces for indentation.
> 

Fixed both of these. I will upload a new patch shortly.
Comment 11 Dirk Pranke 2009-07-28 12:22:42 PDT
(In reply to comment #9)
> It seems like CSSStyleSheet.insertRule should be usable here to avoid a
> large'ish data file:
> http://www.w3.org/TR/DOM-Level-2-Style/css.html#CSS-CSSStyleSheet
> 
> Create an empty <style> tag, and then 
> use document.styleSheets[0].insertRule("a {}", 0) a bunch of times?

I'm not sure if this will follow the same code paths enough to provoke 
the bug. I'll try out a couple of variations to see what happens.

While the large data file is a bit of a pain, it does preserve the structure
of the file that caused the crash nicely.
Comment 12 Dirk Pranke 2009-07-28 19:13:54 PDT
Created attachment 33688 [details]
revied patch w/ sam's comments - data file reduced from 50k lines to 30k
Comment 13 Dirk Pranke 2009-07-28 19:17:37 PDT
(In reply to comment #11)
> (In reply to comment #9)
> > It seems like CSSStyleSheet.insertRule should be usable here to avoid a
> > large'ish data file:
> > http://www.w3.org/TR/DOM-Level-2-Style/css.html#CSS-CSSStyleSheet
> > 
> > Create an empty <style> tag, and then 
> > use document.styleSheets[0].insertRule("a {}", 0) a bunch of times?
> 
> I'm not sure if this will follow the same code paths enough to provoke 
> the bug. I'll try out a couple of variations to see what happens.
>

This appears to be several orders of magnitude slower than linking to a big data file. The test runs on the order of minutes rather than seconds. Note that you also get a slowdown if you were to include a 1 line file through 50k <link>s; I'm not sure which is slower.

Both of these slowdowns probably indicate a whole separate other problem, and I'll file a separate bug for that to dig into why these data structures need to be recreated in these situations.

A 30k line data file is enough to cause Chrome or Safari on Windows to crash, so I've reduced it to that size. I'm still not sure why they're not crashing on the Mac.
Comment 14 Sam Weinig 2009-07-28 22:24:13 PDT
How about just creating the string in JS and then creating an inline <style> tag and adding the string using the string as the content.

Something like:

var s = "a {}\n...";
var style = document.createElement("style");
style.appendChild(document.createTextNode(s));
document.getElementsByTagName("head")[0].appendChild(style);
Comment 15 Dirk Pranke 2009-07-28 22:30:36 PDT
(In reply to comment #14)
> How about just creating the string in JS and then creating an inline <style>
> tag and adding the string using the string as the content.
> 
> Something like:
> 
> var s = "a {}\n...";
> var style = document.createElement("style");
> style.appendChild(document.createTextNode(s));
> document.getElementsByTagName("head")[0].appendChild(style);

I will try that tomorrow; it'll be interesting to see if that's significantly faster than Darin's suggestion.
Comment 16 Dirk Pranke 2009-07-29 16:42:26 PDT
Created attachment 33753 [details]
third time's the charm - generate the list of rules dynamically
Comment 17 Dirk Pranke 2009-07-29 16:44:59 PDT
So, for reference, the initial all-text version run in a couple seconds. Darin's suggested version took two hours to run (so, 1000x slower). Sam's suggestion didn't actually work, but changing the text element to be a list of "a {}" rules did, and runs in only a slightly slower amount of time than the all-text version.

As to why this didn't crash on the Mac, it turns out that the Mac has much larger default stack. Upping the number of rules to 200,000 did the trick.

Hopefully this should resolve the issue.
Comment 18 Sam Weinig 2009-08-03 13:01:27 PDT
Fixed in r46728.