search

flatpak / xdg-desktop-portal

Desktop integration portal
https://flatpak.github.io/xdg-desktop-portal/
GNU Lesser General Public License v2.1
603 stars 194 forks source link

Closing a request can fail due to backend not having created a request object yet. #854

Open robert-ancell opened 2 years ago

robert-ancell commented 2 years ago

An application quickly closing a request (e.g. to cancel a file chooser dialog) can result in a race condition where this close event is not handled in the backend.

For example:

  1. An application calls org.freedesktop.portal.FileChooser.OpenFile.
  2. xdg-desktop-portal creates an org.freedesktop.portal.Request object to handle this request. It listens to the Close method call on this object.
  3. xdg-desktop-portal calls org.freedesktop.impl.portal.FileChooser.OpenFile and passes the object path of the request create in step 2.
  4. xdg-desktop-portal returns the path of the request object to the application.
  5. Once xdg-desktop-portal-gnome receives the org.freedesktop.impl.portal.FileChooser.OpenFile request it creates an org.freedesktop.impl.portal.Request with the provided object path. It listens for the Close method call on this object to close the dialog.

If the application calls org.freedesktop.portal.Request.Close before step 5 above, xdg-desktop-portal will receive this and then call org.freedesktop.impl.portal.Request.Close and fail due to the backend not having yet created this object:

GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod: Object does not exist at path “/org/freedesktop/portal/desktop/request/1_6909/dart2665461754”
robert-ancell commented 2 years ago

A possible solution would be to add a new org.freedesktop.impl.portal.FileChooser.Close method that would pass the object path of the request. In this was the request object path is just being used as a key.

ids1024 commented 2 years ago

Technically a DBus service could probably handle a method call on a path that it hasn't defined on object at, but that may not be easy/possible in the libdbus/gdbus/etc. With higher level APIs anyway.

If the portal backend implements org.freedesktop.DBus.ObjectManager, then xdg-desktop-portal could listen for the InterfacesAdded signal to detect when the backend has created a Request at the path in question, and send Close() only once it has done so. But that may be making assumptions about how ObjectManager is implemented, and has the potential to have the close queued indefinitely if the backend never creates a Request.

ids1024 commented 2 years ago

Or I guess "queuing indefinitely" isn't really an issue here, since xdg-desktop-portal knows the request has been closed once the org.freedesktop.impl.portal.FileChooser.OpenFile call returns, whether or not the backend created a Request object.

So a robust strategy here in xdg-desktop-portal might be:

I believe that would fix the issue without any change to the backend, as long as it has a (correct) ObjectManager implementation. While otherwise having the same race condition but not otherwise introducing issues.

robert-ancell commented 2 years ago

To avoid the requirement on object manager the close could simply be retried using a timer, as the likely issue is simply the backend being a little bit slow. On the worst case this would have the portal causing Close periodically during the whole duration of the request, though in practice I would only expect the retry to be done one or two times. Using an exponential backoff would reduce the overhead of these replies in the worst case.

ids1024 commented 2 years ago

True. Just using a timer without exponential backoff would probably work pretty well too. Using a timer without any kind of backoff would probably not be so good, since the timer would either have to be longer than the time backoff would start at, or it would potentially spam a lot of calls. Though a good backend implementation should implement all methods to either return or create the Request fairly quickly.

ids1024 commented 2 years ago

Or even without any backoff it might not be too bad given it wouldn't send a Close until the previous one received an error response, adding some more delay and making sure it isn't queuing up more than one message for the backend.