Skip to content

Fix memory leaks in device monitor teardown path#85

Open
pablopda wants to merge 2 commits intonoroadsleft000:mainfrom
pablopda:fix/null-device-memory-leak
Open

Fix memory leaks in device monitor teardown path#85
pablopda wants to merge 2 commits intonoroadsleft000:mainfrom
pablopda:fix/null-device-memory-leak

Conversation

@pablopda
Copy link
Copy Markdown

Problem

Repeated enable/disable cycles of the extension cause gnome-shell's RSS to grow continuously. This is especially noticeable in environments with frequent network interface churn (Docker, VPN reconnects, USB tethering).

Three separate leaks were identified:

1. Dangling per-device state-changed signals

DeviceMonitor.deinit() disconnected the NM client-level signals but never touched the per-device state-changed handlers. After disable, those handlers still held references to the monitor instance and kept scheduling GLib timeouts against a half-torn-down object — leaking both the signal connections and the timeout sources.

2. Broken teardown chain — DeviceMonitor.deinit() was never called

DevicePresenter had no deinit() method, so AppController.deinit() could never propagate cleanup downward. The entire monitor stayed alive across disable cycles with all its NM signals still connected.

On the DevicePresenter side, resetDeviceStats was subscribed to the device-reset broadcaster via .bind(this), which creates a new function reference on every call. Since EventBroadcaster.unsubscribe() relies on identity comparison (indexOf), there was no way to unsubscribe that handler — it leaked on every cycle.

3. Stale device map entries

_loadDevices() adds entries to this._devices by key, but never cleared the map before rebuilding it. Interfaces that disappeared between reloads (e.g. Docker veth pairs being torn down) persisted as stale NM.Device GObject references indefinitely.

What changed

DeviceMonitor.ts

  • deinit() now cancels the debounce timer, disconnects all per-device state-changed signals, and clears the _devices map — in addition to the existing NM client signal cleanup.
  • _loadDevices() clears _devices before rebuilding so removed interfaces don't persist. Also skips interfaces not managed by NetworkManager (get_device_by_iface returning null), which prevents crashes on orphaned veth pairs.
  • init() connects 5 NM signals instead of 9. Removed any-device-added/any-device-removed (supersets of device-added/device-removed that caused double reloads) and connection-added/connection-removed (fire on profile config edits, not connectivity changes).
  • _scheduleReload() debounces rapid NM signal bursts into a single deferred _loadDevices() call (500ms), preventing redundant /proc/net/dev reads during container start/stop storms.

DevicePresenter.ts

  • Added deinit() that unsubscribes from the device-reset broadcaster and calls _deviceMonitor.deinit(), completing the teardown chain.
  • Converted resetDeviceStats from a regular method to an arrow property, giving it a stable this-bound reference that works with both subscribe() and unsubscribe(). This is consistent with how AppController already handles onRightClick and onSettingChanged.

AppController.ts

  • Calls _devicePresenter.deinit() in the teardown path, placed between saveStats() (needs live data) and _appSettingsModel.deinit().

Resulting teardown chain

GnsExtension.disable()
  → App.stop()
    → AppController.hide()
    → AppController.deinit()
        → unsubscribe broadcasters/settings
        → _devicePresenter.saveStats()
        → _devicePresenter.deinit()
            → unsubscribe from device-reset broadcaster
            → _deviceMonitor.deinit()
                → cancel debounce timer
                → disconnect per-device state-changed signals
                → disconnect 5 NM client signals
                → clear _devices map
        → _appSettingsModel.deinit()
        → uninstallTimers()

Test plan

  • npx tsc --skipLibCheck compiles without errors
  • Enable/disable the extension 20+ times — gnome-shell RSS should stabilize, not grow
  • Start/stop Docker containers while the extension is active — no crashes, no stale veth entries in the device list
  • Verify device list updates correctly on network interface add/remove (WiFi toggle, USB ethernet plug/unplug)
  • Verify device stats reset from the popup menu still works

On repeated enable/disable cycles, gnome-shell RSS grew steadily.
Three root causes were identified:

1. DeviceMonitor.deinit() never disconnected per-device state-changed
   signals. Those handlers held references to the monitor instance and
   kept scheduling GLib timeouts on a half-torn-down object, leaking
   both the signal connections and the timeout sources.

2. DeviceMonitor.deinit() was never called at all. DevicePresenter had
   no deinit() method, so the entire teardown chain stopped at
   AppController — the monitor stayed alive with all its NM client
   signals still connected.

3. The _devices map was never cleared, accumulating stale GObject
   references to NM.Device instances for interfaces that had been
   removed (e.g. Docker veth pairs being torn down).

Additionally, DeviceMonitor.init() subscribed to 9 NM client signals
when only 5 are needed. any-device-added/removed are supersets of
device-added/removed and caused double reloads. connection-added/removed
fire on profile config edits, not connectivity changes, adding noise
without value.

Changes:

- DeviceMonitor.deinit(): disconnect per-device state-changed signals,
  cancel any pending debounce timer, and clear the _devices map before
  disconnecting NM client signals.

- DeviceMonitor._loadDevices(): clear _devices map before rebuilding it
  so removed interfaces do not persist as stale entries. Skip interfaces
  not managed by NetworkManager (null from get_device_by_iface).

- DeviceMonitor.init(): remove any-device-added, any-device-removed,
  connection-added, and connection-removed signals. Keep device-added,
  device-removed, active-connection-added, active-connection-removed,
  and notify::metered (5 total).

- DeviceMonitor: add _scheduleReload() to debounce rapid NM signal
  bursts (e.g. Docker container start/stop) into a single 500ms
  deferred _loadDevices() call.

- DevicePresenter: add deinit() that unsubscribes from the device-reset
  broadcaster and calls _deviceMonitor.deinit(). Convert
  resetDeviceStats to an arrow property so the same reference works for
  both subscribe and unsubscribe (consistent with AppController's
  handler pattern).

- AppController.deinit(): call _devicePresenter.deinit() between
  saveStats() and _appSettingsModel.deinit() to wire the full teardown
  chain.
…onitor

Wrap device enumeration in try/catch to handle transient NM errors from
short-lived interfaces (Docker veth pairs, etc.) that can disappear
between /proc/net/dev read and NM query. Also guard get_ip4_config /
get_ip6_config calls which can throw on devices in transitional states.

Add skipLibCheck to tsconfig.json to avoid build failures from
conflicting ambient @types declarations.
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