WSODs protection still 500 on REST API request

[spacedmonkey]

If code is hooked in rest_api_init and has a fatal error, the shutdown handler doesn't run and user gets a 500 error.

[TimothyBlynJacobs]

I think this is because the WP_EXECUTION_SUCCEEDED constant is defined at the end of wp-settings.php, but the REST API isn't initialized until after wp() is called which takes place in wp-blog-header.php after wp-load.php ( which includes wp-settings.php via wp-config.php.

Maybe the constant should happen at the end of wp-blog-header.php for front-end requests and the same spot in wp-settings.php for all others?

[spacedmonkey]

wp-blog-header.php isn't a good place for it as this file is only included for paths that use the theme. xmlrpc doesn't include it for example.

I have a patch that includes a file at the end of every executable file, but I don't love the work around.

[pento]

Adding to 5.1 milestone for consideration.

[spacedmonkey]

Also idea that I had was to add another define of WP_REST_EXECUTION_SUCCEEDED constant
On this
​this line. Then check of rest_quest is defined first. I will have a patch for this soon.

[spacedmonkey]

@flixos90 @schlessera Latest patch 45989.3.diff.

This patch is super simple, that adds a new filter wp_execution_succeeded and uses for the rest api. They maybe other parts of core than this logic needs to be applied too. It also allow plugin created to do this, which will be super useful.

[TimothyBlynJacobs]

I think a larger decision needs to be made about the scope of the shutdown handler for a request. Anything after wp_loaded will not trigger the error handler. Importantly this excludes much of the admin even though it could still be rendered inaccessible by fatal errors.

[spacedmonkey]

As much as I don't like 45989.diff, I am starting to believe it is the only workable solution. The greater question is, why is WP_EXECUTION_SUCCEEDED needed? If no error is found, the shutdown handler does nothing anyway...

[spacedmonkey]

After feedback from @schlessera and @nacin in this week's #core-php chat on slack, I have updated my patch to reflect this feedback. See 45989.5.diff includes a couple of places that I missed in my first patch.

[flixos90]

This ticket is based on the old fatal error recovery mode implementation and will be covered as part of #46130.

[flixos90]

Accidentally closed this, it should be a 5.2 priority related to #46130.

[spacedmonkey]

Looking at the current patch 45989.5.diff I really don't like the solution. The patch as is currently standard relays on putting define( 'WP_EXECUTION_SUCCEEDED', true ); on every point that WordPress exits execution. This is a extremely brittle, as if there are any places that I have missed where wordpress exits, the error message may show. Also any plugins and themes also exit with putting define( 'WP_EXECUTION_SUCCEEDED', true );, the error may display.

I believe a better solution would be check if header_sent and if the last error was fatal error, display the fatal error message using wp_die. Headers sent, means that much more of core can be error handled. Many plugins hook into template_redirect, rest_api_init or pre_get_posts, this is where the fatal error may happen and this wouldn't be handle by the current solution.

[TimothyBlynJacobs]

Why check for headers_sent at all?

[flixos90]

I agree we should find a better solution, and I agree with both your thoughts.

How about the following:

• We remove the whole WP_EXECUTION_SUCCEEDED thing entirely.
• In the fatal error handler, if headers_sent(), we don't display anything custom - as in we don't call wp_die() or print a custom message. In that case, all we do is the internal error handling (e.g. recovery mode). Displaying anything about the error would defer to what happens by default.

Reasons for omitting the display part under these circumstances:

• We can't predict how our custom message will mess up / conflict with already rendered output, possibly worse than before.
• The nicely formatted error notification is useful, but not super crucial to have in all cases.
• Most fatal errors probably happen before sending headers, so impact of omitting wouldn't be too significant.

[spacedmonkey]

The reason we have to check if header_sent, two fold.

1. If the fatal error happens mid way through the render of say the home page, call wp_die, will render another html and body tag. This is unlikely that this is what anyone would want.
2. wp_die sets no-cache header and status code to 500. This can't be done if headers are already set.

If headers already sent, we can just echo the message and die...

This idea was discussed before, but @schlessera had some issue with it.

[TimothyBlynJacobs]

Hm, how does that interact with people who have errors being displayed or other extraneous output?

[spacedmonkey]

Uploaded a new patch. 45989.6.diff.

This patch use headers_sent as the check. This is an improvement to what we had before as most use case of plugin hook were covered. Hooking in a fatal error to the wp_head action, still gave me a WSOD, but I am not sure how we can work around this. This should be committed ASAP.

[TimothyBlynJacobs]

I still think we should run recovery mode even if headers have been sent, otherwise we are going to regress when errors are displayed in some form.

Then we could render the error template conditionally. Perhaps, allow it if headers have been sent, but we haven't started rendering actual HTML? On the front-end that'd perhaps be something like did_filter( 'template_include' ), not sure about the admin.

[spacedmonkey]

@TimothyBlynJacobs Good catch on recovery mode not working if header already sent. In 45989.7.diff, I fix this issue.

As for your other feedback, I think it is impossible to display a message once the render has started. It is important to know what has already been rendered. Some example of page types.

• XML - RSS feeds
• JSON - rest api
• JSONp - Rest api
• string - ajax request
• xmlrpc - xmlrpc.
• html - front / back end.

There is just no way to know what format to output in. If it is html, you dont even know if the body tag has been opened. I don't think this is something we can fix in 5.2 / phase 1.Unless I am missing something.

[spacedmonkey]

After this week's #core-php meeting, it was agreed that 45989.7.diff was a good work around for this issue by @schlessera

Assigning to @flixos90 for merge.

[SergeyBiryukov]

In 45014:

Bootstrap/Load: Always run the fatal error handler at shutdown, but don't display the PHP error template once headers are sent.

If a fatal error occurs midway through a page load, or in a REST API request, it still needs to be handled internally for the recovery mode, but the custom message may conflict with already rendered output, e.g. by displaying HTML markup in an XML or JSON request.

Props spacedmonkey, flixos90, TimothyBlynJacobs.
Fixes #45989. See #44458.