Closed Bug 1201598 Opened 7 years ago Closed 8 months ago

Implement Linux backend for WebMIDI

Categories

(Core :: DOM: Device Interfaces, task, P3)

task

Tracking

()

RESOLVED FIXED
97 Branch
Tracking Status
firefox97 --- fixed

People

(Reporter: qdot, Assigned: gsvelto)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

Implement platform specific linux backend for WebMIDI
See this retrospective for some needed kernel changes that I am (too slowly) working on:
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-June/093233.html
Also I would love to help with this. I have really zero time to contribute code right now but hopefully can give some guidance (and maybe some code eventually).
Making this depend on bug 1135640 since we're doing midi platform specifics in Rust.
Depends on: oxidation
Severity: normal → enhancement
Priority: -- → P3
Assignee: kyle → nobody
Type: enhancement → task

Paul I'm bumping into a silly issue while working on this. I tried some simple examples which request the midi access objects, pick an input port and stuff their handler in the onmidimessage event handler. I mash keys on my MIDI keyboard and they get delivered to the event handler via the midir implementation of the MIDI platform service. So far so good. After a while however the MIDIAccess object disappears and all the ports with it. Looking at the code flow I noticed that this is the only reference holding the MIDIAccess object alive. When my implementation sends the port list it gets cleared. But that's odd, because I would expect the onmidimessage handler to hold the MIDIInput/MIDIPort object alive and the MIDIAccess object with it.

Am I missing something? Maybe the MIDIPort objects should keep an explicit reference to the MIDIAccess object they've been obtained from? I must admit I haven't touched the DOM in a while and it shows.

Note this behavior can be seen even when using the TestMIDIPlatformService.cpp implementation, it's not specific to mine.

Flags: needinfo?(padenot)

Can you share your test page ? We might want to add an explicit ref indeed.

Flags: needinfo?(padenot)
Attached file GC'd MIDI ports test (obsolete) —

I ran into this issue on this page where it seems that the input port object gets destroyed before I have a chance to do anything with it. I also managed to reproduce the issue with this patch. If I remove the SpecialPowers.exactGC() call and just make the output.send([0x90, 0x00, 0x7F]); invocation directly the test passes. With the GC cycles in place however the input port goes away and the test hangs.

FYI I think I've narrowed down the issue but I'm still extremely puzzled by it. If I do the following on a MIDIInput object:

input.onmidimessage = (event) => { /* do stuff */ };

The event handler is called but the object is not kept alive unless I have another reference to it. On the other hand if I do this:

input.addEventHandler("midimessage", (event) => { /* do stuff */ });

It doesn't work at all! That is my handler is never invoked and the object is not held alive. I'm trying to figure out what's wrong but it's very puzzling because MIDIPort inherits from EventHandler and MIDIInput inherits from MIDIPort so I don't see where the problem could be coming from.

The rest of the implementation is shaping up nicely but I still have to add proper error-handling and fix some issues where some messages can be duplicated.

smaug, do you know the reason why we're seeing what comment 7 describes?

This particular handler is implemented somewhat manually, but it's not obvious to me what the difference would be between the two lines above ?

Flags: needinfo?(bugs)

I forgot something, if I do this:

input.addEventHandler("statechange", (event) => { /* do statechange stuff */ });
input.addEventHandler("midimessage", (event) => { /* do midimessage stuff */ });

The statechange handler gets called just fine, it's just the midimessage handler that doesn't. It's as if the event handlers declared in MIDIPort work but the ones in MIDIInput don't.

(In reply to Gabriele Svelto [:gsvelto] from comment #7)

FYI I think I've narrowed down the issue but I'm still extremely puzzled by it. If I do the following on a MIDIInput object:

input.onmidimessage = (event) => { /* do stuff */ };

The event handler is called but the object is not kept alive unless I have another reference to it. On the other hand if I do this:

Sounds like some issue in the midi implementation, someone isn't keeping a strong reference to the object.

input.addEventHandler("midimessage", (event) => { /* do stuff */ });

There is no such thing as addEventHandler. It is addEventListener

Flags: needinfo?(bugs)

(In reply to Olli Pettay [:smaug] from comment #11)

There is no such thing as addEventHandler. It is addEventListener

Yeah, sorry, typo in my comments. It's addEventListener() that simply doesn't appear to work on MIDIInput objects.

some minimal testcase would be useful here.

If you can do something like
input.addEventListener("foo", function(e) { console.log(e); });
input.dispatchEvent(new Event("foo"));

then at least the event handling is fine.

And does a debug build give any interesting warnings?

This patch applies to one of the test cases and highlights the issue: with the patch applied the test never finishes because the "midimessage" event handler is never called.

If you can do something like
input.addEventListener("foo", function(e) { console.log(e); });
input.dispatchEvent(new Event("foo"));

then at least the event handling is fine.

I tried and it seems like the event is not dispatched.

And does a debug build give any interesting warnings?

No, nothing at all. What's super odd is that "statechange" events work just fine on the same object. If in the test you replace:

input.onstatechange = (event) => { checkCallbacks(event.port); };

with

input.addEventListener("statechange", (event) => { checkCallbacks(event.port); });

that works!

Depends on: 1741568

This also updates the tests to reflect the changes in port state

Here's my WIP which mostly works but still relies on hacks (in the last two patches) and has a few limitations:

  • There's no support for detecting disconnects or new devices being added. This is down to midir not supporting this functionality directly and I assume some of the underlying implementation (I'm mostly thinking about Windows 7 here). It's possible to work around this by polling the port list once the service is running but I haven't gone there yet.
  • Error management is very limited, in part because it's not exposed directly in midir (e.g. there's no way to tell when an input port has been disconnected) and in part because I couldn't find a good way to make it fit.
  • Code using addEventListener("midimessage") doesn't work, see above.
  • Some sites (like www.midimonitor.com) pop up the permission dialog too many times and yield duplicate events, probably a bug somewhere in my code.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED

Alright, I figured out what the problem was and it's been sitting under my nose the whole time. The midimessage event listener needs to be special-cased in EventListenerManager::AddEventListenerInternal like we do with other event listeners which have an implicit behavior. Specifically we need to open the input port if the port is closed when the event listener is set like we do manually here.

BTW I wonder if we could refactor this to prevent this type of issue in the future. We have other event listeners that don't use IMPL_EVENT_HANDLER() to implement the property setter/getter because they need to do something in addition to adding the event listener; I wonder if there are some subtle bugs lurking in there.

Event handler and listener are different. Only adding handler seems to have this special case per spec that it opens midi port.
Adding a listener shouldn't do that.
(MessagePorts have similar odd behavior per spec)

Oh my. That means Chrome doesn't implement the spec correctly because it does it in both cases. Various web pages which use the API rely on it. I feel very silly. Or maybe I missed something else?

Quick update on what I'm doing:

  • I'm removing the hack to convert midir timestamps to TimeStamps directly. Instead I will be using the duration between two midir timestamps plus a TimeStamp taken when the port is opened to generate DOM timestamps for every event. This way the timestamps will be always coherent both among each other and WRT other instances of TimeStamp and the duration between each event will preserve the value and precision of the original samples (which appear to start at 0 and count the number of microseconds elapsed since the port was opened)
  • I'm carefully checking how Chrome deals with event handlers, listeners and the state of ports. Once I'm done with that I'll import their test suite and adapt our implementation to pass them. This is obviously not optimal but it will guarantee that we're compatible with existing code
Attachment #9251125 - Attachment is obsolete: true

I've updated the patch to properly construct TimeStamps without resorting to ugly hacks but I've got bad news on the implementation front: Chrome does open ports implicitly when using MIDIInput.addEventListener("midimessage", ...) rather than just when setting the event handler. This is doubly problematic: first of all because it doesn't match the spec (which clearly states that ports should be opened implicitly only when setting an event handler and not an event listener and secondly because it's not easy to implement. I have to ponder a bit about how to handle this.

You can override EventTarget::EventListenerAdded.

I filed https://github.com/WebAudio/web-midi-api/issues/225 for this. Both need to implicitly open the port for web compat.

FYI I'm looking at how the MIDIAccess/MIDIPort objects are being dealt with by the cycle collector. It appears that the DOM event handlers for midimessage events do not keep the MIDIPort objects alive but IIUC they should. If I have no direct reference to a port in a piece of JS code and the object is only referenced (in theory) by the event handler then the only actual reference is the one from the MIDIAccess object. Once the MIDIAccess object goes away so do the ports, irrespective of the handlers. Given this behavior I suspect there's something amiss with the NS_IMPL_CYCLE_COLLECTING_* macros.

Attachment #9251081 - Attachment is obsolete: true

It seems that the macros used for cycle collection are in order so I really can't figure out why my objects are being collected. Olli, can you give me a hand with the issue that can be reproduced with attachment 9248856 [details]? What I'm seeing is the following:

  • A MIDIAcess object is created, it holds references to a bunch of MIDIInput and MIDIOutput ports
  • I attach an event handler to a MIDIInput port
  • I drop all other references to the MIDIAccess and MIDIInput port objects save for the handler
  • The MIDIInput object my handler is registered on gets collected

Is this expected behavior? Shouldn't the handler be sufficient to keep the MIDIInput object alive?

The reason I'm asking is that there's plenty of pages out there doing just that and the MIDIInput objects don't appear to be collected by Chrome. That being said Chrome seems to do things a little differently: the MIDIInput object in their implementation has a strong reference to the MIDIAccess one, while it doesn't in ours. I don't think that should make a difference in this case because that'd create a cycle between the two and I'm not sure if that'd help given that its external references being dropped that ultimately lead to this issue (if it is an issue at all).

Flags: needinfo?(bugs)

Event handlers or listeners aren't supposed to keep the target alive by default.

You may want to use https://searchfox.org/mozilla-central/rev/016925857e2f81a9425de9e03021dcf4251cafcc/dom/events/DOMEventTargetHelper.cpp#247-254 to explicitly keep the object alive.

Seems like the spec doesn't define exact GC behavior. There are notes, but nothing like
https://html.spec.whatwg.org/#garbage-collection-2 as an example.
Also, the spec talks about EventHandlers keeping the object alive, but what about event listeners (addEventListener)?

Flags: needinfo?(bugs)

(In reply to Olli Pettay [:smaug] from comment #30)

Event handlers or listeners aren't supposed to keep the target alive by default.

Thanks, this is the piece of information I was missing.

You may want to use https://searchfox.org/mozilla-central/rev/016925857e2f81a9425de9e03021dcf4251cafcc/dom/events/DOMEventTargetHelper.cpp#247-254 to explicitly keep the object alive.

That does the trick, now I can reproduce what Chrome does at least WRT midimessage events. I'll check what Chrome does with the other events too to be sure we match what they do. While using KeepAliveIfHasListenersFor() I noticed something which I'm not sure if it's a design choice or an accidental footgun. DOMEventTargetHelper() overrides EventListenerAdded() and EventListenerRemoved() in order to call MaybeUpdateKeepAlive(). If a class (in this case MIDIInput) overrides EventListenerAdded()/EventListenerRemoved() then KeepAliveIfHasListenersFor() won't work unless the overridden methods deliberately call DOMEventTargetHelper::EventListenerAdded() and DOMEventTargetHelper::EventListenerRemoved(). Is that intentional?

Seems like the spec doesn't define exact GC behavior. There are notes, but nothing like
https://html.spec.whatwg.org/#garbage-collection-2 as an example.
Also, the spec talks about EventHandlers keeping the object alive, but what about event listeners (addEventListener)?

I don't know, I'll add that to the list of tests I must run on Chrome to figure out what it's doing.

(In reply to Gabriele Svelto [:gsvelto] from comment #31)

Is that intentional?

Yes. But indeed that should be documented in the .h

When one uses KeepAliveIfHasListenersFor, need to be careful to not cause leaks.
Something needs to call Release eventually
https://searchfox.org/mozilla-central/rev/eea0fac2cb8cae1e5a2e38959eeaa0b7f6d23456/dom/events/DOMEventTargetHelper.cpp#299,306

Thanks! I filed bug 1745003 to add a comment about that.

Depends on: 1745443

Comment on attachment 9251124 [details]
WIP: Bug 1201598 - HACK: Do not clear the access holder to keep the objects alive

Revision D131352 was moved to bug 1745443. Setting attachment 9251124 [details] to obsolete.

Attachment #9251124 - Attachment is obsolete: true
Attachment #9248856 - Attachment is obsolete: true

With the DOM object liveness under review I just need to fix the ALSA configure test and we should be good here.

Attachment #9251122 - Attachment description: WIP: Bug 1201598 - Adjust how port state is handled when adding and opening MIDI ports → Bug 1201598 - Adjust how port state is handled when adding and opening MIDI ports r=padenot
Attachment #9251123 - Attachment description: WIP: Bug 1201598 - Add a midir-based implementation for WebMIDI → Bug 1201598 - Add a midir-based implementation for WebMIDI r=padenot

Leave open because I'll start landing some patches here but not all of them.

Keywords: leave-open
Pushed by gsvelto@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3080df6e8f62
Adjust how port state is handled when adding and opening MIDI ports r=padenot
Pushed by gsvelto@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c5e98c30bbf2
Add a midir-based implementation for WebMIDI r=padenot
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Keywords: leave-open
Resolution: --- → FIXED
Depends on: 1747637
Regressions: 1747637
Severity: normal → --
Target Milestone: --- → 97 Branch
No longer depends on: 1747637
You need to log in before you can comment on or make changes to this bug.