🔒 [security fix] Fix JSONP memory leak with hybrid cleanup strategy#221
🔒 [security fix] Fix JSONP memory leak with hybrid cleanup strategy#221
Conversation
This commit addresses a memory leak in the JSONP utility function where callback properties were accumulating on the global `window` object. Based on user feedback, a hybrid approach has been implemented: 1. On success: The callback property is completely deleted from the `window` object (with a fallback to `undefined`) to ensure no memory leak occurs for the most common case. 2. On timeout: The callback property is replaced with a `noop` function. This prevents console `TypeError` if a late-arriving response script attempts to call the callback after the timeout. Unit tests in `test/util/jsonp.test.ts` have been updated to verify this behavior, including a robust check for property removal on success. Co-authored-by: deflis <206113+deflis@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the jsonp utility to improve memory management and error handling. The cleanup function now distinguishes between successful completions and timeouts; on success, the callback is deleted from the global window object to prevent memory leaks, while on timeout, it is replaced with a noop function to suppress console errors from delayed responses. Corresponding tests have been added to verify these behaviors. I have no feedback to provide.
This commit addresses a memory leak in the JSONP utility function where
callback properties were accumulating on the global `window` object.
A hybrid approach has been implemented based on user requirements:
- On success: The callback property is deleted from `window` to reclaim memory.
- On timeout: The callback is replaced with a `noop` function to suppress
errors from late-loading scripts.
Additionally, a lint error was fixed by using optional catch binding
(`catch { ... }`) for the `delete` fallback logic.
Tests have been updated to verify property deletion on success and
`noop` replacement on timeout.
Co-authored-by: deflis <206113+deflis@users.noreply.github.com>
1 participant
🎯 What: Fixed a memory leak in the
⚠️ Risk: Continuous use of JSONP would lead to an accumulation of properties on the global
jsonputility function using a hybrid cleanup strategy.windowobject, potentially causing memory issues in long-running applications.🛡️ Solution:
delete window[id]to fully reclaim memory.noopfunction to suppress errors from late-loading scripts.PR created automatically by Jules for task 8630123667505666188 started by @deflis