Fix crash by moving editor style logic into a hook with useMemo #53596
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
noahtallen
added
[Type] Bug
|
Size Change: +2.1 kB (0%) Total Size: 1.51 MB
ℹ️ View Unchanged
|
noahtallen
changed the title
Mamaduka
reviewed
Aug 12, 2023
|
I was thinking that maybe we should call ‘mapSelect’ a second time in dev-mode and compare with latest value. This would help us catch bugs like this early in the development. Happy to push draft PR next week of this. Cc @jsnajdr, @youknowriad |
youknowriad
added
the
Backport to WP Minor Release
Mamaduka
approved these changes
Aug 14, 2023
jsnajdr
reviewed
Aug 14, 2023
Yes, this looks like a good idea and I'm curious how many Gutenberg bugs it will catch once implemented. React's own But |
tyxla
approved these changes
Aug 14, 2023
jsnajdr
requested changes
Aug 14, 2023
|
Hey peeps, would be good to get this merged soon so it can go into 6.3.1. There's been reports of the editor crashing with 6.3 here which may be related to the bug fixed by this PR. |
|
The feedback should be addressed now. It would be great to get a re-review since I moved the whole logic inside the |
jsnajdr
approved these changes
Aug 15, 2023
tellthemachines
pushed a commit
that referenced
this pull request
Aug 16, 2023
* Move editor style logic into a hook whith useMemo * Remove unnecessary useMemo * Move the whole logic inside the 'useMemo' * Add missing useSelect dep --------- Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com>
tellthemachines
removed
the
Backport to WP Minor Release
tellthemachines
added a commit
that referenced
this pull request
Aug 16, 2023
* Footnotes: Fix recursion into updating attributes when attributes is not an object (#53257) * Fix crash by moving editor style logic into a hook with useMemo (#53596) * Move editor style logic into a hook whith useMemo * Remove unnecessary useMemo * Move the whole logic inside the 'useMemo' * Add missing useSelect dep --------- Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com> * Adding an is_array check before using count in case $footnotes is not countable (#53660) * Footnotes: fix accidental override (#53663) * Footnotes: fix accidental override * Remove double quotes * Footnotes: autosave is not slashing JSON (#53664) * Footnotes: autosave saves decoded JSON * wp_slash * Curly quote * Slash on save and restore * Ensure the preview dropdown popover closes (<16.3) for e2e tests --------- Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com> Co-authored-by: Noah Allen <noahtallen@gmail.com> Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com> Co-authored-by: Ramon <ramonjd@users.noreply.github.com> Co-authored-by: Ella <4710635+ellatrix@users.noreply.github.com> Co-authored-by: ramon <ramonjd@gmail.com>
|
I believe the bug I'm experiencing is related to this same issue. Do you guys have an idea when a patch will be released?
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Follow-up to #52767 which optimizes the editor style calculation with
useMemoto prevent possible infinite render loops.Why?
After installing Gutenberg 16.4 on a large multisite instance (WordPress.com), we began noticing editor crashes, with React giving the "maximum update depth exceeded". After debugging and
git bisect, I tracked this to #52767, and reverting that PR fixes the issue.After some experimentation, I found that the issue is prevented when we avoid creating new styles arrays on every
useSelectcall.From what I can tell, this crash happens in two scenarios:
editorSettings.stylesmatchesstyle.__unstableType && style.__unstableType !== 'theme'In either of those cases, the array with an unstable reference is returned and the editor crashes.
@ellatrix, can you verify that this makes sense and I'm not breaking anything?
How?
I moved the styles logic to its own hook and used
useMemoto maintain array references. Some of the logic was adjusted to make that easier (such as using array filter for thepresetStylesarray, and basinghasThemeStyleson whether every style is a preset or not).Testing Instructions
I don't know how to reproduce this outside of our environment, but I have verified the fix there. So I think this will follow the same testing steps as #52767, which is that the e2e test passes. (Which it is!)
Testing Instructions for Keyboard
Screenshots or screencast