Opened 11 years ago

Closed 10 years ago

#9268 closed defect (fixed)

Massive memory leak in Internet Explorer 7 caused by dijit._editor.RichText

Reported by: Snowman Owned by: bill
Priority: high Milestone: 1.3.2
Component: Editor Version: 1.3.0
Keywords: memory leak richtext editor onfocus _loadFunc iframe ie ie7 Cc: dojo@…
Blocked By: Blocking:

Description

See attached file for demonstration of the leak (I've only tested it in Internet Explorer 7 on WinXP). Note that the leak is at least 2 to 3MB per page refresh, and can be much (MUCH!) larger for complicated pages with lots of JavaScript?.

Cause:

RichText?.js creates two references from its IFrame to JavaScript? objects that are never cleaned up explicitly: An "onfocus" handler (connected in the "onLoad" method) and the "_loadFunc" function (set up in the "open" method). The "onfocus" handler is created only in IE, the "_loadFunc" function is created in IE, WebKit? & Opera.

The memory leak can be fixed by cleaning up these references in the "destroyRendering" method (which is currently empty), for example:

destroyRendering: function() {
  if (dojo.isIE) {
    this.iframe.onfocus = null;
  }
  this.iframe._loadFunc = null;
},

Attachments (1)

editor_memleak.html (935 bytes) - added by Snowman 11 years ago.

Download all attachments as: .zip

Change History (19)

Changed 11 years ago by Snowman

Attachment: editor_memleak.html added

comment:1 Changed 11 years ago by bill

Component: GeneralEditor
Milestone: tbd1.4
Owner: changed from anonymous to bill
Status: newassigned

OK, I'll destroy those handlers in destroyRendering(), although this makes me wonder why we aren't calling _Widget.destroyRendering() to get rid of the whole iframe. It's purposely avoiding doing that ever since [3518].

comment:2 Changed 11 years ago by bill

Resolution: fixed
Status: assignedclosed

(In [17509]) Fix big memory leak on IE; we need to clobber the event handlers on the <iframe> before destroying it (fixes #9268). Thanks to Snowman for pointing out problem/fix.

Also adds minimal fix for dangling widgetid=editor1 left in DOM tree after editor1.destroy() is called, which was messing up page unload (fixes #9275). Note that I removed RichText?'s call to destroyRendering() since _Widget.destroy() already calls destroyRendering().

!strict

comment:3 Changed 11 years ago by davidmark

Resolution: fixed
Status: closedreopened

IE memory leaks are caused by circular references involving host objects.

http://www.jibbering.com/faq/faq_notes/closures.html#clMem

Should be able to avoid the circular reference, eliminating the need to clean it up. Looking into it as the proposed solution uses a browser detection flag (soon to be eliminated.)

comment:4 Changed 11 years ago by davidmark

The fix is:

ifr = null;

(last line of the open method.)

That breaks the circular reference. There is none that I can see with the focus listener.

There was an odd Safari-only setTimeout that needed attention in the trunk as well (relies on preserving the ifr reference.) Should be simple enough to fix (e.g. use hitch.)

There is a lot of browser detection in this module. I've removed most of it in my branch, but it will be a month before I get to testing the Dijit widgets.

comment:5 Changed 11 years ago by bill

Umm, I tested for leaks after [17509] and didn't see any. Do you have a test case that shows that there's still a leak?

comment:6 in reply to:  5 Changed 11 years ago by davidmark

Replying to bill:

Umm, I tested for leaks after [17509] and didn't see any. Do you have a test case that shows that there's still a leak?

There shouldn't be a leak in either case. It is just that my solution addresses the underlying problem, while the other works around it. There's no need to preserve the reference to the Iframe, so we can set it to null before exiting the open method. The other two lines can be deleted as no evasive action is necessary at that point. Also would leak if for some reason the teardown is not called (added complexity.)

comment:7 Changed 11 years ago by bill

Resolution: fixed
Status: reopenedclosed

Sorry, I don't see any reason to worry about a local variable that's going out of scope. In any case though I'm closing this ticket since the problem mentioned in the description is fixed.

comment:8 Changed 11 years ago by davidmark

I recommend it be re-opened. The whole point is that the reference to the IFrame is being preserved in a closure. That's what creates the circular reference which causes the leak, requiring a patch to plug it. Furthermore, if the destroyRendering method is not called by an unload listener, there is no guarantee it will be called at all, so the possibility of leaks remains (and unload listeners are best avoided.)

The current patch also uses browser detection, which is in the process of being removed from Dojo. Fixing the problem directly, which should be the first choice, involves no browser detection, less code, less complexity and better encapsulation (i.e. does not rely on another method call to work.)

It's fixed in my branch. Will make the merge a bit easier if a similar fix is applied to the trunk.

comment:9 Changed 11 years ago by bill

Isn't there still a circular reference? ifr._loadFunc references this, and this.iframe points back to the iframe (aka ifr).

Perhaps we could clear _loadFunc() as soon as it fires, rather than (or in addition to) clearing it in destroy(). But onfocus() needs to be defined until the Editor is destroyed, and that too has a circular reference.

comment:10 in reply to:  9 Changed 11 years ago by davidmark

Replying to bill:

Isn't there still a circular reference? ifr._loadFunc references this, and this.iframe points back to the iframe (aka ifr).

No. The objected pointed to by the - this - identifier is not referenced by the function's variable object.

Perhaps we could clear _loadFunc() as soon as it fires, rather than (or in addition to) clearing it in destroy(). But onfocus() needs to be defined until the Editor is destroyed, and that too has a circular reference.

Remove this in onLoad:

var _this = this;

That will create a circular reference with the focus listener as it preserves a reference to the editor object, which references the IFrame, etc. Use hitch instead.

comment:11 Changed 11 years ago by bill

I don't know what you mean by "variable object" but clearly _loadFunc references this, so it looks circular to me, albeit w/one more level of indirection.

Nor do I see that using dojo.hitch() rather than _this will remove a circularity. dojo.hitch(this, ...) effectively declares a local variable called scope equivalent to _this.

Anyway, you mentioned above that you haven't tested your changes yet. After you've tested them on all platforms we can consider adding them into dijit. I'm not going to change things now.

comment:12 in reply to:  11 Changed 11 years ago by davidmark

Replying to bill:

I don't know what you mean by "variable object" but clearly _loadFunc references this, so it looks circular to me, albeit w/one more level of indirection.

http://www.jibbering.com/faq/faq_notes/closures.html#clIRExSc

Appears hitch is complicating things here.

Nor do I see that using dojo.hitch() rather than _this will remove a circularity. dojo.hitch(this, ...) effectively declares a local variable called scope equivalent to _this.

It sets the - this - identifier, which is unrelated to scope (the docs are not using the term properly.) This is the onfocus circular reference:

[_this]->[iframe]->[focus listener]->[variable object]->[_this]

I recommended the hitch method to eliminate the need to preserve a reference to the editor object (_this.) The question is whether that will create a new circular reference. Looking at the hitch method, it appears it will. The primary complication here is that the Object objects have properties that are DOM nodes.

Can get around this for the moment:

var editNode = this.editNode = this.document.body.firstChild;
var tabStop = this.tabStop = dojo.doc.createElement('div');
tabStop.tabIndex = -1;

if (typeof editNode.setActive != 'undefined') {
	this.editingArea.appendChild(tabStop);
	this.iframe.onfocus = function(){
		editNode.setActive();
	};
}
tabStop = null; // Don't need this one

And add this last line to the method:

ap = null;

The - ap - variable held a reference to the IFrame, so would create this chain:

[Iframe]->[focus listener]->[variable object]->[IFrame]

Setting - ap - to null breaks the last link.

That will do that one. The other one needs more reorganization as it requires a reference to the Object object, which references the Iframe and several other host objects. Go ahead and set it to null in body onload (right after it is called.) I'll look at this again when I get back to Dijit as we shouldn't be augmenting host objects.

Anyway, you mentioned above that you haven't tested your changes yet. After you've tested them on all platforms we can consider adding them into dijit. I'm not going to change things now.

I've been dealing with these IE leaks for a long time. Once you understand what is happening behind the scenes, they are easily avoidable. These only need to be tested in IE (the other browsers are not affected one way or the other.) We'll deal with the other changes later. This file in particular will be very different once all of the browser detection is removed. That's the main reason I re-opened the ticket as it was adding more browser detection.

comment:13 Changed 11 years ago by bill

I tried the changes you suggested for the onfocus handler, to remove the circular dependency. However, they didn't work. When I remove the code in close() to null-out the onfocus handler, there's still a big leak when refreshing the page in test_Editor.html. This is on IE6.

So, my suggestion to you is to actually try these changes yourself, and if/when you get something that removes memory leaks on IE and correctly functions on all browsers -- not just IE, because remember that safari has a setTimeout() and you need to make sure you don't break that -- then supply a patch and I can probably check it in.

comment:14 in reply to:  13 Changed 11 years ago by davidmark

Replying to bill:

I tried the changes you suggested for the onfocus handler, to remove the circular dependency. However, they didn't work. When I remove the code in close() to null-out the onfocus handler, there's still a big leak when refreshing the page in test_Editor.html. This is on IE6.

IE6 is the one that leaks. Supposedly they fixed the garbage collection issue in IE7.

You have to fix both of them for it to work. The one fix removed the circular reference involving the focus listener. But the other one (_loadFunc expando) will still leak memory. I just removed the _loadFunc expando from my branch, so that one won't be an issue in the future (and one less host object augmentation to worry about.)

So, my suggestion to you is to actually try these changes yourself, and if/when you get something that removes memory leaks on IE and correctly functions on all browsers -- not just IE, because remember that safari has a setTimeout() and you need to make sure you don't break that -- then supply a patch and I can probably check it in.

This is a simple problem and it is important to understand how these leaks get introduced (so as to avoid them in the future.) The leaks will vanish like magic once both chains are broken, just as they are with the patch on unload. I will test when I get to this point in my branch, but it would make sense to me to take the opportunity to fix this properly in the trunk. And the Safari setTimeout can use hitch without creating a circular reference.

Also, it is trivial to modify the hitch method to protect against circular references. It's on my list as I'm sure this issue will come up again.

comment:15 Changed 11 years ago by bill

You misunderstood my comments above about your suggested focus fix not working, but anyway, I don't want to discuss this any more until you have a tested patch that simplifies the Editor code while still not leaking memory.

comment:16 in reply to:  15 Changed 11 years ago by davidmark

Replying to bill:

You misunderstood my comments above about your suggested focus fix not working, but anyway, I don't want to discuss this any more until you have a tested patch that simplifies the Editor code while still not leaking memory.

I don't have time to create a patch for the trunk (or test anything in IE6) at the moment, but I would like to help you understand the problem. These leaks are *very* easy to introduce, but just as easy to avoid if you know how to spot them. Unraveling leaky scripts and diagramming the chains is how I learned to spot them. This one is fairly well tangled, but no Gordian knot. As such, it is a good opportunity to learn.

The current patch is a workaround with browser detection and is not guaranteed to work without the use of an unload listener. Both of these practices should be avoided. I am trying to illustrate the cause of the underlying problem so that such workarounds will be unnecessary.

You've got two leaks in this file (at least.) One is the _loadFunc expando, which is also a complication as it augments a host object (also best avoided.) I fixed that this morning (see my branch.) The other is simpler. If you applied my above suggested changes to the onLoad method and removed *only* the line that set the onfocus property to null and still had a leak, then we have this:

[Iframe]->[focus listener]->???->[Iframe]

So, looking at the scope of the onLoad method, I see only one possibility that leads ultimately back to the Iframe and that is the DOM reference held in editNode (first child of the body.) I'm glad we had this discussion as this is a new wrinkle (and further illustrates the potential perils with host object references and closures.)

[Iframe]--onfocus-->[focus listener]->[variable object]->[first child of body]--ownerDocument-->[document]--parentWindow-->[window]--frameElement-->[Iframe]

That's as maybe. We can avoid the entire situation by eliminating all host objects from the scope of the focus listener (and eliminating the closure while we are at it.)

	_onFocusFactory: function() {
		var id = this.id;

		return function() {
			dijit.byId(id).editNode.setActive();
		};
	},

	_focusTimer: function(){
		setTimeout(dojo.hitch(this, "focus"), this.updateInterval)
	},

And make these two changes in the onLoad method:

this.iframe.onfocus = this._onFocusFactory();

[...]

dojo.addOnLoad(dojo.hitch(this, this._focusTimer));

For good measure, change the last line in onLoad to discard all DOM references:

editNode = tabStop = win = ap = null;

Now there is no closure and only a harmless string in the scope of the focus listener, which cannot possibly lead back to the IFrame.

Would be simpler if hitch had leak protection. I'll put that in later today.

And have fun with this. If we can't laugh at IE6, what can we laugh at? :) Right now it is laughing at us for having to work around it. :(

comment:17 Changed 10 years ago by bill

Milestone: 1.41.3.2
Resolution: fixed
Status: closedreopened

Should backport this to 1.3 branch...

comment:18 Changed 10 years ago by bill

Resolution: fixed
Status: reopenedclosed

(In [18472]) Backport editor memory leak fix in [17509] to 1.3 branch, fixes #9268 !strict.

Note: See TracTickets for help on using tickets.