-
Notifications
You must be signed in to change notification settings - Fork 363
IPRO with LoadFromFile gives null responses for unknown file extensions #719
Comments
Do you have an example site I could test this on? Either one running PageSpeed with IPRO enabled or just one where I could copy the html down and test on my own machine. |
@jeffkaufman I will try to create an isolated environment for you - having my config file and WordPress set up in there. In this particular case, it seems that when using
|
@jeffkaufman Would you prefer a Vagrant or a Docker image? |
Vagrant would probably be easier. |
Before you send me an, actually, let me play around with the combination of |
I've reproduced this locally; working on it. |
To reproduce:
No content is served. The error log gets:
|
Looking at the |
I also see this in error.log when reproducing:
That makes me think that with this https://code.google.com/p/modpagespeed/source/browse/trunk/src/net/instaweb/rewriter/in_place_rewrite_context.cc#114 - which states in comments: // If we are the origin, we do not have to pass through bytes
// if we aren't rewriting --- the caller is expected to fall back to
// the server's native method if FetchInPlaceResource fails. A backtrace of when NgxBaseFetch::HandleHeadersDone() is called confirms this:
|
@jeffkaufman ...
streaming_ = false;
set_request_headers(NULL);
set_response_headers(NULL);
set_extra_response_headers(NULL);
response_headers()->set_status_code(HttpStatus::kNotFound);
...
The response status code is set to ...
streaming_ = false;
set_request_headers(NULL);
response_headers()->set_status_code(HttpStatus::kNotFound);
set_response_headers(NULL);
set_extra_response_headers(NULL);
... |
@oschaaf That change almost works, but gives memory errors. I saw
|
With @oschaaf's change |
Looking at the corrupted |
This sounds like it might be a conflict between null terminated strings and length-specified ones? |
In |
Yeah, I think there's a lifetime issue. I see (selected log entries):
|
A simple fix would be to make Normally we delete the base fetch after everything else is complete, with But why do we need to release the base fetch when we decline the request? Can't we just wait and let it be released when nginx cleans us up with |
Ok: I think this fixes it:
I'll make a pull request after running full tests. |
It looks like the reordering change, (1) above, breaks |
Here's another potential solution: when deciding whether to load something from file, considering |
This would be nice as it will save me the whitelisting I have in place.
|
Right; limiting to known file extensions is probably something we should do whether or not we also fix other things in response to this bug. |
@jeffkaufman "It looks like the reordering change, (1) above, breaks TEST: In-place resource optimization" |
@jeffkaufman "Right; limiting to known file extensions is probably something we should do whether or not we also fix other things in response to this bug." |
@oschaaf The existing IPRO test passes with the existing code and fails with your change. The new test fails with the existing code and passes with your change. I haven't gotten into the details of why your change breaks the existing IPRO test; it looks reasonable to me. I'm now going off in a different direction looking at content types, though, if you're interested at pushing your change further. |
@jeffkaufman I looked at the change I proposed some more: #721 passes all tests |
This bug applies to mod_pagespeed as well:
It's kind of unsettling that Either #721 or limiting |
Actually, it looks like that's supposed to happen. The intent of the code that wrote it is to serve a 403 and give the viewer that note about the missing content type. What's going wrong here is without #721 |
Fixed in trunk with r4067. |
When I turned the feature on, all my web fonts broke having zero length. In messages, I see PageSpeed trying to optimize fonts in-place.
The text was updated successfully, but these errors were encountered: