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

Delay removing iframe to fix renderer crash #6952

Merged
merged 1 commit into from
Jan 17, 2023
Merged

Conversation

Nokel81
Copy link
Collaborator

@Nokel81 Nokel81 commented Jan 16, 2023

Signed-off-by: Sebastian Malton sebastian@malton.name

fixes #6934

Signed-off-by: Sebastian Malton <sebastian@malton.name>
@Nokel81 Nokel81 added bug Something isn't working blocker labels Jan 16, 2023
@Nokel81 Nokel81 added this to the 6.4.0 milestone Jan 16, 2023
@Nokel81 Nokel81 requested a review from a team as a code owner January 16, 2023 21:44
@Nokel81 Nokel81 requested review from jansav and jim-docker and removed request for a team January 16, 2023 21:44
parentElem.removeChild(iframe);

// Must only remove iframe from DOM after it unloads old code. Else it crashes
iframe.addEventListener("load", () => parentElem.removeChild(iframe), {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"load"? How does this work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That event even fires when the load fails.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I don't see where the "load" event gets called in this situation. By iframe.setAttribute("src", "");?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://developer.mozilla.org/en-US/docs/Web/API/Window/load_event

This means that the DOM and all script resources have been loaded.

An empty src means "no src" or empty since that is the default.

@jim-docker
Copy link
Contributor

There's still an issue with connection races but the renderer process is not getting killed anymore.

Screen.Recording.2023-01-16.at.5.46.43.PM.mov

Plus the context menu doesn't always come up right away, or respond right away.

Copy link
Contributor

@jim-docker jim-docker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving as it fixes the crash. I discovered that this bug is manifested by #6851 (bug exists at that PR commit, but not if you check out the commit just before)

@aleksfront
Copy link
Contributor

What kind of renderer crash it fixes? This one #6934?

@Nokel81
Copy link
Collaborator Author

Nokel81 commented Jan 17, 2023

@aleksfront Yes

@Nokel81 Nokel81 merged commit 8ace84f into master Jan 17, 2023
@Nokel81 Nokel81 deleted the fix-renderer-crash branch January 17, 2023 13:55
@Nokel81 Nokel81 mentioned this pull request Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blank window when cluster disconnected
3 participants