search

prototypejs / prototype

Prototype JavaScript framework
http://prototypejs.org/
Other
3.54k stars 639 forks source link

IE8 Memory Leak with no script or content, just Prototype >= 1.7.1.0 #275

Closed ghost closed 8 years ago

ghost commented 9 years ago

Given the following HTML, IE8 grows memory usage in the task manager by ~1mb per second:

<html>
<head>
    <meta http-equiv="X-UA-Compatible" content="IE=8">
    <script src="//ajax.googleapis.com/ajax/libs/prototype/1.7.1.0/prototype.js"></script>
    <meta http-equiv="refresh" content="1">
</head>
<body>
</body>
</html>

This occurs with IE8 version 8.0.7601.17514 supplied by Microsoft as a Virtualbox VM here: https://www.modern.ie/en-gb/virtualization-tools (IE8, Win7, Build 20131127)

This does not occur when any of these conditions are met:

Version 1.7.2.0 also exhibits this problem.

Does anyone know what may have changed in 1.7.1.0 onwards that may have caused this?

ghost commented 9 years ago

As an update, the trigger for the memory leak is IE8 Standards Mode vs Quirks Mode (which is what the tag is forcing on and off).

In Quirks mode this leak does not occur.

savetheclocktower commented 9 years ago

Haven't investigated yet, but what changed from 1.7.0 to 1.7.1 was a complete rewrite of dom.js. Maybe we're creating an element that never gets nulled out.

savetheclocktower commented 9 years ago

I can confirm this is happening in IE8 in standards mode. In quirks mode, it doesn't happen. In IE10 it doesn't happen no matter which mode we're in.

The venerable sIEve doesn't detect any leaks. I am… stumped for now.

ghost commented 9 years ago

Using that exact HTML from the first post, sIEve shows 1 leak for every time you press the 'Go' button (see attached).

275-sieve

A DIV tag is apparently the culprit but I can't see any way of connecting that to the code that instantiated it.

savetheclocktower commented 9 years ago

Well, it's annoying that you can reproduce that and I can't, but at least it's something to go on.

One way to narrow it down would be to find all the places that we create a div (shouldn't be too many) and add an ID to each one after creation. Looks like sIEve will show the ID of the leaking element.

jwestbrook commented 9 years ago

I added line number ids to all (at least I think) the created div elements in this gist

https://gist.github.com/jwestbrook/fefe7a5c0fe01f8426ec

ghost commented 9 years ago

Ah, sorry for having you chase tails there, the DIV leak is in 1.7.1 and must've been plugged previously.

The code in your gist shows no leaks in sIEve.

ghost commented 9 years ago

At a glance, our application doesn't appear to be broken when using 1.7.0.

Do you know if rolling back to 1.7.0 in a production environment would have a negative impact or is that really dependent on how prototype has been used (e.g. do you know of many backward incompatible changes or new features added that could do with more rigorous testing)?

Also, I'd like to help in fixing this if I can, do you have any suggestions of where I could start?

jwestbrook commented 9 years ago

OK I updated the gist for 1.7.1, can you try that one?

https://gist.github.com/jwestbrook/fefe7a5c0fe01f8426ec

ghost commented 9 years ago

The leak I was seeing in sIEve for Prototype 1.7.1 (and visible in that last gist you posted) seems to have been fixed with this code:

  function destroyCache_IE() {
    DIV = null;
    ELEMENT_CACHE = null;
  }

  if (window.attachEvent)
    window.attachEvent('onunload', destroyCache_IE);

In these commits: https://github.com/sstephenson/prototype/commit/554d512cc5bd61e2c2b5c233bc356945b199c411 https://github.com/sstephenson/prototype/commit/37e6c1154d68b6333d3358ef69ecfb6d440476e2

Adding that same code to the gist confirms that.

The 1.7.2 code still shows 1mb memory increase for every refresh without any detected leaks in sIEve, which was why savetheclocktower was stumped. Not sure where to start diagnosing it besides just turning stuff on/off inside Prototype until it stops happening.

jwestbrook commented 9 years ago

Yes - I saw the same behavior in my test setup with 1.7.2 - I'll do some digging this weekend to try and find it

savetheclocktower commented 9 years ago

@aura-geoff-viljoen 1.7.1 and 1.7.2 had some pretty important fixes for newer IE versions, though I can't say for sure whether the stuff we fixed would affect your app. But my guess is that switching to 1.7.0 would fix this IE8 issue but create a couple new IE10 issues.

savetheclocktower commented 9 years ago

@aura-geoff-viljoen As for ways you could help: if you're up for it, you could go through a built file of prototype.js, look for places that could create leaks, then change them one-by-one to see if they address the issue.

Any place where we interact with host objects can be a culprit. Historically, it's been the DOM nodes that we create for capability checks (hence the importance of nulling out those variables after they're no longer needed), but XHR objects also fit that profile. Since loading the script is enough to trigger the memory use, you'd be looking for things that happen in the course of loading the file — like capability checks.

Another option, if you're well-versed in how we generate a built prototype.js, is to go into the source, remove these lines in src/prototype.js…

//= require "./prototype/dom"
//= require "./prototype/deprecated"

…and generate a new distributable for testing. If that doesn't exhibit the problem, then you know the culprit is in dom.js, at which point you can go into src/prototype/dom.js and repeat the process of elimination for the sub-sections of dom.js — though be careful, because some of them depend on one another.

Of course, I wouldn't hold it against you if you didn't want to do any of this.

ghost commented 9 years ago

Thanks for the great set of instructions and speedy feedback, they're really appreciated.

I went through the build process and removing the individual components flagged dom.js as the culprit.

Setting 'Element' to null in the unload function of dom.js seems to keep memory usage constant on the IE8 test box, the pull request of the change is available here: https://github.com/sstephenson/prototype/pull/276

I've re-run the test suite on Firefox (v30), IE8 and Chrome (v38.0.2125.111 m) before and after the change and none of the test results change (there were some pre-existing failures though).

Could you let me know if the fix is sane and resolves for you? If so, I'll patch my existing Prototype installation until a next release.

savetheclocktower commented 9 years ago

So I'm not sure what to do with this. It appears that setting Element to null fixes it, but that's a big gun to take out. At some point I'll see if I can narrow it down — whether it's a property of Element that leaks, or some closure created by one of the methods, or some weird third thing.

savetheclocktower commented 8 years ago

I think we're going to close this now that Microsoft is no longer supporting IE 8. The next version of Prototype will drop support for IE 8 as well.