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

Empty resources should not be cached #1068

Closed
sligocki opened this Issue Apr 22, 2015 · 11 comments

Comments

Projects
None yet
3 participants
@sligocki
Copy link
Contributor

sligocki commented Apr 22, 2015

Splitting this new issue off of discussion on Issue #1050:

From @jeffkaufman:

Simple test case: http://www.jefftk.com/simpler-include-css/

$ cat index.html 
<style>body {background: red}</style>
<link rel=stylesheet href="a.css" type="text/css"/>

$ cat a.css 
@import url(b.css) ;

$ cat b.css 
body {background: green}

@media screen and (min-width: 240px) {
  body {background: blue}
}

To get the failure:

  1. Pick a unique set of irrelevant PageSpeed query options to functionally clear the metadata cache. So ?PageSpeedFilters=+[something_silly].
  2. Open that URL in Chrome or FF, it should work, green screen.
  3. Open that URL in the other one, it shouldn't work, red screen.

On the passing side +debug gives me:

<style>@import url(support-files/rwd-v2.css) ;</style><!--Flattening failed:
A media query is too complex in http://www.jefftk.com/simple-include-css
/support-files/rwd-v2.css AND Cannot rewrite http://cfd.enjoy-swimming.
com/image-files-layout/rwd-header.png as it is on an unauthorized domain-->

On the failing side:

<style></style><!--Flattening failed: A media query is too complex in
http://www.jefftk.com/simple-include-css/support-files/rwd-v2.css-->
@sligocki

This comment has been minimized.

Copy link
Contributor

sligocki commented Apr 22, 2015

@sachinjsk is the original reporter.

@sligocki sligocki self-assigned this Apr 22, 2015

@sligocki sligocki added this to the 1.9.32.4 milestone Apr 22, 2015

@sligocki sligocki assigned jeffkaufman and unassigned sligocki Apr 22, 2015

@sachinjsk

This comment has been minimized.

Copy link

sachinjsk commented Apr 22, 2015

Is there a particular filter I could turn off to get around this issue?

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Apr 22, 2015

@sachinjsk

This comment has been minimized.

Copy link

sachinjsk commented Apr 22, 2015

Thanks @jeffkaufman. That didn't quite work. I still see a empty rewritten css cache file. Please note that I also had inline_css turned off.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Apr 23, 2015

Could you paste the config change you made? When I test it here I can reliably reproduce with flatten_css_imports enabled and can't elicit the bug at all with it disabled.

(This isn't a case where flushing the cache matters; configuration options are used in the cache key for the metadata cache, and this is a metadata cache issue.)

@sligocki

This comment has been minimized.

Copy link
Contributor

sligocki commented Apr 23, 2015

Looks to me like http://www.enjoy-swimming.sbi2b.sitesell.com/?ModPagespeedFilters=+debug does not have the bug and http://www.enjoy-swimming.sbi2b.sitesell.com/?ModPagespeedFilters=+debug,+flatten_css_imports does have the bug.

@sachinjsk, do you mean that the cache file is still on your hard disk? That shouldn't be a problem.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Apr 23, 2015

Hypothesis: the value of flattening_suceeded_ isn't getting stored in the cache, so that if a.css imports b.css, the rewritten a.css isn't in cache, but the rewritten a.css is, then we use the cached a.css as if flattening_suceeded_ even if it didn't.

@sachinjsk

This comment has been minimized.

Copy link

sachinjsk commented Apr 23, 2015

@sligocki: No, I meant the css cache file that mod_pagespeed generated was empty.

@jeffkaufman: I just tried this again. But now the page loads fine with flatten_css_imports disabled. I can't seem to reproduce what I was seeing yesterday. Yesterday the cache file that was getting generated for main.css was empty. Now I see @import url(rwd-v2.css); in it. (test url: http://www.enjoy-swimming.sbi2b.sitesell.com/?ModPagespeed=on)

Here are the changes I made yesterday:
ModPagespeedDisableFilters inline_css
ModPagespeedDisableFilters flatten_css_imports

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Apr 27, 2015

@sachinjsk Thanks! I'm still working on this here. If you see this issue anymore now that you have DisableFilters flatten_css_imports please let me know, because that really shouldn't happen. You can remove the DisableFilters inline_css if you like.

@sachinjsk

This comment has been minimized.

Copy link

sachinjsk commented Apr 27, 2015

I will. Thanks for looking into this.

On Mon, 27 Apr 2015 5:39 am Jeff Kaufman notifications@github.com wrote:

@sachinjsk https://github.com/sachinjsk Thanks! I'm still working on
this here. If you see this issue anymore now that you have DisableFilters
flatten_css_imports please let me know, because that really shouldn't
happen. You can remove the DisableFilters inline_css if you like.


Reply to this email directly or view it on GitHub
#1068 (comment)
.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jun 10, 2015

This was fixed as a side-effect of 8f93d5c, which made us more cautious around empty resources. Our css optimizer still is not correctly handling cases where flattening fails, incorrectly putting an empty resource in the cache, but we recover because we no longer treat empty resources as valid.

Closing this because users won't run into the "works on chrome but not firefox" problem anymore, but opening #1092 because there's still a bug in the css flattening.

@jeffkaufman jeffkaufman changed the title CSS blanked on Firefox, but not Chrome (or vice versa) CSS removed with complex media queries. Jul 27, 2015

@jeffkaufman jeffkaufman changed the title CSS removed with complex media queries. Empty resources should not be cached Jul 27, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment