Strip subresource hints pagespeed automatic system test configuration

Corresponding changes for:
- pull https://github.com/pagespeed/mod_pagespeed/pull/1204
- issue Strip subresource hints #973 https://github.com/pagespeed/mod_pagespeed/issues/973

config

@@ -29,8 +29,8 @@ if [ "$mod_pagespeed_dir" = "unset" ] ; then
     echo "   You need to separately download the pagespeed library:"
     echo ""
     echo "     $ cd /path/to/ngx_pagespeed"
-    echo "     $ wget https://dl.google.com/dl/page-speed/psol/1.10.33.6.tar.gz"
-    echo "     $ tar -xzvf 1.10.33.6.tar.gz # expands to psol/"
+    echo "     $ wget https://dl.google.com/dl/page-speed/psol/1.9.32.1.tar.gz"
+    echo "     $ tar -xzvf 1.9.32.1.tar.gz # expands to psol/"
     echo ""
     echo "   Or see the installation instructions:"
     echo "     https://github.com/pagespeed/ngx_pagespeed#how-to-build"
@@ -118,13 +118,6 @@ case "$NGX_GCC_VER" in
   ;;
 esac
 
-# workaround for a bug in nginx-1.9.11, see:
-# http://hg.nginx.org/nginx/rev/ff1e625ae55b
-NGX_VERSION=`grep nginx_version src/core/nginx.h | sed -e 's/^.* \(.*\)$/\1/'`
-if [ "$NGX_VERSION" = "1009011" ]; then
-    CFLAGS="$CFLAGS -Wno-write-strings"
-fi
-
 if [ "$WNO_ERROR" = "YES" ]; then
     CFLAGS="$CFLAGS -Wno-error"
 fi
@@ -193,7 +186,65 @@ ngx_feature_test="
 # Test whether we have pagespeed and can compile and link against it.
 . "$ngx_addon_dir/cpp_feature"
 
-if [ $ngx_found = no ]; then
+if [ $ngx_found = yes ]; then
+  ps_src="$ngx_addon_dir/src"
+  ngx_addon_name=ngx_pagespeed
+  NGX_ADDON_DEPS="$NGX_ADDON_DEPS \
+    $ps_src/log_message_handler.h \
+    $ps_src/ngx_base_fetch.h \
+    $ps_src/ngx_caching_headers.h \
+    $ps_src/ngx_event_connection.h \
+    $ps_src/ngx_fetch.h \
+    $ps_src/ngx_gzip_setter.h \
+    $ps_src/ngx_list_iterator.h \
+    $ps_src/ngx_message_handler.h \
+    $ps_src/ngx_pagespeed.h \
+    $ps_src/ngx_rewrite_driver_factory.h \
+    $ps_src/ngx_rewrite_options.h \
+    $ps_src/ngx_server_context.h \
+    $ps_src/ngx_url_async_fetcher.h \
+    $psol_binary"
+  NGX_ADDON_SRCS="$NGX_ADDON_SRCS \
+    $ps_src/log_message_handler.cc \
+    $ps_src/ngx_base_fetch.cc \
+    $ps_src/ngx_caching_headers.cc \
+    $ps_src/ngx_event_connection.cc \
+    $ps_src/ngx_fetch.cc \
+    $ps_src/ngx_gzip_setter.cc \
+    $ps_src/ngx_list_iterator.cc \
+    $ps_src/ngx_message_handler.cc \
+    $ps_src/ngx_pagespeed.cc \
+    $ps_src/ngx_rewrite_driver_factory.cc \
+    $ps_src/ngx_rewrite_options.cc \
+    $ps_src/ngx_server_context.cc \
+    $ps_src/ngx_url_async_fetcher.cc"
+  # Save our sources in a separate var since we may need it in config.make
+  PS_NGX_SRCS="$NGX_ADDON_SRCS"
+
+  if [ "$position_aux" = "true" ] ; then
+     HTTP_AUX_FILTER_MODULES="$HTTP_AUX_FILTER_MODULES $ngx_addon_name"
+  else
+    # Make pagespeed run immediately before gzip and Brotli.
+    if echo $HTTP_FILTER_MODULES | grep ngx_http_brotli_filter_module >/dev/null; then
+      module=ngx_http_brotli_filter_module
+    elif [ $HTTP_GZIP = YES ]; then
+      module=$HTTP_GZIP_FILTER_MODULE
+    else
+      module=$HTTP_RANGE_HEADER_FILTER_MODULE
+    fi
+
+    HTTP_FILTER_MODULES=$(echo $HTTP_FILTER_MODULES |\
+      sed "s/$module/$module $ngx_addon_name/")
+  fi
+
+  # Make the etag header filter run immediately before range header filter.
+  HTTP_FILTER_MODULES=$(echo $HTTP_FILTER_MODULES |\
+    sed "s/$HTTP_RANGE_HEADER_FILTER_MODULE/$HTTP_RANGE_HEADER_FILTER_MODULE ngx_pagespeed_etag_filter/")
+
+  CORE_LIBS="$CORE_LIBS $pagespeed_libs"
+  CORE_INCS="$CORE_INCS $pagespeed_include"
+  echo "List of modules (in reverse order of applicability): "$HTTP_FILTER_MODULES
+else
   cat << END
 $0: error: module ngx_pagespeed requires the pagespeed optimization library.
 Look in obj/autoconf.err for more details.
@@ -201,115 +252,6 @@ END
   exit 1
 fi
 
-ps_src="$ngx_addon_dir/src"
-ngx_addon_name=ngx_pagespeed
-NGX_ADDON_DEPS="$NGX_ADDON_DEPS \
-$ps_src/log_message_handler.h \
-$ps_src/ngx_base_fetch.h \
-$ps_src/ngx_caching_headers.h \
-$ps_src/ngx_event_connection.h \
-$ps_src/ngx_fetch.h \
-$ps_src/ngx_gzip_setter.h \
-$ps_src/ngx_list_iterator.h \
-$ps_src/ngx_message_handler.h \
-$ps_src/ngx_pagespeed.h \
-$ps_src/ngx_rewrite_driver_factory.h \
-$ps_src/ngx_rewrite_options.h \
-$ps_src/ngx_server_context.h \
-$ps_src/ngx_url_async_fetcher.h \
-$psol_binary"
-NPS_SRCS=" \
-$ps_src/log_message_handler.cc \
-$ps_src/ngx_base_fetch.cc \
-$ps_src/ngx_caching_headers.cc \
-$ps_src/ngx_event_connection.cc \
-$ps_src/ngx_fetch.cc \
-$ps_src/ngx_gzip_setter.cc \
-$ps_src/ngx_list_iterator.cc \
-$ps_src/ngx_message_handler.cc \
-$ps_src/ngx_pagespeed.cc \
-$ps_src/ngx_rewrite_driver_factory.cc \
-$ps_src/ngx_rewrite_options.cc \
-$ps_src/ngx_server_context.cc \
-$ps_src/ngx_url_async_fetcher.cc"
-# Save our sources in a separate var since we may need it in config.make
-PS_NGX_SRCS="$NGX_ADDON_SRCS \
-$NPS_SRCS"
-
-# Make pagespeed run immediately before gzip and Brotli.
-if echo $HTTP_FILTER_MODULES | grep ngx_http_brotli_filter_module >/dev/null; then
-  next=ngx_http_brotli_filter_module
-elif [ $HTTP_GZIP = YES ]; then
-  next=ngx_http_gzip_filter_module
-else
-  next=ngx_http_range_header_filter_module
-fi
-
-if [ -n "$ngx_module_link" ]; then
-  # nginx-1.9.11+
-  ngx_module_type=HTTP_FILTER
-  ngx_module_name="ngx_pagespeed ngx_pagespeed_etag_filter"
-  ngx_module_incs="$ngx_feature_path"
-  ngx_module_deps=
-  ngx_module_srcs="$NPS_SRCS"
-  ngx_module_libs="$ngx_feature_libs"
-  ngx_module_order="ngx_http_range_header_filter_module\
-                    ngx_pagespeed_etag_filter\
-                    ngx_http_gzip_filter_module \
-                    ngx_http_brotli_filter_module \
-                    ngx_pagespeed \
-                    ngx_http_postpone_filter_module \
-                    ngx_http_ssi_filter_module \
-                    ngx_http_charset_filter_module \
-                    ngx_http_xslt_filter_module \
-                    ngx_http_image_filter_module \
-                    ngx_http_sub_filter_module \
-                    ngx_http_addition_filter_module \
-                    ngx_http_gunzip_filter_module \
-                    ngx_http_userid_filter_module \
-                    ngx_http_headers_filter_module"
-
-    . auto/module
-
-    if [ $ngx_module_link != DYNAMIC ]; then
-      # ngx_module_order doesn't work with static modules,
-      # so we must re-order filters here.
-      if [ "$position_aux" = "true" ] ; then
-        HTTP_AUX_FILTER_MODULES="$HTTP_AUX_FILTER_MODULES $ngx_addon_name"
-      else
-        HTTP_FILTER_MODULES=$(echo $HTTP_FILTER_MODULES \
-                             | sed "s/ngx_pagespeed//" \
-                             | sed "s/$next/$next ngx_pagespeed/")
-      fi
-      # Make the etag header filter run immediately before range header filter.
-      HTTP_FILTER_MODULES=$(echo $HTTP_FILTER_MODULES \
-                           | sed "s/ngx_pagespeed_etag_filter//" \
-                           | sed "s/ngx_http_range_header_filter_module/ngx_http_range_header_filter_module ngx_pagespeed_etag_filter/")
-    else
-      # config.make is not executed for dynamic modules
-      CFLAGS="$CFLAGS -Wno-c++11-extensions"
-      if [ "$position_aux" = "true" ] ; then
-        ngx_module_type=HTTP_AUX_FILTER
-        ngx_module_order=""
-      fi
-    fi
-else
-  CORE_LIBS="$CORE_LIBS $pagespeed_libs"
-  CORE_INCS="$CORE_INCS $pagespeed_include"
-  NGX_ADDON_SRCS="$PS_NGX_SRCS"
-  if [ "$position_aux" = "true" ] ; then
-    HTTP_AUX_FILTER_MODULES="$HTTP_AUX_FILTER_MODULES $ngx_addon_name"
-  else
-    HTTP_FILTER_MODULES=$(echo $HTTP_FILTER_MODULES | sed "s/$next/$next $ngx_addon_name/")
-  fi
-
-  # Make the etag header filter run immediately before range header filter.
-  HTTP_FILTER_MODULES=$(echo $HTTP_FILTER_MODULES |\
-    sed "s/ngx_http_range_header_filter_module/ngx_http_range_header_filter_module ngx_pagespeed_etag_filter/")
-fi
-
-echo "List of modules (in reverse order of applicability): "$HTTP_FILTER_MODULES
-
 # Test whether the compiler is compatible
 ngx_feature="psol-compiler-compat"
 ngx_feature_name=""

src/ngx_base_fetch.cc

@@ -118,8 +118,6 @@ const char* BaseFetchTypeToCStr(NgxBaseFetchType type) {
   return "can't get here";
 }
 
-// TODO(oschaaf): replace the ngx_log_error with VLOGS or pass in a
-// MessageHandler and use that.
 void NgxBaseFetch::ReadCallback(const ps_event_data& data) {
   NgxBaseFetch* base_fetch = reinterpret_cast<NgxBaseFetch*>(data.sender);
   ngx_http_request_t* r = base_fetch->request();
@@ -140,40 +138,19 @@ void NgxBaseFetch::ReadCallback(const ps_event_data& data) {
   if (refcount == 0 || detached) {
     return;
   }
-
   ps_request_ctx_t* ctx = ps_get_request_context(r);
 
-  // If our request context was zeroed, skip this event.
-  // See https://github.com/pagespeed/ngx_pagespeed/issues/1081
-  if (ctx == NULL) {
-    // Should not happen normally, when it does this message will cause our
-    // system tests to fail.
-    ngx_log_error(NGX_LOG_WARN, ngx_cycle->log, 0,
-        "pagespeed [%p] skipping event: request context gone", r);
-    return;
-  }
-
-  // Normally we expect the sender to equal the active NgxBaseFetch instance.
-  DCHECK(data.sender == ctx->base_fetch);
-
-  // If someone changed our request context or NgxBaseFetch, skip processing.
-  if (data.sender != ctx->base_fetch) {
-      ngx_log_error(NGX_LOG_WARN, ngx_cycle->log, 0,
-          "pagespeed [%p] skipping event: event originating from disassociated"
-          " NgxBaseFetch instance.", r);
-      return;
-  }
+  CHECK(data.sender == ctx->base_fetch);
+  CHECK(r->count > 0) << "r->count: " << r->count;
 
   int rc;
-  bool run_posted = true;
   // If we are unlucky enough to have our connection finalized mid-ipro-lookup,
   // we must enter a different flow. Also see ps_in_place_check_header_filter().
   if ((ctx->base_fetch->base_fetch_type_ != kIproLookup)
       && r->connection->error) {
     ngx_log_error(NGX_LOG_DEBUG, ngx_cycle->log, 0,
-      "pagespeed [%p] request already finalized %d", r, r->count);
+      "pagespeed [%p] request already finalized", r);
     rc = NGX_ERROR;
-    run_posted = false;
   } else {
     rc = ps_base_fetch::ps_base_fetch_handler(r);
   }
@@ -186,11 +163,8 @@ void NgxBaseFetch::ReadCallback(const ps_event_data& data) {
 
   ngx_connection_t* c = r->connection;
   ngx_http_finalize_request(r, rc);
-
-  if (run_posted) {
-    // See http://forum.nginx.org/read.php?2,253006,253061
-    ngx_http_run_posted_requests(c);
-  }
+  // See http://forum.nginx.org/read.php?2,253006,253061
+  ngx_http_run_posted_requests(c);
 }
 
 void NgxBaseFetch::Lock() {

src/ngx_pagespeed.cc

@@ -284,8 +284,7 @@ ngx_int_t ps_base_fetch_handler(ngx_http_request_t* r) {
     // modules running after us to manipulate those responses.
     if (!status_ok && (ctx->base_fetch->base_fetch_type() != kHtmlTransform
                        && ctx->base_fetch->base_fetch_type() != kIproLookup)) {
-      ps_release_base_fetch(ctx);
-      return ngx_http_filter_finalize_request(r, NULL, status_code);
+      return status_code;
     }
 
     if (ctx->preserve_caching_headers != kDontPreserveHeaders) {
@@ -303,12 +302,6 @@ ngx_int_t ps_base_fetch_handler(ngx_http_request_t* r) {
                 STR_CASE_EQ_LITERAL(header->key, "Last-Modified") ||
                 STR_CASE_EQ_LITERAL(header->key, "Expires"))))) {
           header->hash = 0;
-          if (STR_CASE_EQ_LITERAL(header->key, "Location")) {
-            // There's a possible issue with the location header, where setting
-            // the hash to 0 is not enough. See:
-            // https://github.com/nginx/nginx/blob/master/src/http/ngx_http_header_filter_module.c#L314
-            r->headers_out.location = NULL;
-          }
         }
       }
     } else {
@@ -459,6 +452,11 @@ void copy_response_headers_from_ngx(const ngx_http_request_t* r,
 
   headers->set_status_code(r->headers_out.status);
 
+  if (r->headers_out.location != NULL) {
+    headers->Add(HttpAttributes::kLocation,
+                 str_to_string_piece(r->headers_out.location->value));
+  }
+
   // Manually copy over the content type because it's not included in
   // request_->headers_out.headers.
   headers->Add(HttpAttributes::kContentType,
@@ -492,10 +490,6 @@ ngx_int_t copy_response_headers_to_ngx(
 
   ngx_int_t i;
   for (i = 0 ; i < pagespeed_headers.NumAttributes() ; i++) {
-    // For IPRO cache misses, these gs_ variables may point to freed memory
-    // when nginx writes the headers to the output as the NgxBaseFetch instance
-    // that owns this memory gets released during request processing. So we
-    // copy these strings later on.
     const GoogleString& name_gs = pagespeed_headers.Name(i);
     const GoogleString& value_gs = pagespeed_headers.Value(i);
 
@@ -516,9 +510,6 @@ ngx_int_t copy_response_headers_to_ngx(
     }  // else we don't preserve any headers.
 
     ngx_str_t name, value;
-    value.len = value_gs.size();
-    value.data = reinterpret_cast<u_char*>(
-        string_piece_to_pool_string(r->pool, value_gs.c_str()));
 
     // To prevent the gzip module from clearing weak etags, we output them
     // using a different name here. The etag header filter module runs behind
@@ -529,15 +520,11 @@ ngx_int_t copy_response_headers_to_ngx(
       name.data = reinterpret_cast<u_char*>(
           const_cast<char*>(kInternalEtagName));
     } else {
-      name.len = name_gs.size();
-      name.data = reinterpret_cast<u_char*>(
-          string_piece_to_pool_string(r->pool, name_gs.c_str()));
-    }
-
-    // In case string_piece_to_pool_string failed:
-    if (name.data == NULL || value.data == NULL) {
-        return NGX_ERROR;
+      name.len = name_gs.length();
+      name.data = reinterpret_cast<u_char*>(const_cast<char*>(name_gs.data()));
     }
+    value.len = value_gs.length();
+    value.data = reinterpret_cast<u_char*>(const_cast<char*>(value_gs.data()));
 
     // TODO(jefftk): If we're setting a cache control header we'd like to
     // prevent any downstream code from changing it.  Specifically, if we're
@@ -548,17 +535,24 @@ ngx_int_t copy_response_headers_to_ngx(
     // net/instaweb/apache/header_util:AddResponseHeadersToRequest
 
     // Make copies of name and value to put into headers_out.
+
+    u_char* value_s = ngx_pstrdup(r->pool, &value);
+    if (value_s == NULL) {
+      return NGX_ERROR;
+    }
+
     if (STR_EQ_LITERAL(name, "Cache-Control")) {
-      ps_set_cache_control(r, reinterpret_cast<char*>(value.data));
+      ps_set_cache_control(r, const_cast<char*>(value_gs.c_str()));
       continue;
     } else if (STR_EQ_LITERAL(name, "Content-Type")) {
       // Unlike all the other headers, content_type is just a string.
-      headers_out->content_type = value;
+      headers_out->content_type.data = value_s;
+      headers_out->content_type.len = value.len;
 
       // We should not include the charset when determining content_type_len, so
       // scan for the ';' that marks the start of the charset part.
       for (ngx_uint_t i = 0; i < value.len; i++) {
-        if (value.data[i] == ';') {
+        if (value_s[i] == ';') {
           break;
         }
         headers_out->content_type_len = i + 1;
@@ -578,6 +572,11 @@ ngx_int_t copy_response_headers_to_ngx(
       continue;
     }
 
+    u_char* name_s = ngx_pstrdup(r->pool, &name);
+    if (name_s == NULL) {
+      return NGX_ERROR;
+    }
+
     ngx_table_elt_t* header = static_cast<ngx_table_elt_t*>(
         ngx_list_push(&headers_out->headers));
     if (header == NULL) {
@@ -585,10 +584,10 @@ ngx_int_t copy_response_headers_to_ngx(
     }
 
     header->hash = 1;  // Include this header in the output.
-    header->key.data = name.data;
     header->key.len = name.len;
-    header->value.data = value.data;
+    header->key.data = name_s;
     header->value.len = value.len;
+    header->value.data = value_s;
 
     // Populate the shortcuts to commonly used headers.
     if (STR_EQ_LITERAL(name, "Date")) {
@@ -637,7 +636,7 @@ typedef struct {
 typedef struct {
   // If pagespeed is configured in some server block but not this one our
   // per-request code will be invoked but server context will be null.  In those
-  // cases we need to short circuit, not changing anything.  Currently our
+  // cases we neet to short circuit, not changing anything.  Currently our
   // header filter, body filter, and content handler all do this, but if anyone
   // adds another way for nginx to give us a request to process we need to check
   // there as well.
@@ -3067,9 +3066,7 @@ void ps_exit_child_process(ngx_cycle_t* cycle) {
   ps_main_conf_t* cfg_m = static_cast<ps_main_conf_t*>(
       ngx_http_cycle_get_module_main_conf(cycle, ngx_pagespeed));
   NgxBaseFetch::Terminate();
-  if (cfg_m->driver_factory != NULL) {
-    cfg_m->driver_factory->ShutDown();
-  }
+  cfg_m->driver_factory->ShutDown();
 }
 
 // Called when nginx forks worker processes.  No threads should be started

test/nginx_system_test.sh

@@ -1241,14 +1241,6 @@ check_from "$OUT" fgrep -qi '404'
 MATCHES=$(echo "$OUT" | grep -c "Cache-Control: override") || true
 check [ $MATCHES -eq 1 ]
 
-start_test Custom 404 does not crash.
-URL=http://custom404.example.com/mod_pagespeed_test/
-URL+=A.doesnotexist.css.pagespeed.cf.0.css
-# The 404 response makes wget exit with an error code, which we ignore.
-OUT=$(http_proxy=$SECONDARY_HOSTNAME $WGET_DUMP -O /dev/null -S $URL 2>&1) || true
-# We ignored the exit code, check if we got a 404 response.
-check_from "$OUT" fgrep -qi '404'
-
 start_test Shutting down.
 
 # Fire up some heavy load if ab is available to test a stressed shutdown

test/pagespeed_test.conf.template

@@ -967,14 +967,6 @@ http {
     }
   }
 
-  server {
-    listen @@SECONDARY_PORT@@;
-    listen [::]:@@SECONDARY_PORT@@;
-    server_name custom404.example.com;
-    pagespeed FileCachePath "@@SECONDARY_CACHE@@";
-    error_page 404 /404.html;
-  }
-
   server {
     listen @@SECONDARY_PORT@@;
     listen [::]:@@SECONDARY_PORT@@;
@@ -1756,6 +1748,25 @@ http {
           https://www.gstatic.com/psa/static;
     }
 
+    location /mod_pagespeed_test/strip_subresource_hints/default/ {
+      pagespeed DisableFilters add_instrumentation;
+      pagespeed RewriteLevel CoreFilters;
+      pagespeed DisAllow *dontrewriteme*;
+    }
+    location /mod_pagespeed_test/strip_subresource_hints/default_passthrough/ {
+      pagespeed DisableFilters add_instrumentation;
+      pagespeed RewriteLevel PassThrough;
+    }
+    location /mod_pagespeed_test/strip_subresource_hints/preserve_on/ {
+      pagespeed DisableFilters add_instrumentation;
+      pagespeed PreserveSubresourceHints on;
+      pagespeed RewriteLevel CoreFilters;
+    }
+    location /mod_pagespeed_test/strip_subresource_hints/preserve_off/ {
+      pagespeed DisableFilters add_instrumentation;
+      pagespeed PreserveSubresourceHints off;
+      pagespeed RewriteLevel CoreFilters;
+    }
     # $host implicitly tests script variable support. I'd love to test it more
     # directly, but so far this is the best I've come up with and duplicating
     # the test doesn't seem to make sense.