Cannot read properties of null (reading 'id')

[stephanedupont]

Description I'm using webmidi.js on a project that is deployed both as a web app and an Electron app. I'm using Sentry for crash reports, and I've got a lot of errors Cannot read properties of null (reading 'id') from webmidi.js

Environment:

• Library version and flavour:

webmidi@^3.1.4:
  version "3.1.5"
  resolved "https://registry.yarnpkg.com/webmidi/-/webmidi-3.1.5.tgz#1dfea41fde25cb02d77cc98f0bac06047cd4baac"
  integrity sha512-l+keAdhhWbW4ygJPgThcidDEMRW3l39Ppz389hbcxijiWwyo1k62GgO0JGb5V7we59qYLTVCCjC9fwYWob1wXA==
  dependencies:
    djipevents "^2.0.7"
  optionalDependencies:
    jzz "^1.5.6"

• Runtime: Chrome 112, Chrome 113, Edge 113, Electron 22.3.2, Electron 22.3.9 (and probably many more)
• Language: TypeScript
• Operating system: Windows

Details Here's the stack trace from Sentry:

[djipco]

I am not too familiar with Sentry but it seems that MIDIInput objects are not available in this particular setup. In a "normal" environment, this problem does not occur.

Could you provide a simple code excerpt that generates the error and that I could run on my end?

[stephanedupont]

I cannot as I cannot reproduce it myself. I just have plenty of these errors as crash reports, but didn't find a way to reproduce them.

The full stack trace shown above shows that it's called in onInterfaceStateChange, so I cannot really wrap this around a try/catch or do anything about it. It seems that it's something internal to the library.

Here's my full (and very simple) use of webmidi if it can help:

import {WebMidi} from "webmidi";
import {CONFIG_LOG_D, CONFIG_LOG_V, CONFIG_LOG_W} from "../config/Config";
import {Log} from "./Log";
import {Alteration, NoteName} from "./MusicTheory/Note";
import {globalManager} from "./GlobalManager";
import {getSettingsStore} from "../stores/settings/SettingsStore";
import {App} from "../types/App";

const LOG_TAG = "MIDIHelper: ";
const MINIMUM_DELTA_BETWEEN_TWO_MIDI_MESSAGES = 30 * 1000000; // 30ms

export interface MIDIEventListener {
  onNoteOn?(note: NoteName, alteration: Alteration, octave: number, velocity: number, timestamp: number): void;
  onNoteOff?(note: NoteName, alteration: Alteration, octave: number, timestamp: number): void;
}

export class MIDIHelper {

  private static isEnabling: boolean = false;
  private static listener: MIDIEventListener | null = null;

  private static lastNoteOnMessageTimestamps: { [key: number]: number } = {};
  private static lastNoteOffMessageTimestamps: { [key: number]: number } = {};

  static init(): void {
    if (MIDIHelper.isEnabling || WebMidi.enabled) return;
    if (CONFIG_LOG_D) Log.D(LOG_TAG + "Enabling WebMidi");
    WebMidi.enable()
      .then(MIDIHelper.onEnabled)
      .catch(e => MIDIHelper.onEnableError(e));
  }

  static disable(): void {
    if (CONFIG_LOG_D) Log.D(LOG_TAG + "Disabling WebMidi");
    WebMidi.disable()
      .then(MIDIHelper.onDisabled)
      .catch(e => MIDIHelper.onDisableError(e));
  }

  static setListener(listener: MIDIEventListener | null): void {
    MIDIHelper.listener = listener;
  }

  static getListener(): MIDIEventListener | null {
    return MIDIHelper.listener;
  }

  private static onEnabled(): void {
    MIDIHelper.isEnabling = false;

    if (CONFIG_LOG_D) Log.D(
      LOG_TAG + "WebMidi enabled with success, starting to listening on available inputs...");

    WebMidi.inputs.forEach(input => {
      if (CONFIG_LOG_D) Log.D(LOG_TAG + `Starting to listen on input ${input.manufacturer} ${input.name}`);
      input.addListener("noteon", e => {
        MIDIHelper.onNoteOnMessageReceived(e.note.number, e.note.attack, Math.round(e.timestamp * 1000000));
      })
      input.addListener("noteoff", e => {
        MIDIHelper.onNoteOffMessageReceived(e.note.number, Math.round(e.timestamp * 1000000));
      })
    });
  }

  private static onEnableError(e: any) {
    MIDIHelper.isEnabling = false;

    if (CONFIG_LOG_W) Log.W(
      LOG_TAG + "Error enabling WebMidi", e);

    setTimeout(() => {
      globalManager.displaySnack("Web MIDI API doesn't seem to be supported on this browser.")
    }, 2000);
  }

  private static onDisabled(): void {
    if (CONFIG_LOG_D) Log.D(
      LOG_TAG + "WebMidi disabled with success.");
  }

  private static onDisableError(e: any) {
    // Beside logging the error, we don't do anything, that's not very important...
    if (CONFIG_LOG_W) Log.W(
      LOG_TAG + "Problem disabling WebMidi", e);
  }

  private static onNoteOnMessageReceived(pitch: number, velocity: number, timestamp: number): void {
    // Whatever, code here is not important and is not using webmidi
  }

  private static onNoteOffMessageReceived(pitch: number, timestamp: number): void {
    // Whatever, code here is not important and is not using webmidi
  }
}

[djipco]

As you can surely understand, without a means to reproduce the problem, it becomes very hard for me to troubleshoot the problem.

If you run the small TypeScript example provided in this repo, do you get any errors?

[stephanedupont]

If I understand correctly, when we call WebMidi.disable(), _destroyInputsAndOutputs() is called and makes an array or promises that will call input.destroy() and output.destroy() on all inputs and outputs, then set the this._inputs and this._outputs variable to empty arrays.

... but as they are promises, it won't be executed instantly, so the following can happen (and is probably what I see in my error reports):

Let's imagine we have several inputs (A,B,C,...)

• Input A is destroyed
• There are still inputs/outputs to destroy so it's possible that before all promises are executed, input A is changing state and _onInterfaceStateChange is called.
• This could lead to the stack trace above, where destroy() has been called on Input A, and therefore, the _midiInput field of input A is null (has been set to null in destroy())

... so shouldn't the getInputById add a null check like this?

getInputById(id, options = {disconnected: false}) {

    if (this.validation) {
      if (!this.enabled) throw new Error("WebMidi is not enabled.");
      if (!id) return;
    }

    if (options.disconnected) {
      for (let i = 0; i < this._disconnectedInputs.length; i++) {
        if (this._disconnectedInputs[i]._midiInput && this._disconnectedInputs[i].id === id.toString()) return this._disconnectedInputs[i];
      }
    } else {
      for (let i = 0; i < this.inputs.length; i++) {
        if (this.inputs[i]._midiInput && this.inputs[i].id === id.toString()) return this.inputs[i];
      }
    }

  };

[stephanedupont]

... or maybe something like this:

async disable() {
  this._isDisabling = true;
  ...
}

_onInterfaceStateChange(e) {
  if (this.isDisabling) return;
  ...
}

[djipco]

Stéphane,

Thank you so much for this analysis. This is really helpful. I made a minor adjustment to the codebase that will probably fix the issue. The fix passes all unit tests but, before I release it, could you try it out in your context to see if it indeed fixes the problem?

You can just replace the /dist/cjs folder in your project with the one attached here: cjs.zip

[stephanedupont]

Thank you so much (for this and for the really useful library!).

Unfortunately, as I cannot reproduce it directly, I can just wait to see if I have any new crash reports.

I would also have to deploy a new build of the app, so the changes won't necessarily be pushed before at least a few days, probably more a couple of weeks.

But I'm very confident that your fix should works, your code seem to be fixing this edge case. So don't wait on me :)

Again, thank you very much!

[djipco]

Okay, no worries. I just published release v3.1.6 on NPM. Hopefully it will fix your issue. Feel free to chime in if it didn't. Thx.

[stephanedupont]

Unfortunately, this still happens with version 3.1.6.

Here's a picture that shows the full stack trace:

[djipco]

Without means to reproduce the problem, it is very hard for me to work on a solution. I'm going to close this for now but feel free to reopen it if you have more info.

[stephanedupont]

@dijipco I think I gave a possible fix above. Introduce a null check (which I explained why it might be needed) will not hurt the code and doesn't mean that you need to be able to reproduce it to fix it.

I don't have the permission to reopen it, or else I would have.

[djipco]

You are right, sorry for the confusion. I implemented your suggestion in release 3.1.14 (for both getInputById() and getOutputById(). Hopefully this fixes the problem.

If you could report back here to let me know if it does indeed fix the problem, that would be appreciated. Thanks (and sorry for the long delay).

[stephanedupont]

Thank you, will do!