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

Parsing complex CSS can fail, leaving cache in an inconsistent state. #1092

Closed
jeffkaufman opened this Issue Jun 10, 2015 · 12 comments

Comments

Projects
None yet
3 participants
@jeffkaufman
Copy link
Contributor

jeffkaufman commented Jun 10, 2015

In CssHierarchy::RollUpContents, if minified_contents_ was set externally, without Parse()
running, then it's possible it contains something incompatible with flattening. The problem is something like flattening_failed_ not being saved in our cache. This was causing #1068, but then 8f93d5c fixed that, so I don't think this is visible externally, but our css filter is still doing something incorrect.

@sachinjsk

This comment has been minimized.

Copy link

sachinjsk commented Oct 2, 2015

@jeffkaufman I just tested out #1068 with v1.9.32.6. "Works in Chrome but not in Firefox' issue is fixed. But I still see the flattening css issue. If you try out http://www.enjoy-swimming.sbi2b.sitesell.com you'll see that the page is broken because of the css rewrite fail.

Happy to assist if you need anything from me to get this issue fixed.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Oct 5, 2015

Have you cleared your cache since upgrading? I think you may still have an (incorrectly) empty resource in cache from the previous version. https://developers.google.com/speed/pagespeed/module/system#flush_cache

@sachinjsk

This comment has been minimized.

Copy link

sachinjsk commented Oct 5, 2015

I did. I went in and deleted everything in the cache directory, just to be sure. I still see the issue.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Oct 5, 2015

Did you also touch cache.flush? Or did you delete things while pagespeed wasn't running? Simply deleting things while pagespeed is running isn't enough because pagespeed does some in-memory caching.

@sachinjsk

This comment has been minimized.

Copy link

sachinjsk commented Oct 5, 2015

Purged entire cache via pagespeed admin. I can still see it happening. Its random though. Only certain rewrites fail.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Oct 5, 2015

Ok, thanks!

@oschaaf would you be able to look into this? It sounds like empty resources are not being fully excluded, and we could either fix that here or by fixing the way we handle flattening failing.

@oschaaf oschaaf self-assigned this Oct 5, 2015

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented Oct 5, 2015

@jeffkaufman I'll look into it

oschaaf added a commit that referenced this issue Oct 5, 2015

CssImageRewriter: Flag failed flattening when ExpendChildren fails
I need to set up a repro to confirm, but I suspect this fixes #1092
@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented Oct 6, 2015

Still looking into this. Observations so far:

  • I can reproduce this on my machine with ngx_pagespeed linked to PSOL's version on master using the index and css from http://www.enjoy-swimming.sbi2b.sitesell.com
  • The issue seems to start happening 5 minutes after the first page load (refresh of the css input resource?), after which it persists.
  • Testing IPRO on master.css with an empty cache and freshly started server: The first 5 minutes things look as expected. After that I'm receiving three byte-long responses:
oschaaf@ubuntu:/tmp/npstest2⟫ curl --silent http://192.168.185.100:8060/master.css | hexdump
0000000 bbef 00bf                              
0000003

A similar byte sequence seems to get stored in cache for the resource:

1 oschaaf@ubuntu:/tmp/npstest2⟫ cat ./v2/192.168.185.100/http,3A/,2F192.168.185.100,3A8060/A.master.css.pagespeed.cf.7KqI9_oL9h.css, | hexdump                                                                                                                                   
0000000 .. ef00 bfbb .....

Perhaps the byte order mark is the reason that 8f93d5c didn't prevent the issue here as it it looks like the content here is non-zero length due to the byte order mark.
Perhaps more important, I still need to figure out why the @import rule is dropped after pagespeed refreshes the underlying file (flattening_failed_ somehow not surviving the refresh, as @jeffkaufman states?).

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented Oct 7, 2015

This minimal patch passes tests, and fixes the issue:

diff --git a/net/instaweb/rewriter/public/css_flatten_imports_context.h b/net/instaweb/rewriter/public/css_flatten_imports_context.h
index d60b8fd..f7cec85 100644
--- a/net/instaweb/rewriter/public/css_flatten_imports_context.h
+++ b/net/instaweb/rewriter/public/css_flatten_imports_context.h
@@ -186,6 +186,9 @@ class CssFlattenImportsContext : public SingleRewriteContext {
         hierarchy_->set_minified_contents(
             output_partition(0)->inlined_data());
         hierarchy_->set_input_contents(hierarchy_->minified_contents());
+        // Parse() will compute flattening_succeeded_, which needs to be restored
+        // See https://github.com/pagespeed/mod_pagespeed/issues/1092
+        hierarchy_->Parse();
       }
     } else {
       // Something has gone wrong earlier. It could be that the resource is

@jeffkaufman what do you think, would this be acceptable performance-wise? Or would it be better to persist state in cache for the flattening_failed_ flag and restore it based on that?

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Oct 7, 2015

When does this code run? Just when optimizing the resource, right? In which case the performance implications should be minimal.

Can we verify that there's no way to get into this on a per-request basis?

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented Oct 7, 2015

@jeffkaufman I tested manually earlier, and the code only fired when optimizing the resource.
If this sounds good, I'll look into adding a test and create a pull request after that.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Oct 7, 2015

Yes, this sounds good.

A test and PR sound great!

oschaaf added a commit that referenced this issue Oct 7, 2015

oschaaf added a commit that referenced this issue Oct 7, 2015

oschaaf added a commit that referenced this issue Oct 7, 2015

@jeffkaufman jeffkaufman changed the title flattening_failed_ not being preserved properly Parsing complex CSS can fail, leaving cache in an inconsistent state. Nov 13, 2015

@pono pono unassigned oschaaf Jan 8, 2018

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