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 returning partial web pages #965

Closed
nikolay opened this Issue May 7, 2015 · 24 comments

Comments

Projects
None yet
4 participants
@nikolay
Copy link
Contributor

nikolay commented May 7, 2015

We just upgraded WordPress to 4.2.2 and suddenly the edit page interface started to serve unfinished pages. When we append ?PageSpeed=Off to the URL, the issue gets fixed, but, more importantly, we already have a rule that disables PageSpeed in the admin, i.e. pagespeed Disallow "*/wp-admin/*"; but it doesn't seem to work. Any ideas, @jeffkaufman?

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented May 11, 2015

@nikolay Is nginx really crashing? Or does something happen to the html that breaks wpadmin?

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented May 11, 2015

When you say "unfinished" do you mean that the html gets cut off partway through, or just that the page doesn't entirely load.

Could you try loading it with PageSpeed on, with the network tab open in the developer tools, to check if any resource loads are failing?

@nikolay

This comment has been minimized.

Copy link
Contributor

nikolay commented May 11, 2015

@jeffkaufman @oschaaf I assume that it's crashing, but I'm not sure if that's really the case. The result is that the page source code is cut off part way and it works fine if I disable PageSpeed. I downgraded WordPress, so, it doesn't seem to be related to WordPress version, just the state of the particular page breaks PageSpeed as we don't have this issue on other pages.

@nikolay

This comment has been minimized.

Copy link
Contributor

nikolay commented May 11, 2015

Here's a sample: https://www.diffchecker.com/3j5qdhfz

BTW, I do have the URL for the sample above in the pagespeed Disallow "*/wp-admin/*";, but it seems that PageSpeed still does some processing and it's obviously not identical to pagespeed off;. I can find a specific workaround and make this particular page work, but the issue is that this race condition could happen on pages I haven't yet identified.

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented May 12, 2015

@nikolay does anything noteworthy get logged to error.log when that happens?

@nikolay

This comment has been minimized.

Copy link
Contributor

nikolay commented May 12, 2015

@oschaaf Nothing. By the way, I removed pagespeed Disallow "*/wp-admin/*"; and everything works. I think the pass-thru mode of disallowed routes hits some race condition or some buffering bug causes the buffer not to flush entirely.

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented May 12, 2015

@nikolay You might be right about that. Looking at the code, the Disallow would disable IPRO but not html serialization/deserialization.
We do, however, create and release an NgxBaseFetch instance for IPRO in this flow.

I believe the extraneous events caused by the unused NgxBaseFetch instance could be responsible for the behaviour that you are seeing. I expect this to be fixed on trunk-tracking.

@nikolay

This comment has been minimized.

Copy link
Contributor

nikolay commented May 12, 2015

@oschaaf Thank you very much!

@nikolay

This comment has been minimized.

Copy link
Contributor

nikolay commented May 12, 2015

@oschaaf @jeffkaufman Unfortunately, it's not working for those pages even without Disallow again. Either it's not always working (i.e. there's some random element at play) or I didn't test properly, but the issue of Disallow not being a real pass-through is still there as behavior differs from pagespeed off; and it shouldn't.

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented May 12, 2015

@nikolay I suspect this is fixed on trunk-tracking by -edit- a5411a1
A partial backport might be possible, @jeffkaufman what do you think about doing that?

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented May 12, 2015

pointed out the wrong commit, edited the previous post to correct that.

@nikolay

This comment has been minimized.

Copy link
Contributor

nikolay commented May 12, 2015

@oschaaf Do you think this is good enough to patch the latest stable with? Of course, I can do testing to validate, but if it works, can I venture into patching production servers with it?

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented May 12, 2015

@nikolay
I think that the required change to fix this carries little or no risk at all, but it needs to be extracted from the patch in included above as that includes a little more.
The change you need starts here: a5411a1#diff-8fb776418fb480e5f084e8500b8f0b0cL1512 and ends here a5411a1#diff-8fb776418fb480e5f084e8500b8f0b0cL1910
Care must be taken however, as master and trunk have diverged a bit, and think it might not apply cleanly.

@nikolay

This comment has been minimized.

Copy link
Contributor

nikolay commented May 12, 2015

So, to clarify, @oschaaf, I need to take parts of the commit (marked by you), not the full commit? Any idea when this whole thing will be released?

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented May 12, 2015

@nikolay Yes, you need to take the marked part from the commit, and not the full commit.
I don't know much about the release eta

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented May 12, 2015

@crowell is working on the release, but not all the fixes are ready. Primarily the for pagespeed/mod_pagespeed/1048 .

@nikolay

This comment has been minimized.

Copy link
Contributor

nikolay commented May 12, 2015

@jeffkaufman Thanks for the heads up!

@crowell

This comment has been minimized.

Copy link
Contributor

crowell commented May 12, 2015

added to list for 1.9.32.4 point release here https://github.com/pagespeed/mod_pagespeed/wiki/Release-1.9.32.4

crowell added a commit that referenced this issue May 13, 2015

backport @oschaff fix of #965
fix issue of PageSpeed silently crashing returning partial web pages

backported from commit a5411a1

crowell added a commit that referenced this issue May 13, 2015

backport @oschaff fix of #965
fix issue of PageSpeed silently crashing returning partial web pages

backported from commit a5411a1

crowell added a commit that referenced this issue May 13, 2015

backport @oschaff fix of #965
fix issue of PageSpeed silently crashing returning partial web pages

backported from commit a5411a1
@crowell

This comment has been minimized.

Copy link
Contributor

crowell commented May 13, 2015

backported to master.

@crowell crowell closed this May 13, 2015

@nikolay

This comment has been minimized.

Copy link
Contributor

nikolay commented May 13, 2015

@crowell Thank you, sir!

@nikolay

This comment has been minimized.

Copy link
Contributor

nikolay commented May 29, 2015

@crowell Any idea when 1.9.32.4 will be out?

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jun 1, 2015

@nikolay We're working on it, but we want to include a fix for #1048. We have a mitigation that we could ship, but if we can get the root cause fixed we'd much rather do that, and we're still debugging.

@nikolay

This comment has been minimized.

Copy link
Contributor

nikolay commented Jun 1, 2015

@jeffkaufman Thank you! Just asking for planning purposes. Will it work with Nginx 1.9?

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jun 2, 2015

Will it work with Nginx 1.9?

Yes, it should. We test against nginx nightlies so that if nginx changes its API we know as far in advance as possible.

crowell added a commit that referenced this issue Jun 10, 2015

fix backport of fix for #965
was missing
ctx->base_fetch->SetRequestHeadersTakingOwnership(request_headers).

crowell added a commit that referenced this issue Jun 10, 2015

fix backport of fix for #965
was missing
ctx->base_fetch->SetRequestHeadersTakingOwnership(request_headers).

@jeffkaufman jeffkaufman changed the title PageSpeed silently crashing returning partial web pages PageSpeed returning partial web pages Jul 27, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment