Skip to content

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

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

Scenario: guard TimeSync execution component against use after cleanup#2113
bltzr wants to merge 1 commit 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

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