Problem/Motivation
Current advagg_split_css_file() can lead to inifinite loops when breaking the css into smaller files. We discovered this first a few months ago on a live site. It completely colapsed on production (as the integration tests we run do not have advagg enabled).
Proposed resolution
Revise the logic inside advagg_split_css_file() not to cause an infinite loop + add explicit protection for ininite loops in this loop:
while (array_key_exists($split_at, $new_css_chunk) === FALSE) {
--$split_at;
}
Boosted this to critical because it is "difficult" to diagnose and completely takes down a site. Finding out where an infinite loop is happening in PHP requires to setup a tracer and then analyze the trace file. Not that it's really difficult, but it can get complicated as these trace files will only be generated by some execution paths, and traces for infinite loops can grow to GB's in size (Actually it will grow until PHP reaches any sort of execution limit).
Remaining tasks
Review/work.
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#10 | advagg-2852487-10-better-css-split-algorithm.patch | 15.4 KB | mikeytown2 |
#2 | main.zip | 218.09 KB | david_garcia |
#2 | 2852487-fix-infinite-loop.patch | 1.27 KB | david_garcia |
Comments
Comment #2
david_garcia CreditAttribution: david_garcia commentedHere goes a patch. The truth is that I was quite unable to completely grasp what advagg_split_css_file() is doing as my knowledge about how CSS splitting works and the involved constraints is very limited.
An important part of the patch is the fact that an exception is thrown in case an infinite loop is detected.
Attached is the css file that makes advagg_split_css_file() collapse.
Comment #3
david_garcia CreditAttribution: david_garcia commentedComment #4
david_garcia CreditAttribution: david_garcia commentedAnd... looks like the YUI css compression is also choking on this style file.
Comment #6
mikeytown2 CreditAttribution: mikeytown2 commented5.6mb CSS file. Too big to add as a test... Going to see if I can get a smaller filesize and add to the test. For now this patch has been committed.
Comment #7
mikeytown2 CreditAttribution: mikeytown2 commentedhttp://jigsaw.w3.org/css-validator/validator found 106 errors with the big css file. Still looking for the trigger for the infinite loop
Comment #8
mikeytown2 CreditAttribution: mikeytown2 commentedComment #9
mikeytown2 CreditAttribution: mikeytown2 commentedOne of your css rules
margin-left: -1px;
in main.css (line 13165) has over 5,000 selectors. That seems highly unusual. Still working on the parser but thought I'd let you know in the mean time.Comment #10
mikeytown2 CreditAttribution: mikeytown2 commentedComment #11
mikeytown2 CreditAttribution: mikeytown2 commentedComment #13
mikeytown2 CreditAttribution: mikeytown2 commentedtest code for future use