Fix flaw in HTTPCache::Find where the fallback value does not get

jmarantz · jmarantz · commit bcf63acfd4a8 · 2016-01-14T10:11:19.000-05:00
decompressed even when the cached response is compressed and the request
does not have accept-encoding.  This was found by observing flakiness in
valgrind-system tests.

Fix flaw in the version of HTTPCache::Put that takes ResponseHeaders*
and mutates it unexpectedly, by adding compression headers.

Fix a flaw in InflatingFetch that Reset didn't reset all the cached
boolean bits.  I don't think this was the cause of anything in
production becasue we only use Reset in tests (where we re-use a Fetch
object).
diff --git a/net/instaweb/http/http_cache.cc b/net/instaweb/http/http_cache.cc
@@ -31,9 +31,9 @@
 #include "pagespeed/kernel/base/statistics.h"
 #include "pagespeed/kernel/base/string.h"
 #include "pagespeed/kernel/base/string_util.h"
-#include "pagespeed/kernel/base/string_writer.h"
 #include "pagespeed/kernel/base/timer.h"
 #include "pagespeed/kernel/cache/cache_interface.h"
+#include "pagespeed/kernel/http/content_type.h"
 #include "pagespeed/kernel/http/google_url.h"
 #include "pagespeed/kernel/http/http_names.h"
 #include "pagespeed/kernel/http/response_headers.h"
@@ -229,7 +229,17 @@ class HTTPCacheCallback : public CacheInterface::Callback {
               headers->IsProxyCacheable(callback_->req_properties(),
                                         callback_->RespectVaryOnResources(),
                                         ResponseHeaders::kHasValidator)) {
-            callback_->fallback_http_value()->Link(callback_->http_value());
+            ResponseHeaders fallback_headers;
+            if (callback_->request_context()->accepts_gzip() ||
+                !callback_->http_value()->ExtractHeaders(&fallback_headers,
+                                                         handler_) ||
+                !InflatingFetch::UnGzipValueIfCompressed(
+                    *callback_->http_value(), &fallback_headers,
+                    callback_->fallback_http_value(), handler_)) {
+              // If we don't need to unzip, or can't unzip, then just
+              // link the value and fallback together.
+              callback_->fallback_http_value()->Link(callback_->http_value());
+            }
           }
         }
       }
@@ -249,10 +259,14 @@ class HTTPCacheCallback : public CacheInterface::Callback {
     if (result_.status != HTTPCache::kFound) {
       headers->Clear();
       callback_->http_value()->Clear();
-    }
-    if (!callback_->request_context()->accepts_gzip()) {
-      HTTPValue* http_value = callback_->http_value();
-      InflatingFetch::UnGzipValueIfCompressed(http_value, headers, handler_);
+    } else if (!callback_->request_context()->accepts_gzip() &&
+               headers->IsGzipped()) {
+      HTTPValue new_value;
+      GoogleString inflated;
+      if (InflatingFetch::UnGzipValueIfCompressed(
+              *callback_->http_value(), headers, &new_value, handler_)) {
+        callback_->http_value()->Link(&new_value);
+      }
     }
     start_ms_ = now_ms;
     start_us_ = now_us;
@@ -380,7 +394,8 @@ HTTPValue* HTTPCache::ApplyHeaderChangesForPut(
   return value;
 }
 
-void HTTPCache::PutInternal(const GoogleString& key,
+void HTTPCache::PutInternal(bool preserve_response_headers,
+                            const GoogleString& key,
                             const GoogleString& fragment, int64 start_us,
                             HTTPValue* value, ResponseHeaders* response_headers,
                             MessageHandler* handler) {
@@ -394,13 +409,23 @@ void HTTPCache::PutInternal(const GoogleString& key,
   if (!value->Empty() && compression_level_ != 0) {
     const ContentType* type = response_headers->DetermineContentType();
     if ((type != NULL) && type->IsCompressible() &&
-        !response_headers->IsGzipped() &&
-        InflatingFetch::GzipValue(compression_level_, value, &compressed_value,
-                                  response_headers, handler)) {
-      // The resource is text (js, css, html, svg, etc.), and not previously
-      // compressed, so we'll compress it and stick the new compressed version
-      // in the cache.
-      value = &compressed_value;
+        !response_headers->IsGzipped()) {
+      ResponseHeaders* headers_to_gzip = response_headers;
+      ResponseHeaders headers_copy;
+      if (preserve_response_headers) {
+        headers_copy.CopyFrom(*response_headers);
+        headers_to_gzip = &headers_copy;
+      }
+      headers_to_gzip->ComputeCaching();
+
+      if (InflatingFetch::GzipValue(compression_level_, *value,
+                                    &compressed_value, headers_to_gzip,
+                                    handler)) {
+        // The resource is text (js, css, html, svg, etc.), and not previously
+        // compressed, so we'll compress it and stick the new compressed version
+        // in the cache.
+        value = &compressed_value;
+      }
     }
   }
   // TODO(jcrowell): prevent the unzip-rezip flow when sending compressed data
@@ -442,7 +467,8 @@ void HTTPCache::Put(const GoogleString& key, const GoogleString& fragment,
       start_us, NULL, &headers, value, handler);
   // Put into underlying cache.
   if (new_value != NULL) {
-    PutInternal(key, fragment, start_us, new_value, &headers, handler);
+    PutInternal(false /* preserve_response_headers */,
+                key, fragment, start_us, new_value, &headers, handler);
     if (cache_inserts_ != NULL) {
       cache_inserts_->Add(1);
     }
@@ -478,7 +504,8 @@ void HTTPCache::Put(const GoogleString& key, const GoogleString& fragment,
       ApplyHeaderChangesForPut(start_us, &content, headers, NULL, handler));
   // Put into underlying cache.
   if (value.get() != NULL) {
-    PutInternal(key, fragment, start_us, value.get(), headers, handler);
+    PutInternal(true /* preserve_response_headers */,
+                key, fragment, start_us, value.get(), headers, handler);
     if (cache_inserts_ != NULL) {
       cache_inserts_->Add(1);
     }
diff --git a/net/instaweb/http/http_cache_test.cc b/net/instaweb/http/http_cache_test.cc
@@ -187,6 +187,16 @@ class HTTPCacheTest : public testing::Test {
                      headers, content, handler);
   }
 
+  void PopulateGzippedEntry(const char* cache_control,
+                            ResponseHeaders* response_headers) {
+    InitHeaders(response_headers, cache_control);
+    response_headers->Add(HttpAttributes::kContentType, "text/css");
+    static const char kCssText[] = ".a         {color:blue;}                  ";
+    http_cache_->SetCompressionLevel(9);
+    response_headers->ComputeCaching();
+    Put(kUrl, kFragment, response_headers, kCssText, &message_handler_);
+  }
+
   scoped_ptr<ThreadSystem> thread_system_;
   SimpleStats simple_stats_;
   MockTimer mock_timer_;
@@ -329,8 +339,10 @@ TEST_F(HTTPCacheTest, StaticInflatingFetch) {
   EXPECT_TRUE(value.ExtractHeaders(&response_headers, NULL));
   // Check that the InflatingFetch gzip methods work properly when extracting
   // data.
-  InflatingFetch::UnGzipValueIfCompressed(&value, &response_headers, NULL);
-  ASSERT_TRUE(value.ExtractContents(&contents));
+  HTTPValue ungzipped;
+  ASSERT_TRUE(InflatingFetch::UnGzipValueIfCompressed(
+      value, &response_headers, &ungzipped, &message_handler_));
+  ASSERT_TRUE(ungzipped.ExtractContents(&contents));
   ASSERT_TRUE(meta_data_out.Lookup("name", &values));
   ASSERT_EQ(static_cast<size_t>(1), values.size());
   EXPECT_EQ(GoogleString("value"), *(values[0]));
@@ -717,7 +729,7 @@ TEST_F(HTTPCacheTest, OverrideCacheTtlMs) {
   // Now advance the time by 310 seconds and set override cache TTL to 300
   // seconds. The lookup fails.
   simple_stats_.Clear();
-  mock_timer_.AdvanceMs(310 * 1000);
+  mock_timer_.AdvanceMs(310 * Timer::kSecondMs);
   callback.reset(NewCallback());
   value.Clear();
   meta_data_in.Clear();
@@ -871,6 +883,59 @@ TEST_F(HTTPCacheTest, UpdateVersion) {
             Find(kUrl, kFragment, &value, &meta_data_out, &message_handler_));
 }
 
+TEST_F(HTTPCacheTest, NoMutateOnPut) {
+  ResponseHeaders response_headers;
+  PopulateGzippedEntry("max-age=300", &response_headers);
+
+  // The value that we have in the cache is compressed, but that should
+  // not cause a mutation in the response headers passed into Put.
+  EXPECT_FALSE(response_headers.HasValue(
+      HttpAttributes::kContentEncoding, "gzip"));
+}
+
+TEST_F(HTTPCacheTest, DecompressFallbackValue) {
+  ResponseHeaders response_headers;
+  PopulateGzippedEntry("max-age=300", &response_headers);
+
+  mock_timer_.AdvanceMs(310 * Timer::kSecondMs);  // Makes entry stale.
+  response_headers.Clear();
+
+  // If we don't have gzip in the request, it should not be in the fallback
+  // value.
+  scoped_ptr<Callback> callback(NewCallback());
+  HTTPValue value;
+  EXPECT_EQ(kNotFoundResult, FindWithCallback(  // Not found because it's stale.
+      kUrl, kFragment, &value, &response_headers, &message_handler_,
+      callback.get()));
+  EXPECT_TRUE(callback->fallback_http_value()->ExtractHeaders(
+      &response_headers, &message_handler_));
+  EXPECT_FALSE(response_headers.HasValue(
+      HttpAttributes::kContentEncoding, "gzip"));
+}
+
+TEST_F(HTTPCacheTest, LeaveFallbackCompressed) {
+  ResponseHeaders response_headers;
+  PopulateGzippedEntry("max-age=300", &response_headers);
+
+  mock_timer_.AdvanceMs(310 * Timer::kSecondMs);  // Makes entry stale.
+  response_headers.Clear();
+
+  // When we enable gzip in the request, though, we will get it because the
+  // value stored in the cache is gzipped.
+  RequestContextPtr request_context(RequestContext::NewTestRequestContext(
+      thread_system_.get()));
+  request_context->SetAcceptsGzip(true);
+  Callback callback(request_context);
+  HTTPValue value;
+  EXPECT_EQ(kNotFoundResult, FindWithCallback(
+      kUrl, kFragment, &value, &response_headers, &message_handler_,
+      &callback));
+  EXPECT_TRUE(callback.fallback_http_value()->ExtractHeaders(
+      &response_headers, &message_handler_));
+  EXPECT_TRUE(response_headers.HasValue(
+      HttpAttributes::kContentEncoding, "gzip"));
+}
+
 class HTTPCacheWriteThroughTest : public HTTPCacheTest {
  protected:
   // Unlike HTTPCacheTest::Callback this can produce different validity for
diff --git a/net/instaweb/http/inflating_fetch.cc b/net/instaweb/http/inflating_fetch.cc
@@ -30,10 +30,8 @@
 namespace net_instaweb {
 
 InflatingFetch::InflatingFetch(AsyncFetch* fetch)
-  : SharedAsyncFetch(fetch),
-    request_checked_for_accept_encoding_(false),
-    compression_desired_(false),
-    inflate_failure_(false) {
+    : SharedAsyncFetch(fetch) {
+  Reset();
 }
 
 InflatingFetch::~InflatingFetch() {
@@ -104,15 +102,16 @@ bool InflatingFetch::HandleWrite(const StringPiece& sp,
   return status && !inflate_failure_;
 }
 
-// Inflate a HTTPValue, if it was gzip compressed, in place.
-void InflatingFetch::UnGzipValueIfCompressed(HTTPValue* http_value,
+// Inflate a HTTPValue, if it was gzip compressed.
+bool InflatingFetch::UnGzipValueIfCompressed(const HTTPValue& src,
                                              ResponseHeaders* headers,
+                                             HTTPValue* dest,
                                              MessageHandler* handler) {
-  if (!http_value->Empty() && headers->IsGzipped()) {
+  if (!src.Empty() && headers->IsGzipped()) {
     GoogleString inflated;
     StringWriter inflate_writer(&inflated);
     StringPiece content;
-    http_value->ExtractContents(&content);
+    src.ExtractContents(&content);
     if (GzipInflater::Inflate(content, GzipInflater::kGzip, &inflate_writer)) {
       if (!headers->HasValue(HttpAttributes::HttpAttributes::kVary,
                              HttpAttributes::kAcceptEncoding)) {
@@ -124,22 +123,23 @@ void InflatingFetch::UnGzipValueIfCompressed(HTTPValue* http_value,
       headers->Replace(HttpAttributes::kContentLength,
                        Integer64ToString(inflated.length()));
       content.set(inflated.c_str(), inflated.length());
-      http_value->Clear();
-      http_value->Write(content, handler);
-      http_value->SetHeaders(headers);
+      dest->Write(content, handler);
+      dest->SetHeaders(headers);
+      return true;
     }
   }
+  return false;
 }
 
 bool InflatingFetch::GzipValue(int compression_level,
-                               const HTTPValue* http_value,
+                               const HTTPValue& http_value,
                                HTTPValue* compressed_value,
                                ResponseHeaders* headers,
                                MessageHandler* handler) {
   StringPiece content;
   GoogleString deflated;
   int64 content_length;
-  http_value->ExtractContents(&content);
+  http_value.ExtractContents(&content);
   StringWriter deflate_writer(&deflated);
   if (!headers->IsGzipped() &&
       GzipInflater::Deflate(content, GzipInflater::kGzip, compression_level,
@@ -217,8 +217,10 @@ void InflatingFetch::Reset() {
   if (inflater_.get() != NULL) {
     inflater_->ShutDown();
     inflater_.reset(NULL);
-    inflate_failure_ = false;
   }
+  request_checked_for_accept_encoding_ = false;
+  compression_desired_ = false;
+  inflate_failure_ = false;
   SharedAsyncFetch::Reset();
 }
 
diff --git a/net/instaweb/http/inflating_fetch_test.cc b/net/instaweb/http/inflating_fetch_test.cc
@@ -315,7 +315,7 @@ TEST(StaticInflatingFetchTest, CompressUncompressValue) {
   headers.Add(HttpAttributes::kContentType, "text/html");
   value.SetHeaders(&headers);
   HTTPValue compressed_value;
-  EXPECT_TRUE(InflatingFetch::GzipValue(9, &value, &compressed_value, &headers,
+  EXPECT_TRUE(InflatingFetch::GzipValue(9, value, &compressed_value, &headers,
                                         &handler));
   StringPiece contents;
   compressed_value.ExtractContents(&contents);
@@ -325,9 +325,10 @@ TEST(StaticInflatingFetchTest, CompressUncompressValue) {
   EXPECT_STREQ(HttpAttributes::kGzip,
                headers.Lookup1(HttpAttributes::kContentEncoding));
   compressed_value.ExtractHeaders(&headers, &handler);
-  InflatingFetch::UnGzipValueIfCompressed(&compressed_value, &headers,
-                                          &handler);
-  compressed_value.ExtractContents(&contents);
+  HTTPValue uncompressed_value;
+  ASSERT_TRUE(InflatingFetch::UnGzipValueIfCompressed(
+      compressed_value, &headers, &uncompressed_value, &handler));
+  uncompressed_value.ExtractContents(&contents);
   // We've unzipped the compressed value, it should now say "hello".
   EXPECT_EQ(kHello, contents);
 }
diff --git a/net/instaweb/http/public/http_cache.h b/net/instaweb/http/public/http_cache.h
@@ -25,7 +25,7 @@
 #include "net/instaweb/http/public/request_context.h"
 #include "pagespeed/kernel/base/atomic_bool.h"
 #include "pagespeed/kernel/base/basictypes.h"
-#include "pagespeed/kernel/base/gtest_prod.h"  // for FRIEND_TEST
+#include "pagespeed/kernel/base/gtest_prod.h"
 #include "pagespeed/kernel/base/ref_counted_ptr.h"
 #include "pagespeed/kernel/base/string.h"
 #include "pagespeed/kernel/base/string_util.h"
@@ -365,7 +365,8 @@ class HTTPCache {
 
   // If headers is passed as NULL, the response headers will be extracted from
   // the HTTPValue. Otherwise, the headers passed in will be used.
-  void PutInternal(const GoogleString& key,
+  void PutInternal(bool preserve_response_headers,
+                   const GoogleString& key,
                    const GoogleString& fragment,
                    int64 start_us,
                    HTTPValue* value,
diff --git a/net/instaweb/http/public/inflating_fetch.h b/net/instaweb/http/public/inflating_fetch.h
@@ -20,9 +20,11 @@
 #define NET_INSTAWEB_HTTP_PUBLIC_INFLATING_FETCH_H_
 
 #include "net/instaweb/http/public/async_fetch.h"
+#include "net/instaweb/http/public/http_value.h"
 #include "pagespeed/kernel/base/basictypes.h"
 #include "pagespeed/kernel/base/scoped_ptr.h"
 #include "pagespeed/kernel/base/string_util.h"
+#include "pagespeed/kernel/http/response_headers.h"
 #include "pagespeed/kernel/util/gzip_inflater.h"
 
 namespace net_instaweb {
@@ -49,15 +51,20 @@ class InflatingFetch : public SharedAsyncFetch {
   // or gzip was already in the request then this has no effect.
   void EnableGzipFromBackend();
 
-  // In-place inflate a GZipped HTTPValue if it has been gzipped-compressed,
-  // updating the headers to reflect the new state.
-  static void UnGzipValueIfCompressed(HTTPValue* http_value,
+  // Inflate a GZipped HTTPValue if it has been gzipped-compressed,
+  // updating the headers to reflect the new state.  Returns false if
+  // the data was not compressed, leaving dest unmodified.
+  //
+  // Notes: dest and src should not be the same object.  If the
+  // unzip fails, you may need to link src into dest.
+  static bool UnGzipValueIfCompressed(const HTTPValue& src,
                                       ResponseHeaders* headers,
+                                      HTTPValue* dest,
                                       MessageHandler* handler);
   // GZip compress HTTPValue, updating the headers reflect the new
   // state, output to compressed_value. Returns true if the value is
   // successfully compressed.
-  static bool GzipValue(int compression_level, const HTTPValue* http_value,
+  static bool GzipValue(int compression_level, const HTTPValue& http_value,
                         HTTPValue* compressed_value, ResponseHeaders* headers,
                         MessageHandler* handler);
 
diff --git a/net/instaweb/rewriter/rewrite_context.cc b/net/instaweb/rewriter/rewrite_context.cc
@@ -3121,11 +3121,14 @@ void RewriteContext::FetchFallbackCacheDone(HTTPCache::FindResult result,
   scoped_ptr<HTTPCache::Callback> cleanup_callback(data);
 
   StringPiece contents;
+  ResponseHeaders* response_headers = data->response_headers();
   if ((result.status == HTTPCache::kFound) &&
       data->http_value()->ExtractContents(&contents) &&
-      (data->response_headers()->status_code() == HttpStatus::kOK)) {
+      (response_headers->status_code() == HttpStatus::kOK)) {
+    DCHECK(!response_headers->IsGzipped() ||
+           Driver()->request_context()->accepts_gzip());
     // We want to serve the found result, with short cache lifetime.
-    fetch_->FetchFallbackDone(contents, data->response_headers());
+    fetch_->FetchFallbackDone(contents, response_headers);
   } else {
     StartFetchReconstruction();
   }
diff --git a/pagespeed/automatic/proxy_interface_test.cc b/pagespeed/automatic/proxy_interface_test.cc
