Repository navigation
Iframes Memory Leak with jQuery and Sizzle #5281
Description
Description
We have a situation where there are 3 levels of nested iframes. The 3rd iframe initializes few heavy size objects, but this iframe is discarded and rebuilt in the DOM whenever user navigates across the application.
A memory leak is noticed where heap size of the application keeps on increasing whenever a new iframe at 3rd level is discarded and constructed.
Link to test case
Goto https://vamsikrishnach90.github.io/ in Chrome. Open developer tools (Hit F12) and navigate to Memory Tab. Pay attention to the JS heap size which is arppox 5 MB.
- Click on 'Load Child Page' button. A child page is loaded inside the iframe of parent page.
- Click on 'Bombard the page!' button on the child page. This will initialize 1 global variable of custom type with a huge json. Open devtools and check for the heap which is now 75 MB.
- Click on 'Remove Child Page' button. The child page (iframe) is removed from the DOM.
Repeat steps 1 - 3 for about 2 more times. After step 2, note down the heap size. You'll notice that the heap keeps on increasing every time we add a child iframe and bombard it. Even though at step 3, we discard the frame, the heap is not claimed by the garbage collector.
Usecase demo help (Picture)
Observations
After repeating above 1-3 steps for 3x times, take a heap snapshot. Notice that the 3 instances of custom class 'Vamsi' that were created every time we added the child page and bombarded it still exist in the heap, even though the corresponding iframes are deleted from the DOM.
Notice in the object graph, that an unloadHandler() has been bound to the Window object belonging to the discarded iframes. Heap Memory Snapshot
Root Cause
The child page has a jquery lookup statement in the parent page:
$('div.msg-banner', window.parent.document).show();
This one line statement is causing the issue for memory leak.
$('div.msg-banner', window.parent.document)
The above part of the statement, calls JQuery find api. This find API is overridden by Sizzle, a pure-JavaScript CSS selector engine adopted by JQuery. This find api from JQuery calls Sizzle's setDocument function See Picture which registers an unloadHandler() with the requested document's(window.parent.document) window object. This is leading in the parent page's unload event bound to unloadHandler() callback function sitting in the child page. Hence, even though the child page iframe is removed from the DOM, the window object is never eligible to be garbage collected and is the reason for memory leak.
Link to Stackoverflow thread: https://stackoverflow.com/questions/76579826/iframes-memory-leak-with-jquery-and-sizzle
Activity
mgol commentedon Jun 29, 2023
Thanks for the report. The unload handler is needed by IE & Edge Legacy; in jQuery 4 that drops support for the latter, we'll explicitly only set it in IE, avoiding the memory leak outside of IE.
To avoid the leak, IE or not, we'd probably have to add an additional
unloadhandler on the document on which jQuery is loaded that would collect all theunloadhandlers attached to other documents and remove them when the main document is unloaded. This might require significant code to pull off, though, which I'm not too happy about.mgol commentedon Jun 29, 2023
We could also consider limiting the issues to IE & Edge Legacy even in the 3.x line by doing an indirect support test - by checking of presence of
documentElement.msMatchesSelector: https://caniuse.com/matchesselectorNeither IE nor Edge Legacy are developed anymore and I cannot imagine modern browsers now suddenly adding this non-standard API so such a check should be safe.
A real fix for all browsers would probably require defining something like:
and then, in the place where we currently attach
unloadHandler, to instead call:That would be +24 bytes, though. If we went for this, I'd also want to first make sure this still fixes the IE issues and I have troubles actually reproducing it in IE 11 with the
unloadHandlerlogic removed. I looked at test cases from https://bugs.jquery.com/ticket/13936 & https://bugs.jquery.com/ticket/14535 and none of them fail without the patch. I suspect the code got simplified and there are fewer code paths that don't already invokesetDocumentearly enough to hit the issue. But if we want to do this, I'd like to first find such a code path.Help with finding a good test case is welcome!
vamsikrishnach90 commentedon Jun 29, 2023
Thanks for responding and acknowledging the problem. Let me know when you have next "Plan of Actions" to fix this issue after your internal meeting and review.
mgol commentedon Jun 29, 2023
Just for reference - @vamsikrishnach90 would you be satisfied if we fix the issue outside of IE & Edge Legacy or do you care about those legacy browsers as well?
I’m asking because every workaround incurs cost: additional size, testing & maintenance burden. This issue existed in the code base for ages and AFAIK this is the first report we have about it. That suggests it may be an edge case and caring about it in IE might be an even bigger edge case.
vamsikrishnach90 commentedon Jun 29, 2023
I am fine with the fix being available to all other browsers except IE & Edge Legacy. @mgol Which version of JQuery do you think is going to receive a fix for this?
mgol commentedon Jun 29, 2023
We still need to discuss this but I'd expect changes to arrive in jQuery 3.7.1.
Selector: Only attach the unload handler in IE & Edge Legacy
mgol commentedon Jun 29, 2023
PR: #5282
mgol commentedon Jun 29, 2023
My workaround from #5281 (comment) would require more changes as
setDocument()should only be called forsubWindowunload so handlers need to be different. I think for now I prefer to just skip the workaround outside of IE & Edge Legacy and accept the memory leak in those two browsers.[-]Iframes Memory Leak with JQuery and Sizzle[/-][+]Iframes Memory Leak with jQuery and Sizzle[/+]Selector: Only attach the unload handler in IE & Edge Legacy
mgol commentedon Jul 10, 2023
Fixes for non-IE non-Edge Legacy browsers in #5282 - and that's all we plan, closing.