Enabling "Optimize CSS files" and drupal dies. It's a nasty bug.

Tested on XAMPP for Windows 1.7.1

To fix empty the cache table and set the preprocess_css variable back to 0

CommentFileSizeAuthor
#1 543892-1-css-aggregation.patch1.3 KBAnonymous (not verified)

Comments

Anonymous’s picture

Anonymous (not verified) commented
StatusFileSize
new 543892-1-css-aggregation.patch1.3 KB

The way css is preprocessed at the moment, all @imports in a css file are loaded, optimised.

Why is this a problem?
If a.css imports b.css which imports c.css, then c.css will be loaded and optimised. It will then replace the import statement in b.css. The resultant css (including the section that is already optimised) will be optimised, after which it will replace the import statement in a.css. At this point a.css (with the already optimised b.css and c.css) will be optimised before finally saving it. This means that many files may be processed multiple times with no benefit. In addition to this, it causes a large section of text to be sent to the regex (since all @imports are resolved.) This can cause a segfault in libpcre on some installations.

The patch?
Simply reorders the operations. The new order is optimise, fix the @imports. This means that each file is optimised only once, thus it keeps the size of the text sent to the regex from growing too large.

JacobSingh’s picture

JacobSingh commented
Status: Active » Reviewed & tested by the community

While this looks like too simple a solution, I'm tempted to say that it is the right patch.

I'm just surprised that no one has run into this yet.

Would like to get a review from the original author if possible.

mikeytown2’s picture

mikeytown2 commented

k give me 30 min or so, i'll test this out.

JacobSingh’s picture

JacobSingh commented
Status: Reviewed & tested by the community » Needs review

I guess some more review is coming, changing status.

mikeytown2’s picture

mikeytown2 commented
Status: Needs review » Needs work

Just checked and this does not fix the error I am having; going to upgrade to XAMPP 1.7.2 and try again.

mikeytown2’s picture

mikeytown2 commented

Still fails with 1.7.2
I get The connection to the server was reset while the page was loading.

JacobSingh’s picture

JacobSingh commented

@mikeytown2: Can you check your apache logs? Also does this happen only on your customized Drupal install or a fresh one?

In Joshua's case, it is only when he's got a ton of CSS files (and big ones), standard Drupal doesn't do this. Would like to see if it is a separate issue.

mikeytown2’s picture

mikeytown2 commented

Brand new install. Get from cvs, apply patch, and install in new database. Where would the apache logs be located? Checked files in xampplite\apache\logs and didn't find any interesting errors

mikeytown2’s picture

mikeytown2 commented

Here's the last entry in the access log - Me turning on css optimization 

127.0.0.1 - - [22/Sep/2009:00:27:55 -0700] "POST /drupal-7/admin/config/development/performance HTTP/1.1" 302 - "http://127.0.0.1/drupal-7/admin/config/development/performance" "Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.3) Gecko/20090824 Firefox/3.5.3 (.NET CLR 3.5.30729)"
JacobSingh’s picture

JacobSingh commented
Status: Needs work » Needs review

@mikeytown2: Your problem sounds like a different problem...

If you see nothing when css aggregation is turned on, it is most likely going to show up as some type of error in your access log, php error log or apache error log.

In Joshua's case (I know because we work together), he was getting Segfaults, and after debugging PHP he figured out it was in the libpcre library (preg_*) and that's how he traced it to this. But on his, normal Drupal installs worked fine - also, he's on Linux.

Best,
Jacob

JacobSingh’s picture

JacobSingh commented

Hmm... That's normal looking. That means that the resulting request didn't make it in your log, probably due to a PHP segfault. Is there a php error log in XAAMP?
-J

mikeytown2’s picture

mikeytown2 commented
Status: Needs review » Needs work

Yeah, php error log is empty... Going to comment out stuff, and locate the bug. The "not so fun way" to figure this out.

mikeytown2’s picture

mikeytown2 commented
Status: Needs work » Needs review

If I comment this out, then it works

function drupal_load_stylesheet_content($contents, $optimize = FALSE) {
  // Remove multiple charset declarations for standards compliance (and fixing Safari problems).
  $contents = preg_replace('/^@charset\s+[\'"](\S*)\b[\'"];/i', '', $contents);

//  if ($optimize) {
//    // Perform some safe CSS optimizations.
//    $contents = preg_replace('<
//      \s*([@{}:;,]|\)\s|\s\()\s* |  # Remove whitespace around separators, but keep space around parentheses.
//      /\*([^*\\\\]|\*(?!/))+\*/ |   # Remove comments that are not CSS hacks.
//      [\n\r]                        # Remove line breaks.
//      >x', '\1', $contents);
//  }

  // Replaces @import commands with the actual stylesheet content.
  // This happens recursively but omits external files.
  $contents = preg_replace_callback('/@import\s*(?:url\()?[\'"]?(?![a-z]+:)([^\'"\()]+)[\'"]?\)?;/', '_drupal_load_stylesheet', $contents);
  return $contents;
}

// Perform some safe CSS optimizations is causing the problems

mikeytown2’s picture

mikeytown2 commented
Status: Needs review » Needs work
mikeytown2’s picture

mikeytown2 commented

Looking at 6.x it appears that the regex was executed in a different order
http://api.drupal.org/api/function/drupal_load_stylesheet/6

mikeytown2’s picture

mikeytown2 commented

Order has nothing to do with it
This is the regex code that causes php to fail

      /\*([^*\\\\]|\*(?!/))+\*/ |   # Remove comments that are not CSS hacks.
mikeytown2’s picture

mikeytown2 commented
Title: Regression: Enable Optimize CSS files on performance page & Drupal dies » Win XAMPP 1.7.2 Regex PHP Bug - Optimize css & long css comments fails
Status: Needs work » Active

For some strange reason, if a css comment is >= 364 characters this code fails; only on windows platforms, using XAMPP (php 5.2.9 & 5.3.0).

      /\*([^*\\\\]|\*(?!/))+\*/ |   # Remove comments that are not CSS hacks.

Inside
5.x: http://api.drupal.org/api/function/drupal_build_css_cache/5
6.x: http://api.drupal.org/api/function/drupal_load_stylesheet/6
7.x: http://api.drupal.org/api/function/drupal_load_stylesheet_content/7

Bug is in 7.x inside the modules/system/system.css file, introduced with this commit
http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/system/system.css?...
Last css comment is over 363 characters

Confirm bug exists in 6.x & 7.x; most likely in 5.x as well.

Edit: Link to old PHP bug, that is very similar.
http://bugs.php.net/bug.php?id=38512

JacobSingh’s picture

JacobSingh commented

This might win random bug of the year award. It got down to tracking CVS commits, but seems to be somewhere in here when the long comments where committed. Perviously there wasn't any in Drupal Core.
http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/system/system.css?...

JacobSingh’s picture

JacobSingh commented

I think @MikeyTown2's issue is different one:

See #584370: Enable Optimize CSS files on performance page & Drupal dies for a rehash of what Joshua found - which is not just windows specific.

rct240’s picture

rct240 commented

DELETEME: After some testing my comment was proven erroneous.

mattyoung’s picture

mattyoung commented

.

donquixote’s picture

donquixote commented

If this helps to reproduce:
http://drupal.org/node/444228#comment-2089300

Totally a PHP / PCRE preg_replace bug.
I reduced it to a small standalone script.

And on PHP.net
http://bugs.php.net/bug.php?id=46551
(I hate the PHP bugtrack system)

Tri’s picture

Tri commented

Hi all.

Read more about this here. There are also patches against HEAD and 6.14.

Please test them and report back there.

mikeytown2’s picture

mikeytown2 commented
Status: Active » Closed (duplicate)

#444228: Optimize CSS option causes php cgi to segfault in pcre function "match"