Bug 27748 – crash w/ stack overflow when same CSS file loaded repeatedly

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Dirk Pranke]

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.

[Dirk Pranke]

Created attachment 33593 [details]
test case - html file that loads a large CSS file repeatedly

[Dirk Pranke]

Created attachment 33594 [details]
test case - CSS file loaded by HTML file

[Dirk Pranke]

test: download the two files, rename the CSS file to "resources/large-linked-file-crash.css", load the HTML file. Wait for crash :)

[Dirk Pranke]

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.

[Dirk Pranke]

cleanup bug filed as bug 27753.

[Dirk Pranke]

Note that the file only appears to cause a crash on windows; not sure why Mac Safari isn't crashing.

[Dirk Pranke]

Created attachment 33597 [details]
patch for crash

[Sam Weinig]

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-.

[Darin Fisher (:fishd, Google)]

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?

[Dirk Pranke]

(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.

[Dirk Pranke]

(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.

[Dirk Pranke]

Created attachment 33688 [details]
revied patch w/ sam's comments - data file reduced from 50k lines to 30k

[Dirk Pranke]

(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.

[Sam Weinig]

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);

[Dirk Pranke]

(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.

[Dirk Pranke]

Created attachment 33753 [details]
third time's the charm - generate the list of rules dynamically

[Dirk Pranke]

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.

[Sam Weinig]

Fixed in r46728.