WordPress.org

Make WordPress Core

Opened 8 months ago

Closed 6 months ago

Last modified 3 months ago

#45989 closed defect (bug) (fixed)

WSODs protection still 500 on REST API request

Reported by: spacedmonkey Owned by: flixos90
Milestone: 5.2 Priority: normal
Severity: major Version: 5.2
Component: Site Health Keywords: servehappy needs-testing has-patch commit
Focuses: multisite Cc:

Description

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.

Attachments (8)

Screenshot 2019-01-14 at 21.49.30.png (560.2 KB) - added by spacedmonkey 8 months ago.
45989.diff (9.5 KB) - added by spacedmonkey 8 months ago.
45989.2.diff (11.1 KB) - added by spacedmonkey 8 months ago.
45989.3.diff (2.0 KB) - added by spacedmonkey 8 months ago.
45989.4.diff (12.4 KB) - added by spacedmonkey 8 months ago.
45989.5.diff (12.9 KB) - added by spacedmonkey 8 months ago.
45989.6.diff (1.0 KB) - added by spacedmonkey 6 months ago.
45989.7.diff (1.6 KB) - added by spacedmonkey 6 months ago.

Download all attachments as: .zip

Change History (47)

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


8 months ago

#2 @TimothyBlynJacobs
8 months ago

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?

#3 @spacedmonkey
8 months ago

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.

@spacedmonkey
8 months ago

#4 @pento
8 months ago

  • Milestone changed from Awaiting Review to 5.1

Adding to 5.1 milestone for consideration.

#5 @flixos90
8 months ago

  • Owner set to flixos90
  • Status changed from new to reviewing

#6 @flixos90
8 months ago

  • Priority changed from normal to high

#7 @spacedmonkey
8 months ago

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.

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


8 months ago

#9 @flixos90
8 months ago

  • Owner changed from flixos90 to schlessera

#10 @spacedmonkey
8 months ago

@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.

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


8 months ago

#12 @TimothyBlynJacobs
8 months ago

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.

#13 @spacedmonkey
8 months ago

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...

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


8 months ago

This ticket was mentioned in Slack in #core-php by timothybjacobs. View the logs.


8 months ago

#16 @spacedmonkey
8 months ago

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.

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


8 months ago

This ticket was mentioned in Slack in #core by spacedmonkey. View the logs.


8 months ago

#19 @flixos90
8 months ago

  • Milestone changed from 5.1 to 5.2

#20 @flixos90
8 months ago

  • Priority changed from high to normal

#21 @flixos90
6 months ago

  • Milestone changed from 5.2 to 5.3

#22 @flixos90
6 months ago

  • Milestone 5.3 deleted
  • Resolution set to invalid
  • Status changed from reviewing to closed

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

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


6 months ago

#24 @flixos90
6 months ago

  • Milestone set to 5.2
  • Resolution invalid deleted
  • Status changed from closed to reopened
  • Version changed from 5.1 to trunk

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

#25 @spacedmonkey
6 months ago

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.

#26 @TimothyBlynJacobs
6 months ago

Why check for headers_sent at all?

#27 @flixos90
6 months ago

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.

#28 @spacedmonkey
6 months ago

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.

#29 @TimothyBlynJacobs
6 months ago

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

This ticket was mentioned in Slack in #core-php by timothybjacobs. View the logs.


6 months ago

#31 @spacedmonkey
6 months ago

  • Keywords has-patch added; needs-patch removed

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.

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


6 months ago

#33 @TimothyBlynJacobs
6 months ago

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.

#34 @spacedmonkey
6 months ago

@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.

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


6 months ago

#36 @spacedmonkey
6 months ago

  • Keywords commit added
  • Owner changed from schlessera to flixos90
  • Status changed from reopened to assigned

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.

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


6 months ago

#38 @SergeyBiryukov
6 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#39 @spacedmonkey
3 months ago

  • Component changed from Bootstrap/Load to Site Health
Note: See TracTickets for help on using tickets.