1748647 - MIDI messages are delivered twice if the same port is opened twice

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gabriele Svelto [:gsvelto]]

STR:

1. Open https://mikatahara.github.io/MidiMonitor/ in a tab
2. Open https://www.midimonitor.com/ in another tab
3. Press some keys on your device

The expected behavior is to see single notes appear on both tabs, what happens instead is that in the second tab every note is delivered twice. I've verified that this issue happens both on Linux and macOS.

Comment 1

[Gabriele Svelto [:gsvelto]]

Alright, the issue here is that we're re-opening a port which is already open. I haven't thought much about how the state of ports should be handled when multiple pages touch them. Mostly because I thought that it shouldn't be possible... but it actually is which will make for some interesting corner cases.

Comment 2

[Gabriele Svelto [:gsvelto]]

After doing some experiments I confirmed that the state of the DOM MIDI ports in a page should be decoupled from the actual ports. That is a page can close a port while another page keeps it open. I honestly thought it would not be possible to share ports which means this now needs some design changes, not just a local fix.

Comment 3

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1748647 - Simplify initialization r=padenot

Comment 4

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1748647 - Split DOM MIDI port state from the actual state of the ports r=padenot

The goal is to make sure that several pages have a coherent view of
potentially shared port. If both page A and page B open an input port they
should both receive messages. If page B closes it, then port A will keep
receiving messages and still see the port open, but B won't and will see it
closed.

This is achieved by sending status changes based on the parent objects of
every DOM midi port instead of broadcasting them to every port. Additionally
the midir implementation now counts the number of times a port has been opened
so that it won't open the same twice or close one too soon.

Depends on D136223

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f5ef2e24b814
Simplify initialization r=padenot
https://hg.mozilla.org/integration/autoland/rev/59dcfab2b5b7
Split DOM MIDI port state from the actual state of the ports r=padenot

Comment 6

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/f5ef2e24b814
https://hg.mozilla.org/mozilla-central/rev/59dcfab2b5b7

Comment 8

[Gabriele Svelto [:gsvelto]]

Comment on attachment 9259522 [details]
Bug 1748647 - Split DOM MIDI port state from the actual state of the ports r=padenot

Beta/Release Uplift Approval Request

• User impact if declined: Web MIDI behavior is incorrect - and potentially leads to a crash - whenever more than one website uses the API at the same time
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Web MIDI is still pref'd off and this is the last stabilization patch that needs to land to provide a solid experience
• String changes made/needed: none

Comment 9

[Ryan VanderMeulen [:RyanVM]]

This is hitting conflicts due to bug 1743462. Should we nominate that for Beta uplift as well?

Comment 10

[Gabriele Svelto [:gsvelto]]

Yeah, that's definitely harmless.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9259521 [details]
Bug 1748647 - Simplify initialization r=padenot

Approved for 97.0b8.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/01286648c0b0
https://hg.mozilla.org/releases/mozilla-beta/rev/a294dd5a4a8d