Skip to content

Scenario: guard TimeSync execution component against use after cleanup - #2113

Open
bltzr wants to merge 2 commits into
ossia:masterfrom
bltzr:fix/scenario-timesync-null-guard
Open

Scenario: guard TimeSync execution component against use after cleanup#2113
bltzr wants to merge 2 commits into
ossia:masterfrom
bltzr:fix/scenario-timesync-null-guard

Conversation

@bltzr

@bltzr bltzr commented Jul 13, 2026

Copy link
Copy Markdown
Member

TimeSyncComponent::cleanup() resets m_ossia_node while the component's destruction is deferred through the execution queue. If the model is edited in that window — toggling autotrigger, changing the trigger expression, changing quantization, or a GUI trigger — the UI-thread handlers still fire and either dereference a null node inside the in_exec lambda or trip SCORE_ASSERT(m_ossia_node) in updateTrigger().

Guard each of those entry points with an early return when m_ossia_node is null.

🤖 Generated with Claude Code

https://claude.ai/code/session_019ya5qsdZbjVtA697QUhJFz

cleanup() resets m_ossia_node while the component's destruction is deferred
through the execution queue. Model edits arriving in that window (autotrigger
toggles, trigger/expression changes, quantization changes, GUI trigger) would
then dereference a null node in the exec lambda, or trip SCORE_ASSERT in
updateTrigger(). Bail out early when m_ossia_node is null.
@bltzr
bltzr requested a review from jcelerier July 13, 2026 22:40
@bltzr

bltzr commented Jul 13, 2026

Copy link
Copy Markdown
Member Author

I am a little unsure about this one. Claude made it following a crash I had but I'm not sure...

start = m_score_node->isStartPoint();
}

SCORE_ASSERT(m_ossia_node);

@jcelerier jcelerier Jul 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this assert removed ? It's a safeguard: if it triggers it means that something else before it is wrong and has to be fixed

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can lower the "expectations" but it leads to worsening of the understandability of the codebase since it creates more potential code paths

…ding

Review rework: restore SCORE_ASSERT(m_ossia_node) and drop the null
guards. Root cause of the post-cleanup calls: cleanup() resets
m_ossia_node immediately, but the component's destruction is deferred
through the exec queue and then the gc queue while the model stays alive
and editable — so a trigger/autotrigger/quantization edit after stop
still fired the ctor connections into the half-dead component.
Disconnecting the model from the component in cleanup() severs every
such path (all guarded methods are reachable only through those
connections), keeping the invariant instead of widening the code paths.

Co-Authored-By: Claude <noreply@anthropic.com>
@bltzr

bltzr commented Jul 26, 2026

Copy link
Copy Markdown
Member Author

Fair — reworked (99835c5): the assert is restored and the guards are gone. Root cause found: cleanup() resets m_ossia_node immediately, but the component's destruction is deferred through the exec queue and then the gc queue while the model stays alive and editable — so after play→stop, a trigger/autotrigger/quantization edit still fired the five ctor con(element, …) connections into the half-dead component (assert in updateTrigger, or a null time_sync captured into an exec lambda). The rework severs the model→component connections in cleanup(); every guarded method was reachable only through them, so the invariant holds again with no extra code paths.

Found while auditing, three things you may want to weigh in on:

  1. EventComponent has the same latent bug with no assert to catch it — its conditionChanged lambda captures m_ossia_event at fire time and would null-deref on the audio thread after cleanup. Same one-line disconnect applies; this PR or a follow-up?
  2. IntervalComponent::cleanup ends with a bare disconnect() — that only severs the component's outgoing signals, not the incoming con(interval(), …, this, …) ones; it may only be safe today because those handlers capture shared_ptrs.
  3. After a normal stop, ExecutionTick.cpp:121 enqueues gc(std::move(context)) into the context's own gc queue, which is never drained once m_ctxData is replaced — the old exec queue (and every self it holds) looks like it can outlive the stop indefinitely, which is what makes this window user-reachable at all. Intended?

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage is 15.314%bltzr:fix/scenario-timesync-null-guard into ossia:master. No base build found for ossia:master.

@jcelerier

Copy link
Copy Markdown
Member

@claude trace the whole dataflow and state machine after timesynccomponent, eventcomponent, statecomponent, intervalcomponent, scenariocomponent, etc. end in "cleanup": can they ever be recreated? If not, extend the patch with the same fixes to the other types

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.

3 participants