fix: store portal restore token under the right source ID

[aiddya]

Description of Change

XDG Desktop Portal provides restore tokens to restore a previously selected PipeWire stream instead of prompting the user again. This restore token is single use only and it has to be replaced when the stream is completed/stopped.

BaseCapturerPipewire maintains two source IDs: one is initialized by the constructor for new sources (source_id_) and another is for capturing previously selected sources (selected_source_id_). The restore token was always being stored under source_id_, even if the capture was ongoing for selected_source_id_. This prevents a stream from being restored more than once. Fix that by storing the restore token under the selected source ID if it exists.

Fixes #40097

cc @VerteDinde (this might fix some of the duplicate permission dialog issues)

Checklist

• PR description included and stakeholders cc'd
• npm test passes
• PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Fixed some redundant permission dialogs while screen sharing on Wayland.

[grulja]

Hi, can you please target this to WebRTC upstream? I see you also have another related fixes, it's unfortunate these will land only for Electron and not for all the others.

I will test this myself, it will take a while for me to build Chromium as I haven't updated my checkout for a while, but I'm not sure it will fix the issue I'm seeing with Google Meet. I'm also not sure it's correct, usually the selected_source_id_ is expected to be the same as source_id_ as the client should only select a source we previously sent him through GetSourceList(). I will look and let you know.

[grulja]

It doesn't fix my issue with Google Meet, but I think I understand the issue you are trying to fix. I believe the main problem in your case is that once the stream is stopped, it will bump the source_id so it will no longer match the selected_source_id_. This is not a problem in Chrome since usually when the stream is stopped, the DesktopCapturer instance is destroyed.

For Google Meet I can see it properly stores the token with source_id_ and tries to access it with selected_source_id_ while both are equal, yet it will open another portal dialog. I will keep digging.

[aiddya]

@grulja I would prefer to target upstream, but WebRTC's CLA requirement is a significant hurdle for me. My employer does not permit CLAs and I'll have to check if there's an exception process.

Did you check the D-Bus calls that were being made? It was helpful for me to debug this issue. I observed that Electron was not passing the restore token in SelectSources call, which led me to look into how source IDs and restore tokens were managed. I think the bug that you're seeing is in Electron as well as I've observed that Slack is unable to restore the stream even once.

[aiddya]

@grulja I looked further into the Google Meet issue. It appears that after the source is chosen, two desktop capturer instances are created in quick succession. The first instance takes the restore token, but is destroyed before it can replace it. When the second one tries to look for the token, it gets nothing. Here are some logs that I added to describe the issue:

[538330:538330:1005/203914.785878:ERROR:desktop_capture.cc(112)] Creating window capturer from DesktopCapture
[538330:538330:1005/203914.787846:ERROR:base_capturer_pipewire.cc(60)] Instantiated with source ID: 1
[538330:538330:1005/203914.788420:ERROR:desktop_capture.cc(97)] Creating screen capturer from DesktopCapture
[538330:538330:1005/203914.788495:ERROR:base_capturer_pipewire.cc(60)] Instantiated with source ID: 2
[538330:538921:1005/203928.183964:ERROR:base_capturer_pipewire.cc(211)] Instance 2: Selecting source ID 2
[538330:538921:1005/203929.201088:ERROR:base_capturer_pipewire.cc(211)] Instance 2: Selecting source ID 2
[538330:538330:1005/203929.511695:ERROR:base_capturer_pipewire.cc(89)] Instance 2: Storing token 07ec33a7-ba27-4683-bfb1-6a12be991f49 under source ID 2
[538330:538921:1005/203930.201934:ERROR:base_capturer_pipewire.cc(211)] Instance 2: Selecting source ID 2
[538330:538921:1005/203931.224216:ERROR:base_capturer_pipewire.cc(211)] Instance 2: Selecting source ID 2
[538330:538921:1005/203931.370653:ERROR:base_capturer_pipewire.cc(66)] Destroying instance 2
[538330:538920:1005/203931.377076:ERROR:base_capturer_pipewire.cc(66)] Destroying instance 1
[538330:538384:1005/203931.463943:ERROR:desktop_capture_device.cc(798)] Creating screen capturer from DesktopCaptureDevice for source ID 2
[538330:538384:1005/203931.464109:ERROR:base_capturer_pipewire.cc(60)] Instantiated with source ID: 3
[538330:538384:1005/203931.464192:ERROR:base_capturer_pipewire.cc(211)] Instance 3: Selecting source ID 2
[538330:539620:1005/203931.465545:ERROR:base_capturer_pipewire.cc(156)] Instance 3: Attempting to restore source ID 2 using restore token 07ec33a7-ba27-4683-bfb1-6a12be991f49
[538330:539620:1005/203931.505070:ERROR:base_capturer_pipewire.cc(66)] Destroying instance 3
[538330:538384:1005/203931.510572:ERROR:desktop_capture_device.cc(798)] Creating screen capturer from DesktopCaptureDevice for source ID 2
[538330:538384:1005/203931.510656:ERROR:base_capturer_pipewire.cc(60)] Instantiated with source ID: 4
[538330:538384:1005/203931.510705:ERROR:base_capturer_pipewire.cc(211)] Instance 4: Selecting source ID 2
[538330:539624:1005/203931.512228:ERROR:base_capturer_pipewire.cc(156)] Instance 4: Attempting to restore source ID 2 using restore token 
[538330:538330:1005/203934.032677:ERROR:base_capturer_pipewire.cc(89)] Instance 4: Storing token 327a403f-8616-42fb-a6f4-5e9b7a0c73b7 under source ID 2
[538330:539624:1005/203938.478562:ERROR:base_capturer_pipewire.cc(66)] Destroying instance 4

The first two BaseCapturerPipeWire instances are from the Chromium picker. The restore token is stored correctly, but is taken by a transient third instance of BaseCapturerPipeWire, which lasts just a few milliseconds. The fix has to somehow avoid creating this transient instance. Delaying the destruction of BaseCapturerPipeWire is not a fix, as it'll just create a race between the portal sending a restore token and the BaseCapturerPipeWire trying to read it.

This PR does fix a valid issue in Electron, but there's probably another issue that has to be fixed to avoid the duplicate permission dialogs.

[grulja]

Interesting. I can certainly see in that scenario why it breaks. I wonder whether we can do a simple fix to both of our issues by just making RestoreManager::TakeToken(SourceId id) not to remove the token from the list. It fixes the issue for me and I believe it should fix yours issue. Still probably the right fix would be not to create the redundant DesktopCapture instance, but could that be a GMeet issue? I don't see this behavior with the WebRTC testing page for example.

[grulja]

I opened a bug and added you to CC: https://bugs.chromium.org/p/webrtc/issues/detail?id=15544
And also opened a merge request: https://webrtc-review.googlesource.com/c/src/+/322621

It should fix both issues.

[aiddya]

I think simply changing RestoreManager to not delete tokens on fetching them is not the right fix by itself, because XDG Desktop Portal returning the same token is an implementation detail. The portal documentation on restore_token says:

If the stored session cannot be restored, this value is ignored and the user will be prompted normally. This may happen when, for example, the session contains a monitor or a window that is not available anymore, or when the stored permissions are withdrawn.

The restore token is invalidated after using it once. To restore the same session again, use the new restore token sent in response to starting this session.

This design choice is inherently racy, as the process to use and replenish a token is not an atomic operation. This is especially true for web browsers, where a stream could be restored multiple times in different threads due to website code. XDG Desktop Portal should document that the restore token will be retained if the stream is restored.

It might be okay to rely on the implementation detail for now and not delete tokens, but only for race conditions. The returned restore token should be stored under the right source ID to handle other cases mentioned in the documentation.

[aiddya]

Replaced with the upstream patch, which subsumes my previous patch and includes an additional fix for race conditions.

[aiddya]

Can we land this before #40114? The patch is included in #40114, so if that PR lands first, I'll have to manually backport this to all the stable branches.

[release-clerk]

Release Notes Persisted

Fixed some redundant permission dialogs while screen sharing on Wayland.

[trop]

I have automatically backported this PR to "26-x-y", please check out #40191

[trop]

I have automatically backported this PR to "28-x-y", please check out #40192

[trop]

I have automatically backported this PR to "27-x-y", please check out #40193