New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pagespeed optimized resource sending incorrect Content-Length header #941

Closed
GoogleCodeExporter opened this Issue Apr 6, 2015 · 7 comments

Comments

Projects
None yet
1 participant
@GoogleCodeExporter
Copy link

GoogleCodeExporter commented Apr 6, 2015

We think have isolated a problem with Pagespeed on nginx 1.7.30.4 where we 
intermittently get responses for Pagespeed optimised resource URLs that have a 
Content-Length header set where more data is sent than the content-length.

The Pagespeed optimised resource intermittently responds with a 
"Content-Length: 62800", which is the size of the resource on the origin 
servers. Nginx actually transmits 74620 bytes.

While this will be causing protocol violations, when you couple this with 
Varnish, the protocol violation is fixed, because Varnish truncates the content 
to match the content-length header sent by nginx.

This results in broken CSS. When cached by a CDN the problem is further 
exacerbated.

The CSS resources are minified (.cf. in the url). Which also raises the 
question about why the resources are larger than the origin resources.

I can provide URLs out of band and reproduction steps if necessary.

Any help would be appreciated.

Original issue reported on code.google.com by jmara...@google.com on 9 May 2014 at 6:27

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Original comment by jmara...@google.com on 9 May 2014 at 6:27

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

There are two possible sources of the bug found:


Look around lines 1065-1068: 
https://code.google.com/p/modpagespeed/source/browse/trunk/src/net/instaweb/rewr
iter/rewrite_context.cc#1065

I think if we are going to absolutify in a streaming fashion, we need to use 
chunked encoding (not call async_fetch->set_content_length(contents.size())


Also, specific to nginx:

ngx_int_t NgxBaseFetch::CollectHeaders(ngx_http_headers_out_t* headers_out) {
  const ResponseHeaders* pagespeed_headers = response_headers();

  if (content_length_known()) {
     headers_out->content_length = NULL;
     headers_out->content_length_n = content_length();
  }

  return copy_response_headers_to_ngx(request_, *pagespeed_headers,
                                      preserve_caching_headers_);

This latter is likely fixed by Otto's pull request: 

https://github.com/pagespeed/ngx_pagespeed/compare/release-1.7.30.4-beta...oscha
af-1.7.30.4-content-length?expand=1


However Otto was not able repro, suggesting that the rewrite_context.cc problem 
might be necessary (if not sufficient) to fix.

Original comment by jmara...@google.com on 9 May 2014 at 6:30

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

I have a candidate fix that needs still review, including a unit-test repro, 
which I've attached.

Original comment by jmara...@google.com on 9 May 2014 at 8:45

Attachments:

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

FWIW, LGTM
For cross referencing: https://github.com/pagespeed/ngx_pagespeed/issues/695

Original comment by osch...@we-amp.com on 9 May 2014 at 8:57

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Fixed in revision 3996

Original comment by jmara...@google.com on 12 May 2014 at 10:34

  • Added labels: Milestone-v31, release-note
@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Original comment by jmaes...@google.com on 21 May 2014 at 3:05

  • Changed title: Pagespeed optimized resource sending incorrect Content-Length header
@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Original comment by jefftk@google.com on 4 Aug 2014 at 6:45

  • Changed state: Fixed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment