Skip to content

[FIX] sap.m.IllustrationPool: gracefully remove DOM pool assets #4309

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dfenerski
Copy link
Contributor

Setup

  1. Assume for whatever reason execution reaches line 464
  2. Execution of _removeAssetFromDOMPool starts
  3. In the function body, there are 2 checks which assert:
    3.1. oDOMPool (a "static" DOM element) exists
    3.2. oAssetDOM (the thing we presumably no longer need and want to remove) exists
  4. If for whatever reason any of these do not exist, the _removeAssetFromDOMPool will silently do nothing (it will leave aSymbolsInDOM intact)
  5. Execution will return to line 461 to an unmodified array, freezing the application in an infinite loop.

Some semantics

In my case, oAssetDOM was being removed dynamically, by some other application code.

Leaving the question whether this element should be removed by some other application code aside, it seems to me the symbol removal should not be tied to the DOM element removal, unless I have missed some other dependency.

In any case, "get your app frozen if you remove the wrong DOM element" feels weird so I decided to check for second opinion by opening this PR.

Copy link

cla-assistant bot commented Jul 14, 2025

CLA assistant check
All committers have signed the CLA.

@NHristov-sap
Copy link
Contributor

Hello @dfenerski ,

Thank you for sharing your enhancement proposal. I've created an internal incident DINC0582971. The code owner will take a look on this PR.

Best Regards,
Nikolay Hristov
GitHub Dispatcher

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