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

- 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 https://github.com/pagespeed/ngx_pagespeed/issues/1081

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;

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) {

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

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@@;