trunk-tracking: update from r3715 to r3736

Squash-merge of Jan's #608 and Otto's #611. * r3726: * Updated closure compiler flags for static JS files. * r3729: * Centralize parsing of FetchHttps in SystemRewriteOptions so ngx_pagespeed can get it too. * To keep the helpful error_message from SerfUrlAsyncFetcher, wire it through RewriteOptions as a new-fangled error_detail. * r3735: * Follow-up changes for downstream caching integration with beaconing dependent filters: If a downstream cache rebeaconing key is configured, we should instrument the page only if the key present in the PS-ShouldBeacon header matches the one in the configuration. This allows us to send no-cache headers for anything that carries the right beaconing key, and continue to send out the original cache control headers in other cases where downstream caching is enabled. * Native fetcher: fortify handling of content length (and absense). * Native fetcher: fail when the stream terminates before having completely parsed the headers. * Tests: Rename `test_filter` -> `start_test` in ngx_system_test.sh for a test. * Tests: Move blockingrewrite key to the http {} block. * Tests: Update localhost -> 127.0.0.1. The native fetcher uses dns to resolve, and won't be able to retreive an ip for localhost. * Tests: Allow outstanding proxy fetches some time to finish when running under valgrind, before terminating nginx. * Valgrind: Add suppressions to make testing a release build pass. This pull update was extra work because the valgrind and native fetcher flows had rotted a bit. We need to make sure to test them with every update.
2014-01-31 10:07:36 -05:00
parent 4783144e7d
commit f5252b569a
6 changed files with 149 additions and 74 deletions
@@ -70,7 +70,8 @@ namespace net_instaweb {
        fetch_start_ms_(0),
        fetch_end_ms_(0),
        done_(false),
-        content_length_(0) {
+        content_length_(-1),
+        content_length_known_(false) {
            ngx_memzero(&url_, sizeof(url_));
            log_ = log;
            pool_ = NULL;
@@ -484,9 +485,16 @@ namespace net_instaweb {
      }

      if (n == 0) {
-        // connection is closed prematurely by remote server,
-        // or the content-length was 0
-        fetch->CallbackDone(fetch->content_length_ == 0);
+        // If the content length was not known, we assume that we have read
+        // all if we at least parsed the headers.
+        // If we do know the content length, having a mismatch on the bytes read
+        // will be interpreted as an error.
+        if (fetch->content_length_known_) {
+          fetch->CallbackDone(fetch->content_length_ == fetch->bytes_received_);
+        } else {
+          fetch->CallbackDone(fetch->parser_.headers_complete());
+        }
+
        return;
      } else if (n > 0) {
        fetch->in_->pos = fetch->in_->start;
@@ -553,13 +561,21 @@ namespace net_instaweb {
    if (n > size) {
      return false;
    } else if (fetch->parser_.headers_complete()) {
-      int64 content_length = -1;
-      fetch->async_fetch_->response_headers()->FindContentLength(
-          &content_length);
-      fetch->content_length_ = content_length;
-      if (fetch->fetcher_->track_original_content_length()) {
+      if (fetch->async_fetch_->response_headers()->FindContentLength(
+              &fetch->content_length_)) {
+        if (fetch->content_length_ < 0) {
+          fetch->message_handler_->Message(
+              kError, "Negative content-length in response header");
+          return false;
+        } else {
+          fetch->content_length_known_ = true;
+        }
+      }
+
+      if (fetch->fetcher_->track_original_content_length()
+         && fetch->content_length_known_) {
        fetch->async_fetch_->response_headers()->SetOriginalContentLength(
-            content_length);
+            fetch->content_length_);
      }

      fetch->in_->pos += n;
@@ -578,11 +594,12 @@ namespace net_instaweb {
      return true;
    }

-    fetch->bytes_received_add(static_cast<int64>(size));
+    fetch->bytes_received_add(size);
+
    if (fetch->async_fetch_->Write(StringPiece(data, size),
        fetch->message_handler())) {
-      fetch->content_length_ -= size;
-      if (fetch->content_length_ <= 0) {
+      if (fetch->content_length_known_ &&
+          fetch->bytes_received_ == fetch->content_length_) {
        fetch->done_ = true;
      }
      return true;
@@ -121,12 +121,13 @@ class NgxFetch : public PoolElement<NgxFetch> {
  AsyncFetch* async_fetch_;
  ResponseHeadersParser parser_;
  MessageHandler* message_handler_;
-  size_t bytes_received_;
+  int64 bytes_received_;
  int64 fetch_start_ms_;
  int64 fetch_end_ms_;
  int64 timeout_ms_;
  bool done_;
  int64 content_length_;
+  bool content_length_known_;

  struct sockaddr_in sin_;
  ngx_log_t* log_;
@@ -1687,12 +1687,25 @@ ngx_int_t ps_resource_handler(ngx_http_request_t* r, bool html_rewrite) {
    ctx->in_place = false;
    ctx->pagespeed_connection = NULL;
    // See build_context_for_request() in mod_instaweb.cc
+    // TODO(jefftk): Is this the right place to be modifying caching headers for
+    // html fetches?  Or should that be done later, in the headers flow for
+    // filter mode, rather than here in resource fetch mode?
    if (!options->modify_caching_headers()) {
      ctx->preserve_caching_headers = kPreserveAllCachingHeaders;
-    } else if (!options->downstream_cache_purge_location_prefix().empty()) {
-      ctx->preserve_caching_headers = kPreserveOnlyCacheControl;
-    } else {
+    } else if (!options->IsDownstreamCacheIntegrationEnabled()) {
+      // Downstream cache integration is not enabled. Disable original
+      // Cache-Control headers.
      ctx->preserve_caching_headers = kDontPreserveHeaders;
+    } else {
+      ctx->preserve_caching_headers = kPreserveOnlyCacheControl;
+      // Downstream cache integration is enabled. If a rebeaconing key has been
+      // configured and there is a ShouldBeacon header with the correct key,
+      // disable original Cache-Control headers so that the instrumented page is
+      // served out with no-cache.
+      StringPiece should_beacon(request_headers->Lookup1(kPsaShouldBeacon));
+      if (options->MatchesDownstreamCacheRebeaconingKey(should_beacon)) {
+        ctx->preserve_caching_headers = kDontPreserveHeaders;
+      }
    }
    ctx->recorder = NULL;

@@ -398,7 +398,6 @@ check_from "$OUT" grep "$EXPECTED_EXAMPLES_TEXT"

 # And also with bad request headers.
 OUT=$(wget -O - --header=PageSpeedFilters:bogus $EXAMPLE_ROOT)
-echo $OUT
 check_from "$OUT" grep "$EXPECTED_EXAMPLES_TEXT"

 # Test that loopback route fetcher works with vhosts not listening on
@@ -834,6 +833,7 @@ function test_optimize_for_bandwidth() {
    check_from "$OUT" grep -q "$3"
  fi
 }
+
 test_optimize_for_bandwidth rewrite_css.html \
  '.blue{foreground-color:blue}body{background:url(arrow.png)}' \
  '<link rel="stylesheet" type="text/css" href="yellow.css">'
@@ -1826,8 +1826,8 @@ start_test resize_rendered_image_dimensions with critical images beacon
 HOST_NAME="http://renderedimagebeacon.example.com"
 URL="$HOST_NAME/mod_pagespeed_test/image_rewriting/image_resize_using_rendered_dimensions.html"
 http_proxy=$SECONDARY_HOSTNAME\
-    fetch_until -save -recursive $URL 'fgrep -c "pagespeed_url_hash"' 1 \
-'--header=X-PSA-Blocking-Rewrite:psatest'
+    fetch_until -save -recursive $URL 'fgrep -c "pagespeed_url_hash"' 2 \
+    '--header=X-PSA-Blocking-Rewrite:psatest'
 check [ $(grep -c "^pagespeed\.CriticalImages\.Run" \
  $WGET_DIR/image_resize_using_rendered_dimensions.html) = 1 ];
 OPTIONS_HASH=$(awk -F\' '/^pagespeed\.CriticalImages\.Run/ {print $(NF-3)}' \
@@ -1842,59 +1842,61 @@ BEACON_URL+="?url=http%3A%2F%2Frenderedimagebeacon.example.com%2Fmod_pagespeed_t
 BEACON_URL+="image_rewriting%2Fimage_resize_using_rendered_dimensions.html"
 BEACON_DATA="oh=$OPTIONS_HASH&n=$NONCE&ci=1344500982&rd=%7B%221344500982%22%3A%7B%22rw%22%3A150%2C%22rh%22%3A100%2C%22ow%22%3A256%2C%22oh%22%3A192%7D%7D"
 OUT=$(env http_proxy=$SECONDARY_HOSTNAME \
-  $WGET_DUMP --post-data "$BEACON_DATA" "$BEACON_URL")
+  $WGET_DUMP --no-http-keep-alive --post-data "$BEACON_DATA" "$BEACON_URL")
 check_from "$OUT" egrep -q "HTTP/1[.]. 204"
 http_proxy=$SECONDARY_HOSTNAME \
  fetch_until -save -recursive $URL \
  'fgrep -c 150x100xOptPuzzle.jpg.pagespeed.ic.' 1

-# TODO(anupama): Currently, the below tests for downstream caching and
-# rebeaconing interaction will pass incorrectly depending on whether the
-# time interval between the wgets is either < or > 5s (kMinBeaconIntervalMs)
-# Check to see if there is a better way to test this.
-
 # Verify that downstream caches and rebeaconing interact correctly for images.
-WGET_ARGS=""
-start_test lazyload_images,rewrite_images with critical images beacon
+start_test lazyload_images,rewrite_images with downstream cache rebeaconing
 HOST_NAME="http://downstreamcacherebeacon.example.com"
-URL="$HOST_NAME/mod_pagespeed_test/image_rewriting/rewrite_images.html"
-# There are 3 images on rewrite_images.html. Check that they are all
-# lazyloaded by default.
-http_proxy=$SECONDARY_HOSTNAME\
-  fetch_until -save -recursive $URL 'fgrep -c pagespeed_lazy_src=' 3
-check [ $(grep -c "^pagespeed\.CriticalImages\.Run" \
-  $WGET_DIR/rewrite_images.html) = 1 ];
-# An immediately issued wget does not result in instrumentation if the
-# correct PS-ShouldBeacon header is not present.
-OUT1=$(http_proxy=$SECONDARY_HOSTNAME\
-          $WGET_DUMP $WGET_ARGS\
-          --header="PS-ShouldBeacon: wrong_rebeaconing_key" $URL)
-# An immediately issued wget results in instrumentation if the PS-ShouldBeacon
-# header has the correct key.
+URL="$HOST_NAME/mod_pagespeed_test/downstream_caching.html"
+URL+="?PageSpeedFilters=lazyload_images"
+# 1. Even with blocking rewrite, we don't get an instrumented page when the
+# PS-ShouldBeacon header is missing.
+OUT1=$(http_proxy=$SECONDARY_HOSTNAME \
+          $WGET_DUMP --header 'X-PSA-Blocking-Rewrite: psatest' $URL)
+check_not_from "$OUT1" egrep -q 'pagespeed\.CriticalImages\.Run'
+check_from "$OUT1" grep -q "Cache-Control: private, max-age=3000"
+# 2. We get an instrumented page if the correct key is present.
 OUT2=$(http_proxy=$SECONDARY_HOSTNAME\
          $WGET_DUMP $WGET_ARGS\
          --header="PS-ShouldBeacon: random_rebeaconing_key" $URL)
-check_not_from "$OUT1" egrep -q "pagespeed\.CriticalImages\.Run"
 check_from "$OUT2" egrep -q "pagespeed\.CriticalImages\.Run"
-
-# Verify that downstream caches and rebeaconing interact correctly for css.
-test_filter prioritize_critical_css
-URL="$HOST_NAME/mod_pagespeed_example/prioritize_critical_css.html"
-# Once candidate selectors are ready, we get an instrumented page.
-http_proxy=$SECONDARY_HOSTNAME\
-    fetch_until -save $URL 'fgrep -c pagespeed.criticalCssBeaconInit' 1
-# An immediately issued wget does not result in instrumentation if the
-# correct PS-ShouldBeacon header is not present.
-OUT1=$(http_proxy=$SECONDARY_HOSTNAME\
+check_from "$OUT2" grep -q "Cache-Control: max-age=0, no-cache"
+# 3. We do not get an instrumented page if the wrong key is present.
+OUT3=$(http_proxy=$SECONDARY_HOSTNAME\
          $WGET_DUMP $WGET_ARGS\
          --header="PS-ShouldBeacon: wrong_rebeaconing_key" $URL)
-# An immediately issued wget results in instrumentation if the PS-ShouldBeacon
-# header has the correct key.
+check_not_from "$OUT3" egrep -q "pagespeed\.CriticalImages\.Run"
+check_from "$OUT3" grep -q "Cache-Control: private, max-age=3000"
+
+# Verify that downstream caches and rebeaconing interact correctly for css.
+test_filter prioritize_critical_css with rebeaconing
+HOST_NAME="http://downstreamcacherebeacon.example.com"
+URL="$HOST_NAME/mod_pagespeed_test/downstream_caching.html"
+URL+="?PageSpeedFilters=prioritize_critical_css"
+# 1. Even with blocking rewrite, we don't get an instrumented page when the
+# PS-ShouldBeacon header is missing.
+OUT1=$(http_proxy=$SECONDARY_HOSTNAME \
+          $WGET_DUMP --header 'X-PSA-Blocking-Rewrite: psatest' $URL)
+check_not_from "$OUT1" egrep -q 'pagespeed\.criticalCssBeaconInit'
+check_from "$OUT1" grep -q "Cache-Control: private, max-age=3000"
+# 2. We get an instrumented page if the correct key is present.
 OUT2=$(http_proxy=$SECONDARY_HOSTNAME\
-           $WGET_DUMP $WGET_ARGS\
-           --header="PS-ShouldBeacon: random_rebeaconing_key" $URL)
-check_not_from "$OUT1" egrep -q "pagespeed.criticalCssBeaconInit"
-check_from "$OUT2" egrep -q "pagespeed.criticalCssBeaconInit"
+          $WGET_DUMP $WGET_ARGS\
+          --header 'X-PSA-Blocking-Rewrite: psatest'\
+          --header="PS-ShouldBeacon: random_rebeaconing_key" $URL)
+check_from "$OUT2" grep -q "Cache-Control: max-age=0, no-cache"
+check_from "$OUT2" egrep -q "pagespeed\.criticalCssBeaconInit"
+# 3. We do not get an instrumented page if the wrong key is present.
+WGET_ARGS="--header=\"PS-ShouldBeacon: wrong_rebeaconing_key\""
+OUT3=$(http_proxy=$SECONDARY_HOSTNAME\
+          $WGET_DUMP $WGET_ARGS $URL)
+check_not_from "$OUT3" egrep -q "pagespeed\.criticalCssBeaconInit"
+check_from "$OUT3" grep -q "Cache-Control: private, max-age=3000"
+

 # Verify that we can send a critical image beacon and that lazyload_images
 # does not try to lazyload the critical images.
@@ -1926,7 +1928,6 @@ BEACON_DATA="oh=$OPTIONS_HASH&n=$NONCE&ci=2932493096"
 OUT=$(env http_proxy=$SECONDARY_HOSTNAME \
  wget -q --save-headers -O - --no-http-keep-alive \
  --post-data "$BEACON_DATA" "$BEACON_URL")
-echo $OUT
 check_from "$OUT" egrep -q "HTTP/1[.]. 204"
 # Now only 2 of the images should be lazyloaded, Cuppa.png should not be.
 http_proxy=$SECONDARY_HOSTNAME \
@@ -2030,7 +2031,7 @@ NONCE=$( \
  $FETCH_FILE)
 BEACON_URL="http://${HOSTNAME}${BEACON_PATH}?url=${ESCAPED_URL}"
 BEACON_DATA="oh=${OPTIONS_HASH}&n=${NONCE}&cs=.big,.blue,.bold,.foo"
-run_wget_with_args --post-data "$BEACON_DATA" "$BEACON_URL"
+run_wget_with_args --no-http-keep-alive --post-data "$BEACON_DATA" "$BEACON_URL"
 # Now make sure we see the correct critical css rules.
 fetch_until $URL \
  'grep -c <style>[.]blue{[^}]*}</style>' 1
@@ -2106,7 +2107,7 @@ CONNECTION=$(extract_headers $FETCH_UNTIL_OUTFILE | fgrep "Connection:")
 check_not_from "$CONNECTION" fgrep -qi "Keep-Alive, Keep-Alive"
 check_from "$CONNECTION" fgrep -qi "Keep-Alive"

-test_filter ngx_pagespeed_static defer js served with correct headers.
+start_test ngx_pagespeed_static defer js served with correct headers.
 # First, determine which hash js_defer is served with. We need a correct hash
 # to get it served up with an Etag, which is one of the things we want to test.
 URL="$HOSTNAME/mod_pagespeed_example/defer_javascript.html?PageSpeed=on&PageSpeedFilters=defer_javascript"
@@ -2310,6 +2311,10 @@ OUT=$($WGET_DUMP --header=Host:date.example.com \
 check_from "$OUT" egrep -q '^Date: Fri, 16 Oct 2009 23:05:07 GMT'

 if $USE_VALGRIND; then
+    # It is possible that there are still ProxyFetches outstanding
+    # at this point in time. Give them a few extra seconds to allow
+    # them to finish, so they will not generate valgrind complaints
+    sleep 3
    kill -s quit $VALGRIND_PID
    wait
    # Clear the previously set trap, we don't need it anymore.
@@ -32,6 +32,7 @@ http {
  pagespeed InPlaceResourceOptimization on;
  pagespeed CreateSharedMemoryMetadataCache "@@SHM_CACHE@@" 8192;
  pagespeed PreserveUrlRelativity on;
+  pagespeed BlockingRewriteKey psatest;

  # CriticalImagesBeaconEnabled is now on by default, but we disable in testing.
  # With this option enabled, the inline image system test will currently fail.
@@ -53,7 +54,6 @@ http {
    server_name max-cacheable-content-length.example.com;
    pagespeed FileCachePath "@@FILE_CACHE@@";

-    pagespeed BlockingRewriteKey psatest;

    pagespeed RewriteLevel PassThrough;
    pagespeed EnableFilters rewrite_javascript;
@@ -202,18 +202,20 @@ http {
  }

  server {
-    # Setup a vhost with the critical image beacon and lazyload filter enabled
-    # to make sure that downstream caches and rebeaconing interact correctly.
+    # Setup a vhost with the critical image beacon enabled to make sure that
+    # downstream caches and rebeaconing interact correctly.
    listen @@SECONDARY_PORT@@;
    server_name downstreamcacherebeacon.example.com;
    pagespeed FileCachePath "@@FILE_CACHE@@";

    pagespeed RewriteLevel PassThrough;
-    pagespeed EnableFilters lazyload_images;
    pagespeed CriticalImagesBeaconEnabled true;
    # Enable the downstream caching feature and specify a rebeaconing key.
    pagespeed DownstreamCachePurgeLocationPrefix "http://localhost:@@SECONDARY_PORT@@/purge";
    pagespeed DownstreamCacheRebeaconingKey random_rebeaconing_key;
+    location ~ .*[.]html {
+      add_header Cache-Control "private, max-age=3000";
+    }
  }

  server {
@@ -273,7 +275,8 @@ http {
    pagespeed on;
    pagespeed RewriteLevel PassThrough;
    pagespeed EnableFilters rewrite_images;
-    pagespeed MapOriginDomain localhost:@@SECONDARY_PORT@@/customhostheader
+    # Don't use localhost, as ngx_pagespeed's native fetcher cannot resolve it
+    pagespeed MapOriginDomain 127.0.0.1:@@SECONDARY_PORT@@/customhostheader
              sharedcdn.example.com/test customhostheader.example.com;
    pagespeed JpegRecompressionQuality 50;
    pagespeed CriticalImagesBeaconEnabled false;
@@ -286,7 +289,6 @@ http {
    server_name forbidden.example.com;
    pagespeed FileCachePath "@@FILE_CACHE@@";

-    pagespeed BlockingRewriteKey psatest;

    # Start with all core filters enabled ...
    pagespeed RewriteLevel CoreFilters;
@@ -570,7 +572,6 @@ http {
    server_name blocking.example.com;
    pagespeed FileCachePath "@@SECONDARY_CACHE@@";

-    pagespeed BlockingRewriteKey psatest;
    pagespeed RewriteLevel PassThrough;
    pagespeed EnableFilters rewrite_images;
  }
@@ -747,11 +748,6 @@ http {
    pagespeed Library 43 1o978_K0_LNE5_ystNklf
    http://www.modpagespeed.com/rewrite_javascript.js;

-    # If X-PSA-Blocking-Rewrite request header is present and its value matches
-    # the value of BlockingRewriteKey below, the response will be fully
-    # rewritten before being flushed to the client.
-    pagespeed BlockingRewriteKey psatest;
-
    add_header X-Extra-Header 1;

    # Establish a proxy mapping where the current server proxies an image
@@ -120,3 +120,46 @@
   fun:ngx_start_worker_processes
   fun:ngx_master_process_cycle
 }
+
+# Extra suppresions for testing in release mode:
+
+{
+   <re2 uninitialised value in optimized code>
+   Memcheck:Cond
+   fun:_ZN3re24Prog8OptimizeEv
+   ...
+}
+{
+   <re2 uninitialised value in optimized code>
+   Memcheck:Value8
+   fun:_ZN3re24Prog8OptimizeEv
+   ...
+}
+
+{
+   <re2 uninitialised value in optimized code>
+   Memcheck:Cond
+   fun:_ZN3re2L4AddQEPNS_9SparseSetEi
+   ...
+}
+
+{
+   <re2 uninitialised value in optimized code>
+   Memcheck:Value8
+   fun:_ZN3re2L4AddQEPNS_9SparseSetEi
+   ...
+}
+
+{
+   <re2 uninitialized value in optimized code>
+   Memcheck:Value8
+   fun:_ZN3re23DFA10AddToQueueEPNS0_5WorkqEij
+   ...
+}
+
+{
+   <re2 uninitialized value in optimized code>
+   Memcheck:Cond
+   fun:_ZN3re23DFA10AddToQueueEPNS0_5WorkqEij
+   ...
+}