Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memory leak issue injecting invalid CSS into view #5912

Closed
chasegiunta opened this issue Apr 12, 2020 · 11 comments
Closed

Memory leak issue injecting invalid CSS into view #5912

chasegiunta opened this issue Apr 12, 2020 · 11 comments

Comments

@chasegiunta
Copy link
Contributor

Description

Though this memory leak issue has shown up with a particular plugin (CPCSS), it's most likely a deeper issue with Craft CMS and/or Yii, so raising it here.

It's not difficult to recreate. All we need is the CPCSS plugin and some text input into one of it's field settings.

Steps to reproduce

See doublesecretagency/craft-cpcss#12

Additional info

  • Craft version: 3.4.15
  • PHP version: 7.3.13
  • Database driver & version: MySQL 5.5.5
  • Plugins & versions: Control Panel CSS 2.2.1
@Mosnar
Copy link
Contributor

Mosnar commented Apr 12, 2020

As of Craft 3.4, CSS injected into the View is minified automatically when appropriate. It seems like there was similar behavior when unclosed comments were encountered by the CSS minification library:
tubalmartin/YUI-CSS-compressor-PHP-port#23

@lindseydiloreto
Copy link
Contributor

@Mosnar That certainly aligns with what I'm seeing...

Ω 2020-04-12 at 3 54 08 PM

@lindseydiloreto
Copy link
Contributor

It seems like the minification author isn't interested in fixing it. Does Craft itself need to be more defensive against broken CSS code?

@chasegiunta chasegiunta changed the title Memory leak issue with project config reading or improper writing Memory leak issue injecting invalid CSS into view Apr 12, 2020
@brandonkelly
Copy link
Member

It seems like the minification author isn't interested in fixing it.

The maintainer claims that the issue was fixed in versions 2.4.8-p10 and 3.0.0. Craft is pulling in 4.0.0+, via mrclay/minify so should be getting that fix.

That said, it appears that the issue was fixed via tubalmartin/YUI-CSS-compressor-PHP-port@210b058, which really is fixing a bug where changing the PHP memory_limit at runtime wasn’t working correctly. Which suggests that there’s no actual bug with minifying invalid CSS, besides that it tends to take more memory to minify than valid CSS.

So @chasegiunta I’m guessing in your case the issue is that PHP isn’t allowed to change its config settings at runtime. Please help verify by changing PHP’s memory_limit to -1 (no limit), restart your web server, and see if that fixes your issue.

@chasegiunta
Copy link
Contributor Author

chasegiunta commented Apr 14, 2020

Well this was fun to watch php-fpm slowly climb the memory charts over the course of the request. Eventually nginx threw a gateway timeout error but php's memory kept climbing until around 4.9 GB when something bailed. Whatever process it tries, never finishes, and thus the CP remains inaccessible.

Screen Shot 2020-04-14 at 10 31 13 AM

@brandonkelly
Copy link
Member

@chasegiunta Thanks. Able to reproduce here as well, and just posted an issue about it (tubalmartin/YUI-CSS-compressor-PHP-port#58) as well as a PR that fixes it (tubalmartin/YUI-CSS-compressor-PHP-port#59).

@chasegiunta
Copy link
Contributor Author

Thanks @brandonkelly! Is that a similar PR fix to this one? tubalmartin/YUI-CSS-compressor-PHP-port#49 . Based on the author's activity, we may not see anything merged in anytime soon 😬. Would using a fork be an option?

@brandonkelly
Copy link
Member

Oh, missed that. And yeah, the maintainer does seem to be MIA. Just added a guard against unclosed CSS bodies to Craft, which fixes this for the next release.

@lindseydiloreto
Copy link
Contributor

Brilliant, thanks @brandonkelly! 🍺

@brandonkelly
Copy link
Member

Craft 3.4.16 is out now with that change.

@LukeTowers
Copy link

LukeTowers commented Feb 22, 2022

FYI @assetic-php & @wintercms will be switching to https://github.com/wikimedia/minify since that is actively maintained, might be worth looking into.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants