Fix hook removal semantics and related hook lifecycle bugs - #1016
Merged
Conversation
The remove_hook function had inverted conditional logic on line 33.
BEFORE:
if (id) {
hook[key] = {}; // Clears ALL hooks when id is provided
} else {
delete hook[key][id]; // Never reached; would cause error
}
AFTER:
if (!id) {
hook[key] = {}; // Clears ALL hooks only when no id provided
} else {
delete hook[key][id]; // Removes the specific hook by id
}
This bug prevented individual hooks from being removed. Callers like Tour.js
that attempted to remove specific hooks would instead clear all hooks for that
event, breaking subsequent event handlers.
Fixes: Hooks cannot be individually removed, causing memory leaks and
unexpected behavior when modules try to clean up their event listeners.
Add guard clause to prevent TypeError when attempting to remove a hook from a key that does not exist in the hooks object. This handles edge cases where callers may accidentally attempt to remove hooks from nonexistent keys.
When removing interaction callbacks, strip the _custom suffix from the stored key to get the real event name that was used when registering the hook. This prevents orphaned hooks from persisting across multiple tours, which was causing state machine corruption and test hangs. Lines changed: 314-319 only (remove_canvas_interaction_callbacks method)
Only call add_canvas_interaction_callbacks/remove_canvas_interaction_callbacks on actual state transitions, not every time the state setter is called. This prevents registering duplicate hooks when state is set to PLAYING while already PLAYING, which was causing orphaned hooks to accumulate and leak across multiple tours. Lines changed: 104-138 (state setter only) - now checks was_playing before calling add/remove
Both screensaver tests were leaving their Tour instances in PLAYING state, leaking interaction hooks into the module-level singleton hook registry. This was invisible before due to the old buggy remove_hook behavior which incidentally swept up orphaned hooks, but now surfaces as cross-test contamination. Added t.tour.clear() call in both tests' .then() block before test.end() to explicitly return the tour to INACTIVE state, triggering proper hook cleanup via remove_canvas_interaction_callbacks().
Collaborator
|
Good catch! This all looks good to me. |
Member
|
Great. Good catch, and thanks for the fixes. I think we can merge and see how it goes on beta.onezoom.org |
Member
|
P.s. looks like the tests are being helpful here. Do we need to add any more? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This started as a one-line bug fix.
remove_hook()had its logic inverted: asking it to remove one specific hook instead wiped out every hook registered for that event.Fixing that bug exposed several latent issues that had been masked by the previous behavior.
remove_hook()removed all hooks instead of the requested hook.Tour.jsattempted to remove hooks using a different key than the one used to register them (*_customsuffix mismatch).Tour.jsre-registered interaction callbacks when repeatedly assigned thePLAYINGstate, creating duplicate registrations.test_tour_Screensaver.jsnever cleaned up itsTourinstances, leaking hooks into subsequent tests because the hook registry is shared across the test process.Each issue was fixed as it was uncovered during testing.
Temporary instrumentation was used to verify the hook lifecycle while diagnosing the interaction between these issues. That instrumentation has been removed.
Testing:
remove_hook().Result: 241/241 tests passing on consecutive full-suite runs.
Although this PR grew beyond the original one-line fix, each additional change was required to restore correct hook lifecycle behavior and keep the test suite passing once remove_hook() behaved as intended.