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

Racy devicechange event design has poor interoperability #972

Closed
jan-ivar opened this issue Sep 26, 2023 · 9 comments · Fixed by #996
Closed

Racy devicechange event design has poor interoperability #972

jan-ivar opened this issue Sep 26, 2023 · 9 comments · Fixed by #996

Comments

@jan-ivar
Copy link
Member

The devicechange event follows § 7.7. Use plain Events for state, but the "state information in the target object." is not available synchronously, so developers have to the following:

navigator.mediaDevices.ondevicechange = async () => {
  const devices = await navigator.mediaDevices.enumerateDevices();
  // examine devices and compare against oldDevices to detect changes once available
  oldDevices = devices;
}

But this is racy. By the time you're finally examining the devices, 100+ milliseconds may have passed. A lot may have happened during that time, including more devicechange events, which the spec allows.

Theoretical? No. I've instrumented this fiddle from #966 (comment), where I keep AirPods in their case initially, then put them on, in macOS Ventura.

Firefox is fine (baseline):

ENUMERATEDEVICES 1 BEGIN...
...ENUMERATEDEVICES 1 END.
ENUMERATEDEVICES 2 BEGIN...
...ENUMERATEDEVICES 2 END.
DEVICECHANGE 1 BEGIN...
ENUMERATEDEVICES 3 BEGIN...
...ENUMERATEDEVICES 3 END.
1: Switching to inserted AirPods
...DEVICECHANGE 1 END.

The first two are me calling enumerateDevices before and after getUserMedia, like you're supposed to, to learn of all devices.

But here's Chrome:

ENUMERATEDEVICES 1 BEGIN...
...ENUMERATEDEVICES 1 END.
ENUMERATEDEVICES 2 BEGIN...
...ENUMERATEDEVICES 2 END.
DEVICECHANGE 1 BEGIN...
ENUMERATEDEVICES 3 BEGIN...
DEVICECHANGE 2 BEGIN...
ENUMERATEDEVICES 4 BEGIN...
DEVICECHANGE 3 BEGIN...
ENUMERATEDEVICES 5 BEGIN...
DEVICECHANGE 4 BEGIN...
ENUMERATEDEVICES 6 BEGIN...
...ENUMERATEDEVICES 3 END.
1: ignoring duplicate event
...ENUMERATEDEVICES 4 END.
2: ignoring duplicate event
...ENUMERATEDEVICES 5 END.
3: ignoring duplicate event
...ENUMERATEDEVICES 6 END.
4: Default - MacBook Pro Microphone (Built-in) removed; reverting to Default - MacBook Pro Microphone (Built-in)
...DEVICECHANGE 4 END.
DEVICECHANGE 5 BEGIN...
ENUMERATEDEVICES 7 BEGIN...
DEVICECHANGE 6 BEGIN...
ENUMERATEDEVICES 8 BEGIN...
...ENUMERATEDEVICES 7 END.
5: ignoring duplicate event
...ENUMERATEDEVICES 8 END.
6: Switching to inserted AirPods
...DEVICECHANGE 6 END.
DEVICECHANGE 7 BEGIN...
ENUMERATEDEVICES 9 BEGIN...
...ENUMERATEDEVICES 9 END.
7: AirPods removed; reverting to Default - AirPods
...DEVICECHANGE 7 END.
DEVICECHANGE 8 BEGIN...
ENUMERATEDEVICES 10 BEGIN...
...ENUMERATEDEVICES 10 END.
8: Default - MacBook Pro Microphone (Built-in) removed; reverting to Default - AirPods
...DEVICECHANGE 8 END.

8 events are fired, and they're fired on top of each other, while the application is still processing the previous event(s) (awaiting enumerateDevices), causing interleaved code execution.

This would look even worse if I didn't write special guards against duplicate events, but this is still a nightmare to reason about.

This might look like an implementation bug on Chrome, except here's Safari:

ENUMERATEDEVICES 1 BEGIN...
...ENUMERATEDEVICES 1 END.
DEVICECHANGE 1 BEGIN...
ENUMERATEDEVICES 2 BEGIN...
ENUMERATEDEVICES 3 BEGIN...
...ENUMERATEDEVICES 2 END.
...ENUMERATEDEVICES 3 END.
1: Switching to inserted MacBook Pro Microphone
...DEVICECHANGE 1 END.

...and so far I haven't even inserted my AirPods yet! This is #966 where Safari fires a devicechange event as part of getUserMedia, tricking my fiddle into thinking a device has been inserted.

As mentioned previously, my fiddle calls enumerateDevices right after getUserMedia like you're supposed to (and have to in other browsers). But here Safari fires devicechange on me, causing my app to again interleave two calls to enumerateDevices, making it hard to reason about. It may even expose it to the race in the OP, since AFAIK there's no spec guarantee that enumerateDevices calls resolve in order.

Safari continued:

DEVICECHANGE 2 BEGIN...
ENUMERATEDEVICES 4 BEGIN...
...ENUMERATEDEVICES 4 END.
2: MacBook Pro Microphone removed; reverting to MacBook Pro Microphone
...DEVICECHANGE 2 END.
DEVICECHANGE 3 BEGIN...
ENUMERATEDEVICES 5 BEGIN...
...ENUMERATEDEVICES 5 END.
3: Switching to inserted AirPods
DEVICECHANGE 4 BEGIN...
ENUMERATEDEVICES 6 BEGIN...
...ENUMERATEDEVICES 6 END.
4: MacBook Pro Microphone removed; reverting to AirPods
...DEVICECHANGE 3 END.
...DEVICECHANGE 4 END.

...the rest here is fairly decent, in that calls are not getting interleaved. But like Chrome there's some funny business going on with the default device. What's that about?

This seems like an interoperability issue. For example: Device switching in Google Meet works in Chrome today but not in Firefox, which I think shows it is possible to write app code that passes a testing matrix in one implementation, yet is not interoperable, unless other browsers implement the same side effects.

This seems like a spec issue. I propose we delay devicechange events to not fire while calls to enumerateDevices() are outstanding.

I'll file a separate issue on the non-standard "Default" device that Chrome and Safari add for microphone only.

@youennf
Copy link
Contributor

youennf commented Sep 28, 2023

The spec says a few things:

  • The User Agent MAY combine firing multiple events into firing one event when several events are due or when multiple devices are added or removed at the same time, e.g. a camera with a microphone.
  • These events are potentially triggered simultaneously on documents of different origins. User Agents MAY add fuzzing on the timing of events to avoid cross-origin activity correlation.(This is a fingerprinting vector.)

It seems hard to fix all races and have consistency between implementations.
If what we are after is allowing web pages to know that a device was inserted, exposing this information without relying on web page enumerateDevices()-based computation will probably give more interoperable results. It will also make web developer's life easier.

@guidou
Copy link
Contributor

guidou commented Sep 29, 2023

If what we are after is allowing web pages to know that a device was inserted, exposing this information without relying on web page enumerateDevices()-based computation will probably give more interoperable results.

I'm OK with adding a reason or similar field to the event as long as we keep it flexible enough not to make implementations not spec compliant for firing the event in a case that doesn't cleanly match one of the possible values in the field. A possible way to do it is by adding a other value as cause.

My view is that:

  1. Applications are more interested in knowing the latest state than the cause for a change in the state.
  2. Having a cause can help in some cases (it would be good to document those).
  3. As time passes, an implementation might find a new reason to fire the event, and it would be good if it could fire the event without breaking compliance with the spec.

@karlt
Copy link
Contributor

karlt commented Oct 5, 2023

I'm not clear what problem exists with firing "devicechange" events while an enumerateDevices() promise is pending. The "devicechange" event contains no information, so it is just a signal that a subsequent enumerateDevices() call may now return a different device list. The client app can compare with previous device lists to determine whether changes are significant. If a "devicechange" event occurs while waiting for an enumerateDevices() promise, then the client app knows that another enumerateDevices() call is necessary to get an up-to-date device list.

Trying to design the API to protect against bugs in client async code is not going to be something that the API can win. The client's response to the change in devices is likely to be async and I don't think we want to continue to delay "devicechange" events while any async operation is pending.

Something that might be helpful to clients would be to specify that enumerateDevices() promises be resolved in the same order as for the corresponding calls on the same MediaDevices.

@chrisguttandin
Copy link
Contributor

There are a few other APIs which follow a slightly different pattern than enumerateDevices().

matchMedia() for example returns a MediaQueryList which does implement an EventTarget and fires events whenever it changes.

requestMIDIAccess() returns a Promise which resolves with a MIDIAccess object. It has properties to synchronously access the inputs and outputs. But it implements an EventTarget, too. It fires a statechange event whenever the list of available devices changes. By the time the event is fired the inputs and outputs are already updated.

When applying the same concept to enumerateDevices() it could look a bit like this.

navigator.mediaDevices
    .enumerateDevices()
    .then((deviceList) => {
        // Log the latest list of devices.
        console.log(deviceList.devices);

        deviceList.onchange = () => {
            // Log the list of devices again.
            // The list got updated already just before the event fired.
            console.log(deviceList.devices);
        };
    });

The Permissions API is another API which follows this pattern.

I know that this would be a breaking change. But maybe there is a way to introduce something like this in a backwards compatible way.

@jan-ivar
Copy link
Member Author

           // The list got updated already just before the event fired.
           console.log(deviceList.devices);

...
The Permissions API is another API which follows this pattern.

That's a live state update model I wouldn't recommend for lists.

@jan-ivar
Copy link
Member Author

jan-ivar commented Oct 12, 2023

The simplest fix here IMHO would be to include the device list in the event:

navigator.mediaDevices.ondevicechange = ({devices}) => {
  // examine devices and compare against oldDevices to detect changes once available
  oldDevices = devices;
}

Async problem solved and 100% backwards compatible (modeled on trackEvent.streams).

@guidou
Copy link
Contributor

guidou commented Oct 12, 2023

The simplest fix here IMHO would be to include the device list in the event:

I'm OK with this change.

@jan-ivar
Copy link
Member Author

This was discussed in https://www.w3.org/2023/10/17-webrtc-minutes.html:

Conclusion: No objection.

@dontcallmedom-bot
Copy link

This issue had an associated resolution in WebRTC April 23 2024 meeting – 23 April 2024 (Racy devicechange event design has poor interoperability in Media Capture and Streams):

RESOLUTION: merged 972 with language clarified on current device list

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants