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
Block Editor: fix crash when unmounting an editor iframe #59992
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Size Change: -12 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I originally had a go at fixing this bug in #59988 I'm familiar with the code and the logic here seems solid.
This fix is better than my effort because it retains the reference to the defaultView
so that the event listener can be removed correctly.
Thanks everyone, it's funny how we fixed the same bug within a 3 hours window 🙂 |
…9992) Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org> Co-authored-by: getdave <get_dave@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Fixes a crash that is reproducible like this:
Open Site Editor, and select a Header area:
The sidebar shows several preview iframes in the "Replace" area.
Now switch from the "Block" tab to the "Template" tab. That unmounts these preview iframes, but also leads to a crash!
This is because the cleanup of the offending
useRefEffect
runs when the iframe element is already removed from DOM. And at that time itscontentWindow
isnull
.The solution is to "hold on" to a reference to the window in a local
nodeWindow
variable. That solves the crash.In theory, the
contentWindow
of an iframe can change when the iframe navigates to a different URL, but the existing code doesn't account for that anyway -- it would be removing a listener from a window different from the one it added the listener to.