From 9ba2502d1750005b4d7dc4bb13379b6e4aa87edc Mon Sep 17 00:00:00 2001 From: Jeff Kaufman Date: Thu, 16 Jul 2015 11:30:41 -0400 Subject: [PATCH] Don't set Last-Modified on IPRO Requests Fixes https://github.com/pagespeed/mod_pagespeed/issues/1106 --- net/instaweb/apache/apache_writer.cc | 1 - net/instaweb/apache/system_test.sh | 22 ++++++++++++++- .../rewriter/in_place_rewrite_context.cc | 1 + net/instaweb/system/admin_site.cc | 27 ++++++++++++------- pagespeed/apache/apache_fetch.cc | 1 - pagespeed/apache/apache_fetch_test.cc | 7 ----- 6 files changed, 39 insertions(+), 20 deletions(-) diff --git a/net/instaweb/apache/apache_writer.cc b/net/instaweb/apache/apache_writer.cc index 67c4ea4235..fa74ce2b24 100644 --- a/net/instaweb/apache/apache_writer.cc +++ b/net/instaweb/apache/apache_writer.cc @@ -80,7 +80,6 @@ void ApacheWriter::OutputHeaders(ResponseHeaders* response_headers) { if (content_length_ != AsyncFetch::kContentLengthUnknown) { ap_set_content_length(request_, content_length_); } - ResponseHeadersToApacheRequest(*response_headers, request_); request_->status = response_headers->status_code(); if (disable_downstream_header_filters_) { diff --git a/net/instaweb/apache/system_test.sh b/net/instaweb/apache/system_test.sh index 4a73e21bb0..0b057271a7 100755 --- a/net/instaweb/apache/system_test.sh +++ b/net/instaweb/apache/system_test.sh @@ -128,8 +128,28 @@ if [ $statistics_logging_enabled = "1" ]; then sleep 2; # Make sure we're around long enough to log stats. fi -# General system tests +start_test ipro resources have etag and not last-modified +URL="$EXAMPLE_ROOT/images/Puzzle.jpg?a=$RANDOM" +# Fetch it a few times until IPRO is done and has given it an ipro ("aj") etag. +fetch_until -save "$URL" 'grep -c E[Tt]ag:.W/.PSA-aj.' 1 --save-headers +# Verify that it doesn't have a Last-Modified header. +check [ $(grep -c "^Last-Modified:" $FETCH_FILE) = 0 ] +# Extract the Etag and verify we get "not modified" when we send the it. +ETAG=$(grep -a -o 'W/"PSA-aj[^"]*"' $FETCH_FILE) +check_from "$ETAG" fgrep PSA +echo $CURL -sS -D- -o/dev/null -H "If-None-Match: $ETAG" $URL +OUT=$($CURL -sS -D- -o/dev/null -H "If-None-Match: $ETAG" $URL) +check_from "$OUT" fgrep "HTTP/1.1 304" +check_not_from "$OUT" fgrep "Content-Length" +# Verify we don't get a 304 with a different Etag. +BAD_ETAG=$(echo "$ETAG" | sed s/PSA-aj/PSA-ic/) +echo $CURL -sS -D- -o/dev/null -H "If-None-Match: $BAD_ETAG" $URL +OUT=$($CURL -sS -D- -o/dev/null -H "If-None-Match: $BAD_ETAG" $URL) +check_not_from "$OUT" fgrep "HTTP/1.1 304" +check_from "$OUT" fgrep "HTTP/1.1 200" +check_from "$OUT" fgrep "Content-Length" +# General system tests start_test Check for correct default X-Mod-Pagespeed header format. OUT=$($WGET_DUMP $EXAMPLE_ROOT/combine_css.html) check_from "$OUT" egrep -q \ diff --git a/net/instaweb/rewriter/in_place_rewrite_context.cc b/net/instaweb/rewriter/in_place_rewrite_context.cc index 8be701919e..4d5ef36643 100755 --- a/net/instaweb/rewriter/in_place_rewrite_context.cc +++ b/net/instaweb/rewriter/in_place_rewrite_context.cc @@ -389,6 +389,7 @@ void InPlaceRewriteContext::FixFetchFallbackHeaders( headers->Replace(HttpAttributes::kEtag, HTTPCache::FormatEtag(StrCat( id(), "-", rewritten_hash_))); } + headers->RemoveAll(HttpAttributes::kLastModified); headers->set_implicit_cache_ttl_ms(Options()->implicit_cache_ttl_ms()); headers->set_min_cache_ttl_ms(Options()->min_cache_ttl_ms()); headers->ComputeCaching(); diff --git a/net/instaweb/system/admin_site.cc b/net/instaweb/system/admin_site.cc index b52b605167..28eb07da75 100644 --- a/net/instaweb/system/admin_site.cc +++ b/net/instaweb/system/admin_site.cc @@ -103,13 +103,16 @@ const Tab kTabs[] = { class AdminHtml { public: AdminHtml(StringPiece current_link, StringPiece head_extra, - AdminSite::AdminSource source, AsyncFetch* fetch, - MessageHandler* handler) + AdminSite::AdminSource source, Timer* timer, + AsyncFetch* fetch, MessageHandler* handler) : fetch_(fetch), handler_(handler) { fetch->response_headers()->SetStatusAndReason(HttpStatus::kOK); fetch->response_headers()->Add(HttpAttributes::kContentType, "text/html"); + int64 now_ms = timer->NowMs(); + fetch->response_headers()->SetLastModified(now_ms); + // Let PageSpeed dynamically minify the html, css, and javasript // generated by the admin page, to the extent it's not done // already by the tools. Note, this does mean that viewing the @@ -211,7 +214,7 @@ void AdminSite::ConsoleHandler(const SystemRewriteOptions& global_options, // TODO(sligocki): Do we want to have a minified version of console CSS? GoogleString head_markup = StrCat( "\n"); - AdminHtml admin_html("console", head_markup, source, fetch, + AdminHtml admin_html("console", head_markup, source, timer_, fetch, message_handler_); if (statistics_enabled && logging_enabled && log_dir_set) { fetch->Write("
\n" @@ -267,7 +270,7 @@ void AdminSite::StatisticsHandler(const RewriteOptions& options, Statistics* stats) { GoogleString head_markup = StrCat( "\n"); - AdminHtml admin_html("statistics", head_markup, source, fetch, + AdminHtml admin_html("statistics", head_markup, source, timer_, fetch, message_handler_); // We have to call Dump() here to write data to sources for // system/system_test.sh: Line 79. We use JS to update the data in refreshes. @@ -293,7 +296,8 @@ void AdminSite::GraphsHandler(const RewriteOptions& options, } GoogleString head_markup = StrCat( "\n"); - AdminHtml admin_html("graphs", head_markup, source, fetch, message_handler_); + AdminHtml admin_html("graphs", head_markup, source, timer_, fetch, + message_handler_); fetch->Write("
Loading Charts...
" "
Loading Charts...
" "
Loading Charts...
" @@ -367,7 +371,8 @@ void AdminSite::ConsoleJsonHandler(const QueryParams& params, void AdminSite::PrintHistograms(AdminSource source, AsyncFetch* fetch, Statistics* stats) { - AdminHtml admin_html("histograms", "", source, fetch, message_handler_); + AdminHtml admin_html("histograms", "", source, timer_, fetch, + message_handler_); stats->RenderHistograms(fetch, message_handler_); } @@ -551,7 +556,7 @@ void AdminSite::PrintCaches(bool is_global, AdminSource source, } else { GoogleString head_markup = StrCat( "\n"); - AdminHtml admin_html("cache", head_markup, source, fetch, + AdminHtml admin_html("cache", head_markup, source, timer_, fetch, message_handler_); fetch->Write("
", message_handler_); @@ -624,7 +629,7 @@ void AdminSite::PrintCaches(bool is_global, AdminSource source, void AdminSite::PrintNormalConfig( AdminSource source, AsyncFetch* fetch, SystemRewriteOptions* global_system_rewrite_options) { - AdminHtml admin_html("config", "", source, fetch, message_handler_); + AdminHtml admin_html("config", "", source, timer_, fetch, message_handler_); HtmlKeywords::WritePre( global_system_rewrite_options->OptionsToString(), "", fetch, message_handler_); @@ -632,7 +637,8 @@ void AdminSite::PrintNormalConfig( void AdminSite::PrintSpdyConfig(AdminSource source, AsyncFetch* fetch, const SystemRewriteOptions* spdy_config) { - AdminHtml admin_html("spdy_config", "", source, fetch, message_handler_); + AdminHtml admin_html("spdy_config", "", source, timer_, fetch, + message_handler_); if (spdy_config == NULL) { fetch->Write("SPDY-specific configuration missing.", message_handler_); } else { @@ -646,7 +652,8 @@ void AdminSite::MessageHistoryHandler(const RewriteOptions& options, // Request for page /mod_pagespeed_message. GoogleString log; StringWriter log_writer(&log); - AdminHtml admin_html("message_history", "", source, fetch, message_handler_); + AdminHtml admin_html("message_history", "", source, timer_, fetch, + message_handler_); if (message_handler_->Dump(&log_writer)) { fetch->Write("
", message_handler_); // Write pre-tag and color messages. diff --git a/pagespeed/apache/apache_fetch.cc b/pagespeed/apache/apache_fetch.cc index 0a291dd1a0..41654c787f 100644 --- a/pagespeed/apache/apache_fetch.cc +++ b/pagespeed/apache/apache_fetch.cc @@ -87,7 +87,6 @@ void ApacheFetch::HandleHeadersComplete() { int64 now_ms = timer_->NowMs(); response_headers()->SetDate(now_ms); - response_headers()->SetLastModified(now_ms); // http://msdn.microsoft.com/en-us/library/ie/gg622941(v=vs.85).aspx // Script and styleSheet elements will reject responses with diff --git a/pagespeed/apache/apache_fetch_test.cc b/pagespeed/apache/apache_fetch_test.cc index a0a5f2a8fa..09a2c5b8d6 100644 --- a/pagespeed/apache/apache_fetch_test.cc +++ b/pagespeed/apache/apache_fetch_test.cc @@ -213,7 +213,6 @@ TEST_F(ApacheFetchTest, Success) { "Set-Cookie: tasty=cookie\n" "Set-Cookie2: obselete\n" "Date: Thu, 01 Jan 1970 00:00:00 GMT\n" - "Last-Modified: Thu, 01 Jan 1970 00:00:00 GMT\n" "X-Content-Type-Options: nosniff\n" "Cache-Control: max-age=0, no-cache\n", HeadersOutToString(&request_)); @@ -239,7 +238,6 @@ TEST_F(ApacheFetchTest, NotFound404) { "Set-Cookie: tasty=cookie\n" "Set-Cookie2: obselete\n" "Date: Thu, 01 Jan 1970 00:00:00 GMT\n" - "Last-Modified: Thu, 01 Jan 1970 00:00:00 GMT\n" "X-Content-Type-Options: nosniff\n" "Cache-Control: max-age=0, no-cache\n", HeadersOutToString(&request_)); @@ -264,7 +262,6 @@ TEST_F(ApacheFetchTest, NoContentType200) { "Set-Cookie2: obselete\n" "Content-Type: text/html\n" "Date: Thu, 01 Jan 1970 00:00:00 GMT\n" - "Last-Modified: Thu, 01 Jan 1970 00:00:00 GMT\n" "X-Content-Type-Options: nosniff\n" "Cache-Control: max-age=0, no-cache\n", HeadersOutToString(&request_)); @@ -290,7 +287,6 @@ TEST_F(ApacheFetchTest, NoContentType301) { "Location: elsewhere\n" "Content-Type: text/html\n" "Date: Thu, 01 Jan 1970 00:00:00 GMT\n" - "Last-Modified: Thu, 01 Jan 1970 00:00:00 GMT\n" "X-Content-Type-Options: nosniff\n" "Cache-Control: max-age=0, no-cache\n", HeadersOutToString(&request_)); @@ -313,7 +309,6 @@ TEST_F(ApacheFetchTest, NoContentType304) { "Set-Cookie: tasty=cookie\n" "Set-Cookie2: obselete\n" "Date: Thu, 01 Jan 1970 00:00:00 GMT\n" - "Last-Modified: Thu, 01 Jan 1970 00:00:00 GMT\n" "X-Content-Type-Options: nosniff\n" "Cache-Control: max-age=0, no-cache\n", HeadersOutToString(&request_)); @@ -334,7 +329,6 @@ TEST_F(ApacheFetchTest, NoContentType204) { "Set-Cookie: tasty=cookie\n" "Set-Cookie2: obselete\n" "Date: Thu, 01 Jan 1970 00:00:00 GMT\n" - "Last-Modified: Thu, 01 Jan 1970 00:00:00 GMT\n" "X-Content-Type-Options: nosniff\n" "Cache-Control: max-age=0, no-cache\n", HeadersOutToString(&request_)); @@ -361,7 +355,6 @@ TEST_F(ApacheFetchTest, AbandonedAndHandled) { "Set-Cookie: tasty=cookie\n" "Set-Cookie2: obselete\n" "Date: Thu, 01 Jan 1970 00:00:00 GMT\n" - "Last-Modified: Thu, 01 Jan 1970 00:00:00 GMT\n" "X-Content-Type-Options: nosniff\n" "Cache-Control: max-age=0, no-cache\n", HeadersOutToString(&request_));