Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#16833 closed defect (fixed)

[patch] dojo.destroy() memory leak after [30096]

Reported by: bill Owned by: Douglas Hays
Priority: high Milestone: 1.9
Component: HTML Version: 1.8.3
Keywords: Cc: Eugene Lazutkin
Blocked By: Blocking:

Description

See attached test case, which happens to use the (deprecated?) dojox/grid but is really to demonstrate memory leaks with dojo.destroy().

On [30095], after the test completes (after pressing the button and waiting for 100 grids to be created and destroyed), IE8 uses 60M of memory, which is not much more than the 58M of memory used after the initial grid is created.

On [30096], after the test completes, IE8 is using 221M.

I know your tests in #12342 showed no memory leak... perhaps the differences is having listeners on the nodes being deleted? According to #16478 sIeve also shows, which we were dismissing as spurious, but perhaps we shouldn't.

Attachments (4)

grid.html (33.1 KB) - added by bill 6 years ago.
test case
16833.patch (518 bytes) - added by Douglas Hays 6 years ago.
additionally use outerHTML for IE <= 8
16833.2.patch (531 bytes) - added by Douglas Hays 6 years ago.
use removeNode for IE in place of removeChild + outerHTML for cleaner leak prevention
removeNode.html (1.1 KB) - added by Douglas Hays 6 years ago.
IE10 test file showing removeNode(false) is superior to both removeChild and removeNode(true) at cleaning up memory

Download all attachments as: .zip

Change History (18)

comment:1 Changed 6 years ago by bill

Milestone: tbd1.9
Owner: changed from Eugene Lazutkin to Douglas Hays
Status: newassigned

Marking for consideration for 1.9; it should at least be looked at before we release.

Changed 6 years ago by bill

Attachment: grid.html added

test case

comment:2 Changed 6 years ago by bill

Summary: memory leak after [30096]dojo.destroy() memory leak after [30096]

Changed 6 years ago by Douglas Hays

Attachment: 16833.patch added

additionally use outerHTML for IE <= 8

comment:3 Changed 6 years ago by Douglas Hays

Cc: Eugene Lazutkin added
Priority: undecidedhigh

comment:4 Changed 6 years ago by Douglas Hays

bill, do you have any examples of leaks involving IE 9 or 10? I added a patch that intentionally adds an IE 8 workaround to coerce IE to release memory being referenced by nodes that have been removed from the DOM. I could expand it to IE 9 and 10 but would prefer to limit it's scope.

comment:5 Changed 6 years ago by bill

grid.html is the only example I have (and by your wording I assume it's not leaking on IE9+?)

Changed 6 years ago by Douglas Hays

Attachment: 16833.2.patch added

use removeNode for IE in place of removeChild + outerHTML for cleaner leak prevention

comment:6 Changed 6 years ago by Douglas Hays

I'd like to get an approval to commit the 16833_2.patch and backport thru 1.6 (last version containing [30096]).

comment:7 Changed 6 years ago by Douglas Hays

Summary: dojo.destroy() memory leak after [30096][patch] dojo.destroy() memory leak after [30096]

I ran performance tests comparing removeNode and removeChild in IE and they are essentially equivalent.

Changed 6 years ago by Douglas Hays

Attachment: removeNode.html added

IE10 test file showing removeNode(false) is superior to both removeChild and removeNode(true) at cleaning up memory

comment:8 in reply to:  7 Changed 6 years ago by Eugene Lazutkin

Replying to doughays:

I ran performance tests comparing removeNode and removeChild in IE and they are essentially equivalent.

I looked at 16833.2.patch. It looks minimal (a one-liner) meaning that it should be simple to test its impact, and I trust that you tested it thoroughly. Please, commit it, and close the ticket.

To comment on the ticket description: the problem with simple innerHTML = "" approach is that it doesn't always clear nodes with still attached event listeners on old IE. Its reason was due to the fact that old IE uses a reference-counting GC, but keeps counters in DOM and JS separately. It can lead to a curious circular references, which are isolated, yet cannot be GC'ed because a part of the loop in DOM, and another part in JS objects --- IE's GC cannot trace loops across DOM/JS boundaries.

Typical "undead" loops are when DOM nodes have listeners, and objects point to DOM nodes back, but there are more exotic varieties, when a closure has an implicit back-reference to a DOM node --- IE6 is famous for including never-used references in closures. IE7 cleaned it up significantly but didn't eliminate such type of leaks completely.

comment:9 Changed 6 years ago by Douglas Hays

Resolution: fixed
Status: assignedclosed

In [31027]:

Fixes #16833. Use removeNode on IE to avoid leaks when event handlers are on the node and removeChild everywhere else. Backport thru 1.6. !strict

comment:10 Changed 6 years ago by Bryan Forbes

Using innerHTML = "" should be perfectly fine if event handlers were connected using either dojo/_base/connect or dojo/on. Both of those modules work around the leak by dereferencing the event handlers through the global scope which allows old IE to clean up event handlers with references to the node in their scope chain (which is where the leak occurs and has nothing to do with referencing or not referencing the variable). Please see http://www.reigndropsfall.net/2011/01/05/internet-explorer-event-handler-leaks/ for more information about how we work around the event handler leak.

It looks like the original patch in [30096] (which eliminated the old "garbage can" approach) was developed to work around a corner case of trying to do innerHTML = "" on an iframe in IE9. It seems silly to move to a solution based on browser sniffs and proprietary methods to avoid a tried and true method of node destruction that works everywhere except for iframes in one browser.

comment:11 Changed 6 years ago by bill

Bryan - It wasn't [just] an issue with iframes. See the grid.html test case I attached above. Are you implying that dojox/grid is using iframes and/or attaching events directly rather than through dojo/_base/connect? If so, which line of code?

comment:12 Changed 6 years ago by Bryan Forbes

I'm not suggesting anything of the sort, although I can't comment on what exactly the EnhancedGrid does so I can't be sure if it's using direct event connection or iframes. I was simply referring to this: you said in your original report that no leak was occurring at [30095] and then [30096] introduced the leak. The change between the two is moving away from the "garbage can" approach which we've had for a long time. Rather than solve the leak introduced by the changes introduced at [30096] by reverting those changes, several attempts at solving the leak were tried until finally a runtime check of a browser sniff and a proprietary method were decided upon. I don't understand how the new way is better or what problem we have solved by telling IE to remove the object from the document hierarchy without destroying its children (which is what passing false to removeNode does).

comment:13 Changed 6 years ago by bill

I think the goal was to fix both #12342 and the leak reported in this ticket. Is there a better way to achieve both those things?

comment:14 Changed 6 years ago by Douglas Hays

In [31049]:

Refs #16833. svg nodes do not have a removeNode method, but don't seem to leak either with removeChild. !strict

Note: See TracTickets for help on using tickets.