Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Fix a crash when removing persistent widgets #10063

Closed
wants to merge 1 commit into from

Conversation

robintown
Copy link
Member

@robintown robintown commented Feb 2, 2023

When a persistent widget is removed, multiple calls to updateShowWidgetInPip happen in succession as each of the widget stores emit updates. But by depending on this.state.persistentWidgetId at the time of the call rather than passing an update function to setState, this had the effect that the removal of the widget could be reverted in the component's state, and so it could end up passing the ID of a removed widget to WidgetPip.

Closes element-hq/element-web#24412


Here's what your changelog entry will look like:

🐛 Bug Fixes

When a persistent widget is removed, multiple calls to updateShowWidgetInPip happen in succession as each of the widget stores emit updates. But by depending on this.state.persistentWidgetId at the time of the call rather than passing an update function to setState, this had the effect that the removal of the widget could be reverted in the component's state, and so it could end up passing the ID of a removed widget to WidgetPip.
@robintown robintown added the T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems label Feb 2, 2023
@robintown robintown requested a review from a team as a code owner February 2, 2023 16:29
Copy link
Contributor

@weeman1337 weeman1337 left a comment

Choose a reason for hiding this comment

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

Okay, but one question.

Bonus points if you can add a test for this 😉 If there is an issue and we don't need to add/change any test besides fixing the error, our tests are not good enough.

persistentWidgetId = this.state.persistentWidgetId,
persistentRoomId = this.state.persistentRoomId,
): void {
private updateShowWidgetInPip(): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

This changed from public to private. Should we keep it public?

@t3chguy t3chguy added the X-Release-Blocker This affects the current release cycle and must be solved for a release to happen label Feb 6, 2023
@andybalaam
Copy link
Contributor

Closing in favour of #10099 which just makes sure we don't make a method private without thinking through the interface change.

@robintown please feel free to fix the public/private thing later if you think it's the right change. Also, more importantly, it would be great if we could test this, as Michael mentioned.

@andybalaam andybalaam closed this Feb 7, 2023
@robintown
Copy link
Member Author

OK, thank you for helping get this merged. I left this PR alone because a unit test is insufficient to test for this defect, and I didn't have time this particular week to troubleshoot my Cypress setup and learn how to write Cypress tests. I will put testing for this on my to-do list.

@andybalaam
Copy link
Contributor

Brilliant, thank you for fixing it @robintown , and thank you in advance for writing the test!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems X-Release-Blocker This affects the current release cycle and must be solved for a release to happen
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Removing BBB widget for everyone crashes Element
4 participants