diff --git a/src/ngx_base_fetch.cc b/src/ngx_base_fetch.cc index 60642f55a..d18476f9c 100644 --- 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(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 index 3ae79611d..dfe4beaa0 100644 --- 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 index ff52a7c68..49e977db8 100644 --- 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 index b70aec36d..7f9045c0c 100644 --- 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@@;