Skip to content
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

Fatal errors due to infinite recursion with Full Site Editing #26923

Closed
bobbingwide opened this issue Nov 12, 2020 · 19 comments
Closed

Fatal errors due to infinite recursion with Full Site Editing #26923

bobbingwide opened this issue Nov 12, 2020 · 19 comments
Labels
[Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") [Type] Bug An existing feature does not function as intended

Comments

@bobbingwide
Copy link
Contributor

bobbingwide commented Nov 12, 2020

Describe the bug
When using Full Site Editing it is fairly easy to create scenarios which lead to the website failing due to infinite recursion.
Some work has been done to prevent the problems occurring visually in the editor, but once saved it becomes very difficult to recover from the problem.

In the simplest scenario below the recursion involves a single post using the core/post-content block. More complex scenarios involve multiple posts each with their own core/query and core/post-content blocks.

WordPress core needs to appreciate that this infinite recursion can occur, take steps to prevent it and produce relevant information to the content creator and/or front end user to explain the actions taken.

I'm raising this as a separate issue to #26593, although I believe the root cause of both problems is the same.
That is, the inability to detect and prevent infinite recursion while processing potentially recursive constructs.

These recursive constructs include:

  • Post Content ( core/post-content )
  • Reusable blocks ( core/block )
  • Template parts ( core/template-parts )

plus, to be confirmed

  • possibly Post Excerpt ( core/post-excerpt )
  • Combinations of the above, if that's possible.

To reproduce
Steps to reproduce the behavior:

  1. Activate a Full Site Editing theme such as Twenty Twenty-One Blocks
  2. Activate Gutenberg as the only plugin.
  3. Create a new post.
  4. Insert a Post Content block
  5. Save
  6. View
  7. See the Fatal error

Notes:

Expected behavior
The infinite recursion should be detected, halted gracefully and an appropriate message reported to the user.
In WP_DEBUG mode contextual information should be displayed to assist with problem resolution.

Screenshots
In this screenshot the Fatal error occurs when the Allowed memory is exceeded.
image

Editor version (please complete the following information):

  • WordPress version: [e.g: 5.3.2] 5.6-beta3
  • Does the website has Gutenberg plugin installed, or is it using the block editor that comes by default? [e.g: "gutenberg plugin", "default"] Gutenberg 9.3.0 and/or Gutenberg built from source

Desktop (please complete the following information):

  • OS: [e.g. iOS] Windows
  • Browser [e.g. chrome, safari] Chrome
  • Version [e.g. 22] Latest

Additional context

@carlomanf
Copy link

carlomanf commented Nov 13, 2020

I agree with you about most of the recursive constructs you mentioned and reported the same issue myself (it was closed as a duplicate of #18704, this one probably will be too.)

The one recursive construct you mentioned that I would dispute is the post excerpt. This shouldn't ever cause infinite recursion because it's only plain text. The other 3 and combinations of them are the only cases I'm aware of.

@carlomanf
Copy link

I also agree a more holistic solution needs to be found to this. The solution that was deployed to #22080 was a bit of a band-aid that overlooked the many other ways this can present.

@carlomanf
Copy link

On closer inspection, you might be right about the post excerpt. Although it doesn't output blocks on the front-end, it does process blocks behind the scenes. That said, any block at all could process blocks behind the scenes and that just highlights the need for a holistic solution rather than block-specific fixes.

@carlomanf
Copy link

I think this could be fixed with some kind of global/static array that keeps track of the blocks currently being rendered. This could be done by modifying WP_Block::render to check whether this block already exists in the array, return early if it does, add it if not, and delete it from the array at the end of the method.

Here is an example with some pseudocode:

Block A renders blocks B and C
Block B renders blocks C and D
Block C renders block B
Block D renders block A

render block A:
    is block A in array?
    no, then add block A to array and render it
    now rendering block A:
        render block B:
            is block B in array?
            no, then add block B to array and render it
            now rendering block B:
                render block C:
                    is block C in array?
                    no, then add block C to array and render it
                    now rendering block C:
                        render block B:
                            is block B in array?
                            yes, so don't render it
                        end block B
                    done rendering block C, delete block C from array
                end block C
                render block D:
                    is block D in array?
                    no, then add block D to array and render it
                    now rendering block D:
                        render block A:
                            is block A in array?
                            yes, so don't render it
                        end block A
                    done rendering block D, delete block D from array
                end block D
            done rendering block B, delete block B from array
        end block B
        render block C:
            is block C in array?
            no, then add block C to array and render it
            now rendering block C:
                render block B:
                    is block B in array?
                    no, then add block B to array and render it
                    now rendering block B:
                        render block C:
                            is block C in array?
                            yes, so don't render it
                        end block C
                        render block D:
                            is block D in array?
                            no, then add block D to array and render it
                            now rendering block D:
                                render block A:
                                    is block A in array?
                                    yes, so don't render it
                                end block A
                            done rendering block D, delete block D from array
                        end block D
                    done rendering block B, delete block B from array
                end block B
            done rendering block C, delete block C from array
        end block C
    done rendering block A, delete block A from array
end block A

@bobbingwide
Copy link
Contributor Author

Hi @carlomanf looks like we're both working to solve the issue. Your PR is a simpler version of the solution I developed. I suppose I should make comments in the PR.

Meanwhile I'm working on developing my own fix that attempts to report to the user the fact that recursion has occurred. I have an idea how I can deal with the recursive reusable blocks in the editor.

I really hope no one attempts to close this as a duplicate of #18704.

@gziolo
Copy link
Member

gziolo commented Nov 13, 2020

It should be as simple as defining a flag in supports that prevents you from inserting a given block inside the same type of block. We have a very similar multiple flag in the supports object when defining a block typet, see https://github.com/WordPress/gutenberg/blob/master/docs/designers-developers/developers/block-api/block-supports.md#multiple

@bobbingwide
Copy link
Contributor Author

Only the simplest case is simple. Users and developers find all sorts of ways to create infinitely recursive constructs. For my reusable blocks scenario I found an edge case where my logic didn't detect recursion. I believe this was because the post being edited, that was eventually being called recursively was a Draft.

@gziolo
Copy link
Member

gziolo commented Nov 13, 2020

Surely you can tweak the content in the database or provide a template that is broken by design. The question is how it should be handled. Maybe it should throw an error to give some feedback to developers/designers. I still think that we have to prevent it from happening through UI. So that's two tasks in one.

@bobbingwide
Copy link
Contributor Author

The question is how it should be handled. Maybe it should throw an error to give some feedback to developers/designers.

That's what I'm working on right now.

I managed to handle the recursion in the front end - with visual feeback indicating where the recursion happened. Now I'm looking at handling it in the editor. For the reusable blocks issue, even though I've prevented recursion in the REST API while rendering the post_content, the front end is still trying to process each wp:block it sees in the raw post_content.

When recursion has been detected in the back end I'm hoping to effectively disable the culprit block in the raw post_content by hooking into rest_prepare_wp_block.

return apply_filters( "rest_prepare_{$this->post_type}", $response, $post, $request );

@bobbingwide
Copy link
Contributor Author

It should be as simple as defining a flag in supports that prevents you from inserting a given block inside the same type of block.
I still think that we have to prevent it from happening through UI.

When recursion is detected in the server I have managed to fiddle the REST API response to allow the block editor to load up a modified version of the Reusable block, with any wp:block converted to something else.

This however doesn't prevent the user from inserting the original reusable block again, nor any of the other blocks that cause the recursion.

I've not looked at the .js code so don't know what it's doing, but I can see that it's looping itself and not talking to the server.
I agree that the UI will have to do something to at least prevent the trivial problem inserting a reusable block into itself.

@carlomanf
Copy link

Surely you can tweak the content in the database or provide a template that is broken by design. The question is how it should be handled. Maybe it should throw an error to give some feedback to developers/designers. I still think that we have to prevent it from happening through UI. So that's two tasks in one.

That's correct. There are two tasks:

  1. Escape the infinite loop on the front-end in case cyclical blocks somehow get saved to the database. This is fully solved by my PR Solve #18704 on front-end #26951.

  2. Help users to avoid creating such a situation through the UI. This is still yet to be solved.

I managed to handle the recursion in the front end - with visual feeback indicating where the recursion happened.

I don't think there should be feedback on the front-end, because it's visible to all site visitors. My patch at #26951 just escapes it with an empty string so site visitors are none the wiser.

@bobbingwide
Copy link
Contributor Author

I don't think there should be feedback on the front-end, because it's visible to all site visitors.

  • In my experience, in many cases, the recursion only becomes visible on the front end.
  • If there is no visible feedback then the content creator / developer won't know there's been a problem.
  • In my routines, when WP_DEBUG is true, additional work is performed to assist with problem determination. See the links in the Additional context.
  • Unfortunately, WordPress/Gutenberg doesn't yet provide a common UI to display messages on the front end.
  • Nor is there an easy solution for the REST API to return this information.
  • I believe we should take the opportunity to improve the feedback given to the user when something has gone awry.

@carlomanf
Copy link

carlomanf commented Nov 15, 2020

  • In my experience, in many cases, the recursion only becomes visible on the front end.

Maybe, but if the system can detect it at the time of editing then it should be reported there and then, instead of waiting for someone to load the front-end.

@bobbingwide
Copy link
Contributor Author

but if the system can detect it at the time of editing then it should be reported there and then

I take it you're agreeing that there should be some ability to provide feedback to the user, preferably in the editor. How that is achieved has yet to be designed.

My requirements would include:

  • Ability to display server messages on the front end through a consistent block based UI.
  • The amount of information displayed should be based on the user's capabilities to correct the problem.
  • Ability to display server messages in the editor, extracted from the REST request's response.

bobbingwide added a commit to bobbingwide/gutenberg that referenced this issue Nov 16, 2020
bobbingwide added a commit to bobbingwide/gutenberg that referenced this issue Nov 16, 2020
bobbingwide added a commit to bobbingwide/gutenberg that referenced this issue Nov 16, 2020
bobbingwide added a commit to bobbingwide/gutenberg that referenced this issue Nov 16, 2020
bobbingwide added a commit to bobbingwide/gutenberg that referenced this issue Nov 16, 2020
bobbingwide added a commit to bobbingwide/gutenberg that referenced this issue Nov 16, 2020
bobbingwide added a commit to bobbingwide/gutenberg that referenced this issue Nov 16, 2020
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Nov 16, 2020
@skorasaurus skorasaurus removed the [Status] In Progress Tracking issues with work in progress label Feb 18, 2021
@bobbingwide
Copy link
Contributor Author

bobbingwide commented Feb 23, 2021

With Gutenberg 10.0.0 this problem has now become worse. If you try to insert a core/post-content block the editor will lock up.

To reproduce
Steps to reproduce the behavior:

  1. Activate a Full Site Editing theme such as Twenty Twenty-One Blocks or Q.
  2. Activate Gutenberg as the only plugin.
  3. Create a new post.
  4. Insert a Post Content block
  5. Wait

Two things can happen.

  1. The block displays "This block has encountered an error and cannot be previewed."

In the console the message is

Uncaught Error: Maximum update depth exceeded. This can happen when a component repeatedly calls setState inside componentWillUpdate or componentDidUpdate. React limits the number of nested updates to prevent infinite loops.
at checkForNestedUpdates (react-dom.82e849f1.js:23228)
at scheduleUpdateOnFiber (react-dom.82e849f1.js:21299)
at dispatchAction (react-dom.82e849f1.js:15795)
at e (index.js:1)
at t (index.js:1)
at index.js:1
at p (index.js:1)
at index.js:1
at index.js:1
at Object.dispatch (index.js:1)

or
2. Infinite loop

@bobbingwide
Copy link
Contributor Author

I've found a new scenario where the post-content block causes the browser to hang.
Discovered in 10.4.1 and reproduced in 10.5.2.

I wrote a plugin called oik-patterns to load patterns directly from .htm files.
When a pattern contained the post-content block then the browser would hang.

I reduced the problem to the simplest... inserting a post-content block in a new post.

I've had multiple results:

  1. Block produces "This block has encountered an error and cannot be previewed."
  2. Browser hangs then becomes a WSOD.

@bobbingwide
Copy link
Contributor Author

This is the screenshot for the simplest scenario.
image

This is an animated gif for the problem with the pattern inserter.
Pre-requisite: a pattern containing the post-content block. In this example it's the main-body.htm pattern.

Steps

  1. Add a pattern.
  2. Choose the header pattern
  3. Notice the main-body.htm pattern changing - the inserted header is now showing up - since it's now part of the post content
  4. Attempt to insert the main-body
  5. Mouse pointer changes to a flashing hand.
  6. Browser locks up.
    pattern-insert-recursion

@skorasaurus skorasaurus added the [Type] Bug An existing feature does not function as intended label Mar 30, 2022
@annezazu annezazu added [Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") and removed [Feature] Full Site Editing labels Jul 24, 2023
@youknowriad
Copy link
Contributor

I believe a solution for this have been already implemented. Closing for now.

cc @mcsf

@mcsf
Copy link
Contributor

mcsf commented Aug 17, 2023

Correct. For the editor, there is now a RecursionProvider component, as well as simple conventions for preventing recursion on the front end. Originally implemented in #28405, #28456, #31455.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") [Type] Bug An existing feature does not function as intended
Projects
None yet
7 participants