Skip to content

Fix BLE disconnect crash and event listener leaks#475

Open
makermelissa-piclaw wants to merge 1 commit intocircuitpython:betafrom
makermelissa-piclaw:fix/ble-disconnect-crash
Open

Fix BLE disconnect crash and event listener leaks#475
makermelissa-piclaw wants to merge 1 commit intocircuitpython:betafrom
makermelissa-piclaw:fix/ble-disconnect-crash

Conversation

@makermelissa-piclaw
Copy link
Copy Markdown
Contributor

Summary

Fixes #377 — BLE disconnect during file write operations throws Modal has not been opened yet. No instance available and, in most cases, prevents the UI from being used again without a page refresh.

Root cause

Two separate bugs in js/workflows/ble.js compound into the reported crash:

Bug 1 — connectionStep() called when no dialog is open

When BLE disconnects mid-file-write (not during the connect flow), the chain is:

gattserverdisconnected event
  → onDisconnected()          (workflow.js)
    → updateConnected(false)   (workflow.js)
      → connectionStep(2)      (ble.js override)
        → _markStepCompleted(2)
          → this.connectDialog.getModal()  ❌ throws

getModal() throws because connectDialog was never opened in this lifecycle — the user was editing a file, not walking the connect wizard. This matches the stack trace in the issue exactly.

Bug 2 — Event listener removal no-ops

switchToDevice() and connectToSerial() attempt to clean up listeners with:

device.removeEventListener('gattserverdisconnected', this.onDisconnected.bind(this));

Every .bind(this) call returns a new function reference, so removeEventListener never matches the previously-added listener. Each reconnect leaks another handler, causing duplicate disconnect callbacks (which makes Bug 1 fire multiple times on a single disconnect).

The same pattern exists for onSerialReceive and the advertisement listener in connectToBluetoothDevice().

Fix

  1. Guard connectionStep() in updateConnected() behind connectDialog.isOpen() so step-marking only runs when the dialog actually exists.
  2. Store bound handlers in the constructor (_boundOnDisconnected, _boundOnSerialReceive) and reuse the same reference for both addEventListener and removeEventListener.
  3. Fix advertisement listener cleanup in connectToBluetoothDevice() the same way (_boundOnAdvertisementReceived), and remove any previous listener before attaching a new one.

Testing

  • npx vite build passes cleanly.

  • Reproduced the crash signature against the unpatched getModal() path and verified the guard short-circuits it while preserving normal behavior when the dialog is open:

    Scenario Unpatched Patched
    Disconnect while connect dialog closed (the bug) Modal has not been opened yet ✅ Skipped safely
    Normal connect with dialog open ✅ Works ✅ Works
  • Hardware end-to-end (file write → disconnect) testing was limited by Linux Chromium watchAdvertisements flakiness; local-only bench review confirms the listener-identity fix, and reviewers on macOS/Windows Chrome can exercise the full flow.

Notes

Only js/workflows/ble.js is modified. No behavior change for connected users or normal flows — only the error path is fixed.

- Guard connectionStep() in updateConnected() behind connectDialog.isOpen()
  to prevent 'Modal has not been opened yet' crash when BLE disconnects
  during file write operations (fixes circuitpython#377)
- Store bound event handlers in constructor and reuse them so
  removeEventListener actually removes the old listener
- Fix advertisement listener cleanup in connectToBluetoothDevice()
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 this pull request may close these issues.

1 participant