Skip to content

remove temporary plugin loader scripts from document.head after execution#7389

Open
sahu-virendra-1908 wants to merge 2 commits into
sugarlabs:masterfrom
sahu-virendra-1908:fix-plugin-script-cleanup
Open

remove temporary plugin loader scripts from document.head after execution#7389
sahu-virendra-1908 wants to merge 2 commits into
sugarlabs:masterfrom
sahu-virendra-1908:fix-plugin-script-cleanup

Conversation

@sahu-virendra-1908
Copy link
Copy Markdown
Contributor

@sahu-virendra-1908 sahu-virendra-1908 commented May 17, 2026

What this fixes

While testing plugin loading in Music Blocks, I noticed that dynamically injected plugin <script> elements remain permanently attached to document.head after plugin execution completes.

The issue happens inside processPluginData() in:

js/utils/utils.js

Current implementation:

const script = document.createElement("script");
script.src = url;

document.head.appendChild(script);

The script executes correctly, but the injected DOM node is never cleaned up afterward.

Because plugin loading can happen multiple times in the same session, repeated plugin reloads gradually accumulate unused <script> tags inside <head>.

Related Issue #7388

Root cause

The dynamically created Blob-based loader scripts are appended to the DOM but never removed after:

  • successful execution
  • failed loading
  • rejected initialization

Current behavior:

script.onload = () => {
    URL.revokeObjectURL(url);
    resolve();
};

script.onerror = e => {
    URL.revokeObjectURL(url);
    reject(e);
};

The Blob URL is revoked correctly, but the actual <script> element remains in the document permanently.

This affects:

  • main plugin scripts
  • additional setup/eval scripts created during plugin initialization

Over long sessions, document.head continuously grows with unused script nodes.

What I changed

Added cleanup for dynamically injected plugin scripts after execution completes.

Updated both:

  • script.onload
  • script.onerror

to remove the temporary script node from document.head.

script.onload = () => {
    URL.revokeObjectURL(url);
    document.head.removeChild(script);
    resolve();
};

script.onerror = e => {
    URL.revokeObjectURL(url);
    document.head.removeChild(script);
    reject(e);
};

Result

  • Temporary plugin loader scripts are cleaned up correctly
  • document.head no longer accumulates unused plugin script tags
  • Long-running sessions avoid unnecessary DOM growth
  • Existing plugin functionality remains unchanged
  • Blob URLs are still revoked properly after execution

PR Category

  • Bug Fix
  • Feature
  • Performance
  • Tests
  • Documentation

Checklist

  • I have tested these changes locally and they work as expected.
  • I have followed the project's coding style guidelines.
  • I have run lint/formatting checks where applicable.
  • I have kept the change minimal and focused on the reported issue.
  • I have enabled "Allow edits by maintainers" for this PR.

@github-actions github-actions Bot added bug fix Fixes a bug or incorrect behavior size/XS Extra small: < 10 lines changed area/javascript Changes to JS source files labels May 17, 2026
@sahu-virendra-1908
Copy link
Copy Markdown
Contributor Author

sahu-virendra-1908 commented May 17, 2026

@sum2it @omsuneri @walterbender kindly review this PR. Happy to make any improvements or changes if required.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 17, 2026

🧪 Jest Test Results

❌ Some Jest tests failed. Please check the logs and fix the issues before merging.

Coverage: Statements: 47% | Branches: 38.51% | Functions: 51.47% | Lines: 47.42%
Master Coverage: Statements: 47.62% | Branches: 38.86% | Functions: 52.45% | Lines: 48.03%

Note: These failures may be introduced by this PR or may already exist in the master branch.
Tip: Update your branch with the latest master and rerun tests.
If the same failures are present on master, they are likely not introduced by this PR.

Failed Tests:

DictActions.test.js

@sahu-virendra-1908
Copy link
Copy Markdown
Contributor Author

sahu-virendra-1908 commented May 17, 2026

@walterbender sir The failing DictActions.test.js appears unrelated to this PR.
Latest upstream commit 464a5bd removed the debug console.log from DictActions.setValue, but the corresponding Jest expectation still checks console.log calls.

@github-actions github-actions Bot added needs-rebase and removed size/XS Extra small: < 10 lines changed labels May 18, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 18, 2026

This PR has merge conflicts with master.

Please rebase your branch:

# Add upstream remote (one-time setup)
git remote add upstream https://github.com/sugarlabs/musicblocks.git

# Fetch latest master and rebase
git fetch upstream
git rebase upstream/master

# Resolve any conflicts, then:
git push --force-with-lease origin YOUR_BRANCH

Tip: Enable "Allow edits from maintainers" on this PR so we can auto-rebase for you next time. This only grants access to your PR branch. Your fork's other branches are not affected.

@github-actions github-actions Bot added size/S Small: 10-49 lines changed area/tests Changes to test files labels May 18, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 18, 2026

🧪 Jest Test Results

✅ All Jest tests passed! This PR is ready to merge.

Coverage: Statements: 47% | Branches: 38.51% | Functions: 51.47% | Lines: 47.42%
Master Coverage: Statements: 47.62% | Branches: 38.86% | Functions: 52.45% | Lines: 48.03%

@sahu-virendra-1908
Copy link
Copy Markdown
Contributor Author

sahu-virendra-1908 commented May 18, 2026 •
edited
Loading

@walterbender sir, I also changed DictActions.test.js to reflect the correct behavior after removing the debug console.log.

The old test was asserting that console.log fires on setValue that was testing the old debug behavior, not the intended production behavior.
The updated test now asserts that console.log is NOT called and also verifies that the value is correctly stored in turtleDicts.

@sahu-virendra-1908
fix: clean up plugin loader scripts after execution
fcdf79e
@sahu-virendra-1908
test: update DictActions setValue test to expect no console.log
28a870a 
The debug console.log was removed from DictActions.setValue() as it
was a leftover from development. The existing test was asserting that
console.log fires on every setValue call, which was the bug behavior.

Updated test to:
1. Assert console.log is NOT called (correct production behavior)
2. Assert the value is actually stored correctly
3. Add mockRestore() for proper spy cleanup

:wq#
@sahu-virendra-1908 sahu-virendra-1908 force-pushed the fix-plugin-script-cleanup branch from 2b44315 to 28a870a Compare May 18, 2026 10:51
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 18, 2026

🧪 Jest Test Results

✅ All Jest tests passed! This PR is ready to merge.

Coverage: Statements: 47.62% | Branches: 38.86% | Functions: 52.45% | Lines: 48.02%
Master Coverage: Statements: 47.62% | Branches: 38.86% | Functions: 52.45% | Lines: 48.03%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

area/javascript Changes to JS source files area/tests Changes to test files bug fix Fixes a bug or incorrect behavior size/S Small: 10-49 lines changed

Projects

@MusicBlocks Issues and PRs
Status: Todo

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@sahu-virendra-1908