From 24df42b6fad05a186feb584932533ebc132f2e20 Mon Sep 17 00:00:00 2001 From: Jeff Kaufman Date: Fri, 15 May 2015 10:33:10 -0400 Subject: [PATCH] If we're receiving a response slowly and the user aborts, we need to note this and not record the truncated resource to cache. Fixes https://github.com/pagespeed/mod_pagespeed/issues/1081 Nginx side of the change: https://github.com/pagespeed/ngx_pagespeed/pull/966 --- net/instaweb/apache/mod_instaweb.cc | 4 +- .../system/in_place_resource_recorder.cc | 8 +++- .../system/in_place_resource_recorder_test.cc | 38 ++++++++++++++++--- .../public/in_place_resource_recorder.h | 8 +++- 4 files changed, 49 insertions(+), 9 deletions(-) diff --git a/net/instaweb/apache/mod_instaweb.cc b/net/instaweb/apache/mod_instaweb.cc index c2a3f6aff0..5e132199f9 100644 --- a/net/instaweb/apache/mod_instaweb.cc +++ b/net/instaweb/apache/mod_instaweb.cc @@ -818,7 +818,9 @@ apr_status_t instaweb_in_place_check_headers_filter(ap_filter_t* filter, // We now have the final headers. If they don't let us cache then we'll // abort even though we've already buffered up the whole resource. InstawebHandler::AboutToBeDoneWithRecorder(request, recorder); - recorder->DoneAndSetHeaders(&response_headers); // Deletes recorder. + // Deletes recorder + recorder->DoneAndSetHeaders(&response_headers, + !request->connection->aborted); } } diff --git a/net/instaweb/system/in_place_resource_recorder.cc b/net/instaweb/system/in_place_resource_recorder.cc index b492c5e92a..161b23157b 100644 --- a/net/instaweb/system/in_place_resource_recorder.cc +++ b/net/instaweb/system/in_place_resource_recorder.cc @@ -208,7 +208,13 @@ void InPlaceResourceRecorder::DroppedDueToSize() { } void InPlaceResourceRecorder::DoneAndSetHeaders( - ResponseHeaders* response_headers) { + ResponseHeaders* response_headers, bool entire_response_received) { + if (!entire_response_received) { + // To record successfully, we must have a complete response. Otherwise you + // get https://github.com/pagespeed/mod_pagespeed/issues/1081. + Fail(); + } + if (!failure_ && !full_response_headers_considered_) { ConsiderResponseHeaders(kFullHeaders, response_headers); } diff --git a/net/instaweb/system/in_place_resource_recorder_test.cc b/net/instaweb/system/in_place_resource_recorder_test.cc index 8827d39124..78bf62eded 100644 --- a/net/instaweb/system/in_place_resource_recorder_test.cc +++ b/net/instaweb/system/in_place_resource_recorder_test.cc @@ -97,7 +97,8 @@ class InPlaceResourceRecorderTest : public RewriteTestBase { StringPiece gzipped(reinterpret_cast(&kGzippedData[0]), STATIC_STRLEN(kGzippedData)); recorder->Write(gzipped, message_handler()); - recorder.release()->DoneAndSetHeaders(&final_headers); + recorder.release()->DoneAndSetHeaders( + &final_headers, true /* complete response */); HTTPValue value_out; ResponseHeaders headers_out; @@ -160,7 +161,8 @@ TEST_F(InPlaceResourceRecorderTest, BasicOperation) { InPlaceResourceRecorder::kPreliminaryHeaders, &prelim_headers); recorder->Write(kHello, message_handler()); recorder->Write(kBye, message_handler()); - recorder.release()->DoneAndSetHeaders(&ok_headers); + recorder.release()->DoneAndSetHeaders( + &ok_headers, true /* complete response */); HTTPValue value_out; ResponseHeaders headers_out; @@ -171,6 +173,26 @@ TEST_F(InPlaceResourceRecorderTest, BasicOperation) { EXPECT_EQ(StrCat(kHello, kBye), contents); } +TEST_F(InPlaceResourceRecorderTest, IncompleteResponse) { + ResponseHeaders prelim_headers; + prelim_headers.set_status_code(HttpStatus::kOK); + + ResponseHeaders ok_headers; + SetDefaultLongCacheHeaders(&kContentTypeCss, &ok_headers); + + scoped_ptr recorder(MakeRecorder(kTestUrl)); + recorder->ConsiderResponseHeaders( + InPlaceResourceRecorder::kPreliminaryHeaders, &prelim_headers); + recorder->Write(kHello, message_handler()); + recorder.release()->DoneAndSetHeaders( + &ok_headers, false /* incomplete response */); + + HTTPValue value_out; + ResponseHeaders headers_out; + EXPECT_EQ(HTTPCache::kNotFound, + HttpBlockingFind(kTestUrl, http_cache(), &value_out, &headers_out)); +} + TEST_F(InPlaceResourceRecorderTest, CheckCacheableContentTypes) { CheckCacheableContentType(&kContentTypeJpeg); CheckCacheableContentType(&kContentTypeCss); @@ -192,7 +214,8 @@ TEST_F(InPlaceResourceRecorderTest, BasicOperationFullHeaders) { InPlaceResourceRecorder::kFullHeaders, &ok_headers); recorder->Write(kHello, message_handler()); recorder->Write(kBye, message_handler()); - recorder.release()->DoneAndSetHeaders(&ok_headers); + recorder.release()->DoneAndSetHeaders( + &ok_headers, true /* complete response */); HTTPValue value_out; ResponseHeaders headers_out; @@ -216,7 +239,8 @@ TEST_F(InPlaceResourceRecorderTest, DontRemember304) { scoped_ptr recorder(MakeRecorder(kTestUrl)); recorder->ConsiderResponseHeaders( InPlaceResourceRecorder::kPreliminaryHeaders, &prelim_headers); - recorder.release()->DoneAndSetHeaders(¬_modified_headers); + recorder.release()->DoneAndSetHeaders( + ¬_modified_headers, true /* complete response */); HTTPValue value_out; ResponseHeaders headers_out; @@ -237,7 +261,8 @@ TEST_F(InPlaceResourceRecorderTest, Remember500AsFetchFailed) { scoped_ptr recorder(MakeRecorder(kTestUrl)); recorder->ConsiderResponseHeaders( InPlaceResourceRecorder::kPreliminaryHeaders, &prelim_headers); - recorder.release()->DoneAndSetHeaders(&error_headers); + recorder.release()->DoneAndSetHeaders( + &error_headers, true /* complete response */); HTTPValue value_out; ResponseHeaders headers_out; @@ -252,7 +277,8 @@ TEST_F(InPlaceResourceRecorderTest, RememberEmpty) { scoped_ptr recorder(MakeRecorder(kTestUrl)); // No contents written. - recorder.release()->DoneAndSetHeaders(&ok_headers); + recorder.release()->DoneAndSetHeaders( + &ok_headers, true /* complete response */); HTTPValue value_out; ResponseHeaders headers_out; diff --git a/net/instaweb/system/public/in_place_resource_recorder.h b/net/instaweb/system/public/in_place_resource_recorder.h index c0d3b79903..b8247018ed 100644 --- a/net/instaweb/system/public/in_place_resource_recorder.h +++ b/net/instaweb/system/public/in_place_resource_recorder.h @@ -109,10 +109,16 @@ class InPlaceResourceRecorder : public Writer { // Because of Apache's quirky filter order, we cannot get both the // uncompressed final contents and the complete headers at the same time. // + // Set entire_response_received to true if you know that the response data fed + // into Write() is complete. For example, if the browser cancelled the + // download and so this is a partial response, set entire_response_received to + // false so we know not to cache it. + // // Does not take ownership of response_headers. // // Deletes itself. Do not use object after calling DoneAndSetHeaders(). - void DoneAndSetHeaders(ResponseHeaders* response_headers); + void DoneAndSetHeaders(ResponseHeaders* response_headers, + bool entire_response_received); const GoogleString& url() const { return url_; } MessageHandler* handler() { return handler_; }