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

Fix infinite event listener ping-pong destroying settings page #1308

Merged
merged 2 commits into from
Aug 13, 2024

Conversation

Kuuuube
Copy link
Member

@Kuuuube Kuuuube commented Aug 13, 2024

Regression introduced in #1270 (ecd9ffc).

The newly introduced 'element' type for dom data binder to update things which are not any type of input (checkboxes, text inputs, etc) can cause an infinite loop when updating the value.

There is a potential ping-pong between

_applyValues(targets, response, ignoreStale) {
for (let i = 0, ii = targets.length; i < ii; ++i) {
const [observer, task] = targets[i];
const {error, result} = response[i];
const stale = (task !== null && task.stale);
if (error) {
if (typeof this._onError === 'function') {
this._onError(error, stale, observer.element, observer.metadata);
}
continue;
}
if (stale && !ignoreStale) { continue; }
observer.value = result;
observer.hasValue = true;
this._setElementValue(observer.element, result);
}
}
and
_onObserverChildrenUpdated(element, observer) {
if (!observer.hasValue) {
return;
}
this._setElementValue(element, observer.value);
}

When the value is updated by one of them, the other triggers, updating it again, triggering the other, and so on. This can occur any time the dictionary aliases in the settings page are updated, including when it is initially loaded.

I've added a guard against 'element' being updated by _onObserverChildrenUpdated. The 'element' type can be expected to have special handling in all cases and should not need to use this.

I've also reverted the styling change that was applied to _onObserverChildrenUpdated in the dictionary aliases pr to make calling this._setElementValue(element, observer.value); appear as a more explicit action instead of a default action.

Tested on Firefox and Chromium.

@Kuuuube Kuuuube added kind/bug The issue or PR is regarding a bug area/performance The issue or PR is related to performance area/ui-ux The issue or PR is related to UI/UX/Design priority/high High priority issue/PR regression This issue or PR is related to a regression labels Aug 13, 2024
@Kuuuube Kuuuube requested a review from a team as a code owner August 13, 2024 00:25
@jamesmaa jamesmaa added this pull request to the merge queue Aug 13, 2024
Merged via the queue into themoeway:master with commit a7c19ce Aug 13, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/performance The issue or PR is related to performance area/ui-ux The issue or PR is related to UI/UX/Design kind/bug The issue or PR is regarding a bug priority/high High priority issue/PR regression This issue or PR is related to a regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants