Skip to content

fix: prevent AudioContext and MediaStream memory leaks in Sampler widget#4950

Merged
walterbender merged 1 commit intosugarlabs:masterfrom
WillyEverGreen:fix-sampler-pitch-detection-memory-leak
Jan 3, 2026
Merged

fix: prevent AudioContext and MediaStream memory leaks in Sampler widget#4950
walterbender merged 1 commit intosugarlabs:masterfrom
WillyEverGreen:fix-sampler-pitch-detection-memory-leak

Conversation

@WillyEverGreen
Copy link
Contributor

Summary

This PR fixes a resource leak in the Sampler widget’s pitch detection feature.
When users start pitch detection and then close the widget, the microphone, AudioContext, and animation loop were not properly cleaned up.


Problem

The startPitchDetection function in js/widgets/sampler.js had multiple resource-management issues:

1. Microphone Never Stopped (Privacy Concern)

Calling navigator.mediaDevices.getUserMedia({ audio: true }) activates the microphone, but the associated MediaStream tracks were never stopped when the widget closed. This resulted in:

  • The browser microphone indicator remaining active
  • Audio capture continuing after the widget was closed
  • Unnecessary battery drain on laptops and mobile devices

2. AudioContext Never Closed

Each pitch detection session created a new AudioContext that was never closed. This caused:

  • Accumulation of active AudioContexts
  • Hitting browser AudioContext limits after repeated use
  • Increased memory usage and eventual audio failures

3. Animation Loop Ran Indefinitely

The requestAnimationFrame(updatePitch) loop had no exit condition:

  • The loop continued running even after the widget was closed
  • CPU cycles were wasted on pitch calculations for an inactive widget
  • Risk of errors when attempting to update removed DOM elements

Solution

This PR adds explicit lifecycle management for pitch detection by tracking and cleaning up all related resources.

Added Resource Tracking

this.pitchDetectionAudioContext = null;
this.pitchDetectionStream = null;
this.pitchDetectionAnimationId = null;
this.isPitchDetectionRunning = false;

@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

@walterbender walterbender merged commit 2d9ede1 into sugarlabs:master Jan 3, 2026
5 checks passed
vyagh added a commit to vyagh/musicblocks that referenced this pull request Jan 9, 2026
Replace addEventListener with onclick assignment for stop button handler
to prevent duplicate event listeners from accumulating each time the play
button is clicked.

The previous implementation added a new click listener to the stop button
every time the play button was clicked, causing:
- Memory leak from accumulating listeners
- Multiple handlers firing on single click
- Performance degradation over time

This fix follows the pattern used elsewhere in toolbar.js (e.g., renderStopIcon)
and matches recent memory leak fixes in widgets (sugarlabs#5030, sugarlabs#5029, sugarlabs#4950, sugarlabs#4913).

Changed Files:
- js/toolbar.js: Replace addEventListener with onclick
- js/__tests__/toolbar.test.js: Update test to match new implementation
vyagh added a commit to vyagh/musicblocks that referenced this pull request Jan 9, 2026
Replace addEventListener with onclick assignment for stop button handler
to prevent duplicate event listeners from accumulating each time the play
button is clicked.

The previous implementation added a new click listener to the stop button
every time the play button was clicked, causing:
- Memory leak from accumulating listeners
- Multiple handlers firing on single click
- Performance degradation over time

This fix follows the pattern used elsewhere in toolbar.js (e.g., renderStopIcon)
and matches recent memory leak fixes in widgets (sugarlabs#5030, sugarlabs#5029, sugarlabs#4950, sugarlabs#4913).

Changed Files:
- js/toolbar.js: Replace addEventListener with onclick
- js/__tests__/toolbar.test.js: Update test to match new implementation
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.

2 participants