Fix crasher on 404 .pagespeed. resources w/a custom location

oschaaf · oschaaf · commit 1964ef5219cc · 2015-12-28T15:59:13.000+01:00
- Fix nginx-side flow so we handle .pagespeed. resources ok when they will land on a customized 404 internal location. - Additionally, check for a wiped request context and make sure we do not dereference a null pointer, which is what hurt in the flow we entered above as the IPRO lookup still was generating events while the nginx side request context was gone. - Also, as a preliminary measure, do not check fail when we receive a stale event originating from a NgxBaseFetch that is no longer associated with our request context. Do log a warning so we'll hear about this happening either through system test failures or a bug report. Fixes #1081
diff --git a/src/ngx_base_fetch.cc b/src/ngx_base_fetch.cc
@@ -118,6 +118,8 @@ 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();
@@ -138,9 +140,30 @@ void NgxBaseFetch::ReadCallback(const ps_event_data& data) {
   if (refcount == 0 || detached) {
     return;
   }
+
   ps_request_ctx_t* ctx = ps_get_request_context(r);
 
-  CHECK(data.sender == ctx->base_fetch);
+  // 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(r->count > 0) << "r->count: " << r->count;
 
   int rc;
diff --git a/src/ngx_pagespeed.cc b/src/ngx_pagespeed.cc
@@ -284,7 +284,8 @@ 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)) {
-      return status_code;
+      ps_release_base_fetch(ctx);
+      return ngx_http_filter_finalize_request(r, NULL, status_code);
     }
 
     if (ctx->preserve_caching_headers != kDontPreserveHeaders) {
diff --git a/test/nginx_system_test.sh b/test/nginx_system_test.sh
@@ -1241,6 +1241,14 @@ 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
diff --git a/test/pagespeed_test.conf.template b/test/pagespeed_test.conf.template
@@ -967,6 +967,14 @@ 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@@;

Original file line number	Diff line number	Diff line change
`@@ -284,7 +284,8 @@ ngx_int_t ps_base_fetch_handler(ngx_http_request_t* r) {`
`284`	`284`	`// modules running after us to manipulate those responses.`
`285`	`285`	`if (!status_ok && (ctx->base_fetch->base_fetch_type() != kHtmlTransform`
`286`	`286`	`&& ctx->base_fetch->base_fetch_type() != kIproLookup)) {`
`287`		`- return status_code;`
	`287`	`+ ps_release_base_fetch(ctx);`
	`288`	`+ return ngx_http_filter_finalize_request(r, NULL, status_code);`
`288`	`289`	`}`
`289`	`290`
`290`	`291`	`if (ctx->preserve_caching_headers != kDontPreserveHeaders) {`
Original file line number	Diff line number	Diff line change
`@@ -967,6 +967,14 @@ http {`
`967`	`967`	`}`
`968`	`968`	`}`
`969`	`969`
	`970`	`+ server {`
	`971`	`+ listen @@SECONDARY_PORT@@;`
	`972`	`+ listen [::]:@@SECONDARY_PORT@@;`
	`973`	`+ server_name custom404.example.com;`
	`974`	`+ pagespeed FileCachePath "@@SECONDARY_CACHE@@";`
	`975`	`+ error_page 404 /404.html;`
	`976`	`+ }`
	`977`	`+`
`970`	`978`	`server {`
`971`	`979`	`listen @@SECONDARY_PORT@@;`
`972`	`980`	`listen [::]:@@SECONDARY_PORT@@;`