fix: don't crash when the menu is gone by the time the layer click arrives - #818
Open
bbrala wants to merge 1 commit into
Open
fix: don't crash when the menu is gone by the time the layer click arrives#818bbrala wants to merge 1 commit into
bbrala wants to merge 1 commit into
Conversation
…rives
With `useModal: false` the menu is dismissed by a document-level mousedown
listener rather than by the transparent layer. That listener outlives the
menu it was registered for, so the menu can already be gone when the next
click arrives: a `build` menu empties its own options object once it has
finished hiding, and a `hide` handler calling contextMenu('destroy') tears
it down outright.
handle.layerClick() guarded that case with
root.$menu === null || typeof root.$menu === 'undefined' || ...
and then called root.$menu.trigger('contextmenu:hide') inside the block it
opened for it, so the very case the guard detected was the one that threw
"Cannot read properties of undefined (reading 'trigger')". The throw also
happened before `onhide` could run, so the dismiss listener was never
unregistered and one more piled up for every menu that had been opened.
Skip straight to the cleanup when there is nothing left to hide, covering
both an absent `$menu` and an empty jQuery object. Everything else on the
path is untouched: the same events fire in the same order, `onhide` keeps
its timing, and the `isNearRecentSelectChange()` grace period still takes
precedence exactly as before.
Fixes #805
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
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.
The problem
With
useModal: falsethe menu isn't dismissed by the transparent layer but by a document-levelmousedownlistener thatop.layer()registers, which callshandle.layerClick()and unregisters itself again through theonhidecallback it hands in.That listener outlives the menu it was registered for, so by the time the next click arrives the menu may already be gone.
handle.layerClick()guarded that withand then dereferenced
root.$menuinside the very block it opened for it, so the case the first two clauses detect was the case that threwCannot read properties of undefined (reading 'trigger'). Because the throw happened beforeonhideran, the dismiss listener was never removed either and one more accumulated for every menu that had been opened.Full credit to @Bogatinov for the diagnosis in #805: the report pinned the exact line, worked out that the listener leak is a consequence of the throw skipping the deregistration, and noted that the leaked listeners resolve themselves once the bail-out path invokes
onhide. This PR fixes it in the plugin so the monkey-patch in the issue is no longer needed.Two ways the menu goes missing
hideevent handler calling$(selector).contextMenu('destroy'), which is the configuration in the report.buildmenu, which empties its own options object (including$menu) once it has finished hiding, inop.hide(). This one needs nodestroyat all: open a built menu withuseModal: false, pick an item, then click anywhere.$menucan be left dropped altogether or as an empty jQuery object; the old guard covered neither properly, so both shapes are handled.The fix
Hoist a
menuIsGonecheck and, in the branch that would have hidden the menu, skip straight to the cleanup instead of dereferencing a menu that isn't there.onhidestill runs, so the dismiss listener unregisters itself.Fixing the throw is enough to fix the leak as well: no extra teardown was added anywhere. The tests assert the listener count returns to zero over repeated open/destroy cycles (it grew 1, 2, 3 before the fix).
Backwards compatibility
Nothing changes for existing callers on any path that doesn't currently throw:
$menuis present and non-empty, the code takes exactly the same branch as before, fires the samecontextmenu:hideevent, and callsonhideat the same point.isNearRecentSelectChange()grace period (Bug option-select in submenu in firefox #744) still takes precedence over the new bail-out, so a menu that is being kept open by it is unaffected.document.elementFromPoint()is still called at the same point, for the same reason.handle.layerClick's signature and everything else reachable from$.contextMenu.handleare unchanged, so third-party patches against it keep loading.Verified against jQuery 1.12.4, 2.2.4, 3.7.1 and 4.0.0 (unit and acceptance).
Tests
test/unit/issue-805-layerclick-after-destroy.test.js: the built-menu case, thedestroy-from-hidecase, the empty-jQuery$menushape, and a repeated-cycle test for the listener leak. All four fail onmasterwith the reportedTypeErrorand the growing listener count.test/specs/issue-805-no-modal-destroy.js: the same sequence as real browser clicks, asserting nopageerrorand a listener count that returns to zero.Closes #805