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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

david_garcia created an issue. See original summary.

david_garcia’s picture

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

david_garcia’s picture

Status: Active » Needs review
david_garcia’s picture

And... looks like the YUI css compression is also choking on this style file.

mikeytown2’s picture

Title: advagg_split_css_file() can lead to infinite loop » advagg_split_css_file() can lead to infinite loop (needs test now)
Priority: Critical » Normal
Status: Needs review » Active

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

mikeytown2’s picture

http://jigsaw.w3.org/css-validator/validator found 106 errors with the big css file. Still looking for the trigger for the infinite loop

mikeytown2’s picture

Category: Bug report » Task
mikeytown2’s picture

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

mikeytown2’s picture

mikeytown2’s picture

Title: advagg_split_css_file() can lead to infinite loop (needs test now) » advagg_split_css_file() can lead to infinite loop; issue with large css files.

  • mikeytown2 committed befe561 on 7.x-2.x
    Issue #2852487 by mikeytown2: advagg_split_css_file() can lead to...
mikeytown2’s picture

Status: Needs review » Fixed

test code for future use

  module_load_include('inc', 'advagg', 'advagg');
  $file_info = array(
    'data' => 'main.css',
  );
  advagg_split_css_file($file_info);

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.