[pve-devel] [PATCH widget-toolkit] fix #4421: ui: guard setProxy against races of slow vs fast requests

[pve-devel] [PATCH widget-toolkit] fix #4421: ui: guard setProxy against races of slow vs fast requests

[Friedrich Weber]

Some UI components use `Ext.data.Store.setProxy` to change their
associated API endpoint URL in reaction to user input. One example is
`BackupView`, which calls `setProxy` when the user switches from
listing backups on storage A to listing backups on storage B. However,
if A is slow, the UI may receive the response for A *after* the
response for B. It will then display the contents of A as if they were
the contents of B, resulting in a UI inconsistency.

The reason is that `Ext.data.Store` still processes the slow response
for A, even though it is obsolete. This patch overrides the
responsible callback of `Ext.data.Store` to only process responses
belonging to the currently active proxy object. This should rule out
similar race conditions in all components that use the `setProxy` API.
In the above example, the patch results in the response for A being
ignored.

Ignored responses are logged to the browser console.

Note that this patch only concerns components that use `setProxy` for
changing API endpoints. Other components (e.g. those using
`proxy.setURL` for the same purpose) may be open to similar race
conditions.

Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
---
 The original report only concerns the backup view [1], where the race
 condition is easy to trigger. While ruling out this particular race is
 simple, I thought it would be worthwhile to rule out race condition of
 this category for all components. Hence this patch. However, most of the
 other races are much harder to trigger, so it may be questionable
 whether a general fix is needed. So if wanted, I can alternatively
 submit a patch that only fixes the backup view.

 Also, there are several occurrences of the `proxy.setURL` or `proxy.url = ...`
 patterns (see [1]) which are also susceptible to race conditions, and which
 are not fixed by this patch. However, for those, I have not found a nice
 solution that does not involve changing a lot of call sites. If wanted, I can
 give it another try, or alternatively only submit patches for components for
 which triggering the race conditions seems realistic.

 [1] https://bugzilla.proxmox.com/show_bug.cgi?id=4421

 src/Utils.js | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/src/Utils.js b/src/Utils.js
index f55b9a5..8a97487 100644
--- a/src/Utils.js
+++ b/src/Utils.js
@@ -1451,3 +1451,18 @@ Ext.define('Proxmox.Async', {
 	return new Promise((resolve, _reject) => setTimeout(resolve, millis));
     },
 });
+
+Ext.override(Ext.data.Store, {
+    // If the store's proxy is changed while it is waiting for an AJAX
+    // response, `onProxyLoad` will still be called for the outdated response.
+    // To avoid displaying inconsistent information, only process responses
+    // belonging to the current proxy.
+    onProxyLoad: function(operation) {
+	let me = this;
+	if (operation.getProxy() === me.getProxy()) {
+	    me.callParent(arguments);
+	} else {
+	    console.log(`ignored outdated response: ${operation.getRequest().getUrl()}`);
+	}
+    },
+});
-- 
2.30.2

Re: [pve-devel] [PATCH widget-toolkit] fix #4421: ui: guard setProxy against races of slow vs fast requests

[Thomas Lamprecht]

Am 06/03/2023 um 15:03 schrieb Friedrich Weber:
> Some UI components use `Ext.data.Store.setProxy` to change their
> associated API endpoint URL in reaction to user input. One example is
> `BackupView`, which calls `setProxy` when the user switches from
> listing backups on storage A to listing backups on storage B. However,
> if A is slow, the UI may receive the response for A *after* the
> response for B. It will then display the contents of A as if they were
> the contents of B, resulting in a UI inconsistency.
> 
> The reason is that `Ext.data.Store` still processes the slow response
> for A, even though it is obsolete. This patch overrides the
> responsible callback of `Ext.data.Store` to only process responses
> belonging to the currently active proxy object. This should rule out
> similar race conditions in all components that use the `setProxy` API.
> In the above example, the patch results in the response for A being
> ignored.
> 
> Ignored responses are logged to the browser console.
> 
> Note that this patch only concerns components that use `setProxy` for
> changing API endpoints. Other components (e.g. those using
> `proxy.setURL` for the same purpose) may be open to similar race
> conditions.
> 
> Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
> ---
>  The original report only concerns the backup view [1], where the race
>  condition is easy to trigger. While ruling out this particular race is
>  simple, I thought it would be worthwhile to rule out race condition of
>  this category for all components. Hence this patch. However, most of the
>  other races are much harder to trigger, so it may be questionable
>  whether a general fix is needed. So if wanted, I can alternatively
>  submit a patch that only fixes the backup view.

IMO a general fix/future proofing can be OK, so besides a small nit inline:
LGTM, but did not checked/tested this too closely - @Dominik, what do you
think on this?

> 
>  Also, there are several occurrences of the `proxy.setURL` or `proxy.url = ...`
>  patterns (see [1]) which are also susceptible to race conditions, and which
>  are not fixed by this patch. However, for those, I have not found a nice
>  solution that does not involve changing a lot of call sites. If wanted, I can
>  give it another try, or alternatively only submit patches for components for
>  which triggering the race conditions seems realistic.
> 
>  [1] https://bugzilla.proxmox.com/show_bug.cgi?id=4421
> 
>  src/Utils.js | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/src/Utils.js b/src/Utils.js
> index f55b9a5..8a97487 100644
> --- a/src/Utils.js
> +++ b/src/Utils.js
> @@ -1451,3 +1451,18 @@ Ext.define('Proxmox.Async', {
>  	return new Promise((resolve, _reject) => setTimeout(resolve, millis));
>      },
>  });
> +
> +Ext.override(Ext.data.Store, {
> +    // If the store's proxy is changed while it is waiting for an AJAX
> +    // response, `onProxyLoad` will still be called for the outdated response.
> +    // To avoid displaying inconsistent information, only process responses
> +    // belonging to the current proxy.
> +    onProxyLoad: function(operation) {
> +	let me = this;
> +	if (operation.getProxy() === me.getProxy()) {
> +	    me.callParent(arguments);
> +	} else {
> +	    console.log(`ignored outdated response: ${operation.getRequest().getUrl()}`);

maybe warn? also, no need to interpolate text on the console helpers, just add as
it's own param, which often gives a much better dev experience (as object can then
be inspected in full) - here it's just an URL (maybe dump the full thing?), so YNNV.

console.warn('ignored outdated response:', operation.getRequest().getUrl());

> +	}
> +    },
> +});

Re: [pve-devel] [PATCH widget-toolkit] fix #4421: ui: guard setProxy against races of slow vs fast requests

[Dominik Csapak]

On 3/7/23 19:49, Thomas Lamprecht wrote:
> Am 06/03/2023 um 15:03 schrieb Friedrich Weber:
>> Some UI components use `Ext.data.Store.setProxy` to change their
>> associated API endpoint URL in reaction to user input. One example is
>> `BackupView`, which calls `setProxy` when the user switches from
>> listing backups on storage A to listing backups on storage B. However,
>> if A is slow, the UI may receive the response for A *after* the
>> response for B. It will then display the contents of A as if they were
>> the contents of B, resulting in a UI inconsistency.
>>
>> The reason is that `Ext.data.Store` still processes the slow response
>> for A, even though it is obsolete. This patch overrides the
>> responsible callback of `Ext.data.Store` to only process responses
>> belonging to the currently active proxy object. This should rule out
>> similar race conditions in all components that use the `setProxy` API.
>> In the above example, the patch results in the response for A being
>> ignored.
>>
>> Ignored responses are logged to the browser console.
>>
>> Note that this patch only concerns components that use `setProxy` for
>> changing API endpoints. Other components (e.g. those using
>> `proxy.setURL` for the same purpose) may be open to similar race
>> conditions.
>>
>> Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
>> ---
>>   The original report only concerns the backup view [1], where the race
>>   condition is easy to trigger. While ruling out this particular race is
>>   simple, I thought it would be worthwhile to rule out race condition of
>>   this category for all components. Hence this patch. However, most of the
>>   other races are much harder to trigger, so it may be questionable
>>   whether a general fix is needed. So if wanted, I can alternatively
>>   submit a patch that only fixes the backup view.
> 
> IMO a general fix/future proofing can be OK, so besides a small nit inline:
> LGTM, but did not checked/tested this too closely - @Dominik, what do you
> think on this?
> 

This change is non-intrusive enough that it's OK, since it
fixes the reported issue and potentially some more.

When we're only fixing the one reported place, i guess
sooner or later someone else reports another instance of this,
and by then we probably forgot that we fixed it already once ;)

Really fixing all points where something like that can happen is
not easy since most of them are using Proxmox.Utils.API2Request
instead of a store, or as Friedrich already wrote, setting
the URL of the proxy manually, so this seems to be good
middle ground for now.

[pve-devel] applied: [PATCH widget-toolkit] fix #4421: ui: guard setProxy against races of slow vs fast requests

[Thomas Lamprecht]

Am 06/03/2023 um 15:03 schrieb Friedrich Weber:
> Some UI components use `Ext.data.Store.setProxy` to change their
> associated API endpoint URL in reaction to user input. One example is
> `BackupView`, which calls `setProxy` when the user switches from
> listing backups on storage A to listing backups on storage B. However,
> if A is slow, the UI may receive the response for A *after* the
> response for B. It will then display the contents of A as if they were
> the contents of B, resulting in a UI inconsistency.
> 
> The reason is that `Ext.data.Store` still processes the slow response
> for A, even though it is obsolete. This patch overrides the
> responsible callback of `Ext.data.Store` to only process responses
> belonging to the currently active proxy object. This should rule out
> similar race conditions in all components that use the `setProxy` API.
> In the above example, the patch results in the response for A being
> ignored.
> 
> Ignored responses are logged to the browser console.
> 
> Note that this patch only concerns components that use `setProxy` for
> changing API endpoints. Other components (e.g. those using
> `proxy.setURL` for the same purpose) may be open to similar race
> conditions.
> 
> Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
> ---
>  The original report only concerns the backup view [1], where the race
>  condition is easy to trigger. While ruling out this particular race is
>  simple, I thought it would be worthwhile to rule out race condition of
>  this category for all components. Hence this patch. However, most of the
>  other races are much harder to trigger, so it may be questionable
>  whether a general fix is needed. So if wanted, I can alternatively
>  submit a patch that only fixes the backup view.
> 
>  Also, there are several occurrences of the `proxy.setURL` or `proxy.url = ...`
>  patterns (see [1]) which are also susceptible to race conditions, and which
>  are not fixed by this patch. However, for those, I have not found a nice
>  solution that does not involve changing a lot of call sites. If wanted, I can
>  give it another try, or alternatively only submit patches for components for
>  which triggering the race conditions seems realistic.
> 
>  [1] https://bugzilla.proxmox.com/show_bug.cgi?id=4421
> 
>  src/Utils.js | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
>

applied, thanks!