Issue #2790 - Potential null-deref crash in EntityEditMaster after-load listener (unguarded e.detail.shadowRoot)#2791
Merged
Merged
Conversation
oleh-maikovych
changed the title
e.detail.shadowRoot)
…moving the after-load shadowRoot race. EntityEditMaster set closeAfterExecution = false on the embedded master's SAVE action from its after-load listener, dereferencing e.detail.shadowRoot. tg-element-loader fires after-load synchronously right after appendChild, whereas Polymer attaches shadowRoot in ready(), which runs from connectedCallback. When the loader is detached -- the reload path taken during heterogenic re-navigation, and after tg-custom-action-dialog._removeFromDom offloads a cached master -- the embedded master is appended without being connected, ready() has not run, shadowRoot is null, and querySelector threw an uncaught TypeError. Merely guarding the dereference would only silence the crash. closeAfterExecution would never be assigned, nothing would re-attempt it once the master connected, and the SAVE action would keep its rendered close-after-execution=true, publishing canClose=true and closing the dialog -- precisely what EntityEditMaster exists to prevent. The assignment now happens where it cannot be early. EntityManipulationMaster exposes shouldCloseAfterSave(), whose value reaches the embedded master as the shouldCloseAfterSave property via _calcAttrs; tg-element-loader.insertElement assigns attributes before insert(), hence before connectedCallback and ready(). tg-entity-master-behavior honours the property in ready(), alongside the existing compound-master check, where $._saveAction is guaranteed to exist. EntityEditMaster overrides shouldCloseAfterSave() to false; its _seqEditAfterLoadListener thereby became equivalent to the inherited default and has been removed along with the getAfterLoadListener() override. Nested compound-master sections previously received the same assignment incidentally, because after-load bubbles. They remain covered by the tg-master-menu-item-section check in ready(), which is race-free by construction and was subject to the same null-shadowRoot exposure. tg-entity-master-behavior.js is part of the startup bundle, so resources must be re-vulcanised for the change to reach the client.
…tityManipulationMaster. The handler announcing an imminent change of the embedded master's type was defined identically by EntityEditMaster and by application masters that had copied it, each registering its own '_curr-binding-entity-changed' listener. It is meaningful for every manipulation master, as entityType is a property of AbstractEntityManipulationAction, and its sole consumer is tg-custom-action-dialog._handleMasterBeforeChange. AbstractMasterWithMaster gains getAdditionalReadyCallbackCode(), a hook appended to the master's ready callback and defaulting to no code, mirroring the adjacent getAdditionalImports(). EntityManipulationMaster overrides it to define _handleBindingEntityChanged and register the listener, leaving EntityEditMaster to contribute only its own _handleBindingEntityRetrievedError. MasterWithMaster is unaffected. Applications still carrying a copy of the handler continue to work without duplicate events. The platform registers its listener from //@ready-callback, which precedes //@master-is-ready-custom-code, so it updates _prevCurrBindingEntity before a stale copy runs, and that copy then observes no type change. EntityNewAction masters, which use EntityManipulationMaster directly, now carry the handler as well. They are bound to a single entity type, so the event never fires for them.
…unning it.
'tg-action' admits a user-initiated action only while it is enabled, then defers 'run' by 100 ms through '_asyncRun' and '_runOptionAction'.
Neither callback re-checked that state, and 'run' does not check it either.
A tap or a keyboard shortcut could therefore start an action that had become disabled during the delay.
This surfaced while testing sequential creation, where an application master drives 'retrieve' and 'save' on a wrapping master in response to 'save.post.success'.
Both functions disable the master for the duration of their request, and retrieval completion re-enables it.
Pressing Cmd|Ctrl+S in one of those windows admitted the SAVE action of the embedded master, which then ran after the loop had already begun a request.
The wrapping and the embedded masters hold separate 'invoke-saving' debouncers, so the two requests were not merged into one.
Two overlapping transactions resulted, and SQL Server chose one of them as a deadlock victim.
This defect is independent of the after-load race addressed earlier on this branch, which merely made it reachable by letting the sequential loop run to completion.
Both deferred callbacks now re-assert 'action-disabled' immediately before running.
That attribute is maintained by '_buttonStateChanged', which resolves '_optionButtonDisabled' or '_disabled' according to the button variant.
It also drives ':host([action-disabled]) { pointer-events: none }', which is what 'tg-shortcut-processing-behavior._isEnabled' tests when admitting a shortcut.
Admission and execution therefore consult the same source of truth.
Programmatic invocations of '_asyncRun', which pass neither an event nor a shortcut, are exempt.
Developer-driven code may legitimately run a disabled action, most notably to save an unmodified entity, as '_createSavingPromise' explains.
'tg-egi-master-behavior' and 'tg-test-utils' rely on this, as do several web tests.
'_runOptionAction' needs no such exemption, as an option can only be activated by the user.
'_asyncRunAfterContinuation' remains unguarded by design, as it calls 'run' directly to resume a save while the action is disabled for the duration of a continuation.
A side effect is that two rapid presses now produce a single 'run'.
Previously both ran, and only the AJAX requests were merged by the debouncer, which left the saving promise of the first one unresolved.
'tg-action.js' is part of the startup bundle, so resources must be re-vulcanised for the change to reach the client.
jhou-pro
approved these changes
Jul 10, 2026
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.
Resolve #2790
To be completed by the pull request creator
This section should be completed with reference to section Preparing PR of the Code and PR reviews wiki page.
Create the pull request as a draft by tapping the dropdown arrow on the 'Create pull request' button under the pull request description (below the text box where this description is being edited) and changing the default
Create pull requesttoDraft pull request.Or, if the pull request has already been created, convert it to draft by tapping the "Convert to draft" link beneath the "Reviewers" section.
A self-review of all changes has been completed, and the changes are in sync with the issue requirements.
Changes to the requirements have been reflected in the issue description.
Developer documentation (e.g., comments, Javadoc), have been provided where required.
All existing and new Java tests pass successfully by running them with Maven.
All existing and new Web tests pass successfully.
Changes have been inspected for possible NPE situations, and the changes are sufficiently defensive.
The correct base branch has been selected for these changes to be merged into.
The latest changes from the base branch have already been merged into this feature branch (and tested).
Changes subject to performance considerations have been evaluated, and tested against production-size data if applicable.
This pull request does not contain significant changes, and at least one appropriate reviewer has been selected.
The
In progresslabel has been removed from the issue.The
Pull requestlabel has been added to the issue.The pull request has been made ready for review by tapping the "Ready for review" button below the list of commits on the pull request page.
To be completed by the pull request reviewer
This section should be completed with reference to section Performing PR review of the Code and PR reviews wiki page.
The
In progresslabel has been added to the pull request in GitHub.The issue requirements have been read and understood (along with any relevant emails and/or Slack messages).
The correct base branch is specified, and that base branch is up-to-date in the local source.
The issue branch has been checked out locally, and had the base branch merged into it.
All automated tests pass successfully.
Ensure the implementation satisfies the functional requirements.
Ensure that code changes are secure and align with the established coding practices, including code formatting and naming conventions.
Ensure that code changes are documented and covered with automated tests as applicable.
Ensure that code changes are well-suited for informal reasoning.
Ensure that changes are documented for the end-user (a software engineer in the case of TG, or an application user in the case of TG-based applications).
If there are significant changes (described above), special attention has been paid to them.
Marked the task items in section "Significant changes" as completed to indicate that corresponding changes have been reviewed, improved if necessary, and approved.
The issue or issues addressed by the pull request are associated with the relevant release milestone.
To be completed by the pull request reviewer once the changes have been reviewed and accepted
The changes have been merged into the base branch (unless there is a specific request not to do so, e.g., they are to be released to SIT).
The issue branch has been deleted (unless the changes have not been merged - see above, or there is a specific request not to do so).
The
In progresslabel has been removed from the pull request.The
Pull requestlabel has been removed from the issue.