Fix two crashes in editCalendar() (RedBox.onDisplay recursion, null PGList)#66
Conversation
Two latent JS crashes surfaced while investigating horde#65 (empty calendar dialog). Neither is the root cause of horde#65 (that is in horde/core redbox.js), but both throw when triggered. 1. Infinite recursion of RedBox.onDisplay The save/restore pattern for RedBox.onDisplay (duplicated in editEvent, editCalendar, editTask) shares this.redBoxOnDisplay. If a dialog is reopened before the previous one was displayed (so onDisplay never ran its restore), the next call captures its own wrapper into redBoxOnDisplay, which then calls itself -> RangeError: Maximum call stack size exceeded. Tag the wrapper and skip re-capturing it so redBoxOnDisplay always holds the original handler. 2. observe() on a null PGList element $('kronolithC' + type + 'PGList').observe('change', ...) throws when the tasklists PGList <select> is absent, aborting the callback. Guard with an existence check. Refs horde#65
There was a problem hiding this comment.
Pull request overview
This PR hardens the dynamic (AJAX) calendar/task/event dialogs by preventing a RedBox onDisplay wrapper from becoming self-referential (stack overflow) and by avoiding a null-dereference when wiring up permissions-related <select> handlers during editCalendar().
Changes:
- Prevents infinite recursion by tagging Kronolith’s
RedBox.onDisplaywrapper (_kronolithWrap) and avoiding re-capturing the wrapper as the “original” handler. - Guards the
PGListchange-observer registration ineditCalendar()when the corresponding DOM element is absent.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var pgList = $('kronolithC' + type + 'PGList'); | ||
| if (pgList) { | ||
| pgList.observe('change', function() { | ||
| $('kronolithC' + type + 'PG').setValue(1); | ||
| this.permsClickHandler(type, 'G'); |
There was a problem hiding this comment.
Good point, but that's a separate scenario from what this PR targets. This PR fixes the two crashes behind #65 on an instance where sharing is enabled. The broader no_sharing case (unconditional updateGroupDropDown() / perms init in editCalendarCallback()) is a distinct code path we can't test here, so hardening it belongs in its own change to avoid shipping an untested guard. Keeping this PR scoped.
pierrefardel
force-pushed
the
fix/editcalendar-crashes
branch
from
a55726c to
ef2459f
Compare
While investigating #65 (empty calendar dialog), two latent JS crashes in
editCalendar()surfaced. Neither is the root cause of #65 — that is inhorde/core
redbox.js(see horde/Core#185) — but both crash when triggeredand are worth fixing.
1. Infinite recursion of
RedBox.onDisplayThe save/restore pattern for
RedBox.onDisplay(duplicated ineditEvent,editCalendar,editTask) sharesthis.redBoxOnDisplay. If a dialog isopened again before the previous one was displayed (so
onDisplaynever ranits restore), the next call captures its own wrapper into
redBoxOnDisplay, which then calls itself →RangeError: Maximum call stack size exceeded.Fix: tag the wrapper and skip re-capturing it, so
redBoxOnDisplayalwaysholds the original handler, never a wrapper.
2.
.observe()on a null PGList elementThe tasklists
PGList<select>may not be present;$(...)returns null and.observe()throws, aborting the callback. Guarded with an existence check.Testing
Verified on a live H6 dev instance: combined with horde/Core#185, the calendar
create/edit dialog renders correctly and no longer throws.
Refs #65