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

[jeffkaufman]

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]

@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]

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]

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

[jeffkaufman]

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]

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

[jeffkaufman]

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]

@jeffkaufman I'll look into it

[oschaaf]

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 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]

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]

@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]

Yes, this sounds good.

A test and PR sound great!