fixed memory leak while importing html elements in IE #644

bbsteventill · 2016-10-28T19:08:00Z

541 - fixed ie memory leak when importing multiple imports

<!-- import.html -->

<link rel="import" href="/assets/paper-something/paper-something.html" />

<!-- index.html -->

<html><head></head><body>
<link rel="import" href="/import.html" />
</body></html>

this wil no longer leak in IE 11

justinfagnani · 2016-11-01T21:05:50Z

@bbsteventill can you give a brief description of the fix and why it prevents the memory leak?

justinfagnani · 2016-11-01T21:38:28Z

src/CustomElements/observe.js

-  var observer = node.__observer;
+
+  // this needs to be on head or it will leak in IE
+  // IE does not like it when you have non-standard attributes on root dom's, so put


What non-standard attribute is on the root dom, and what's the "root dom"? A document? If so, that answers my next question, which is how do we know the node has a head property?

Also, do we need node.head && node.head.__observer?

The comment on line 250 says node will be either a document or ShadowRoot. ShadowRoot won't have a head, do we have test coverage on this case?

My findings are attributes that are not defined in the w3 spec. I found an example that will cause a leak as part of the UrlResolver in polymer where a 'a' element is being appended to the root document, this will cause a memory leak, but if you set the baseURI that does NOT cause a memory leak. I think it is because baseURI is defined in the w3 spec while __url is not (https://github.com/Polymer/polymer/pull/3964/files)
Note: the easiest way to test this is to create a iron-image that does not have a valid url as its source - it should start to leak in ie because of the .__url not being on the head element.

a root dom, is any root document that has been created with document.implementation.createHTMLDocument

this is why we need the node.head && node.head.__observer, because placing the observer directly on the node (this is where the code is poorly named because node is always a root shadow document at this point in the code)

we do have test coverage of this, the tests will break if you remove the createElement('head') && createElement('body) that was added Element.js

justinfagnani · 2016-11-01T21:39:20Z

src/CustomElements/observe.js

  var observer = new MutationObserver(handler.bind(this, inRoot));
-  observer.observe(inRoot, {childList: true, subtree: true});
-  inRoot.__observer = observer;
+  observer.observe(inRoot.head, {childList: true, subtree: true});


If we're observing both head and body, why not just observe the whole document?

that my friend is what will causes a memory leak in IE; which is what it was before

justinfagnani · 2016-11-01T21:48:26Z

src/HTMLImports/importer.js

@@ -74,7 +74,9 @@ var importer = {
        // generate an HTMLDocument from data
        doc = err ? null : makeDocument(resource, redirectedUrl || url);
        if (doc) {
-          doc.__importLink = elt;
+          // IE will leak if you put the node directly on the shadow dom


by "shadow dom" do you mean ShadowRoot? Which variable here is a ShadowRoot? Maybe this should say "Document"

…ks in IE

davidmedina-contact · 2016-12-01T18:05:06Z

This merge causes an error in IE11 for my components:

SCRIPT5007: Unable to get property 'getAttribute' of undefined or null reference
File: webcomponents-lite.min.js, Line: 64, Column: 3

var src = script.getAttribute('src');

bbsteventill · 2016-12-01T20:20:14Z

@jdmedina More information is required; first do not use your own custom minified version of the script. Second I know that the debugger in IE does different things then what happens in chrome; for example when you try to view properties on a prototype I think the IE debugger evaluates the properties for you; this may be bad because things like script.getAttribute('src'), your script object may be null

davidmedina-contact · 2016-12-01T21:21:24Z

@bbsteventill By custom you mean the lite version (sans shady dom)? Even though IE was saying the error is on that minified file, I was not using that minified file. I am using Fiddler to hot-swap the file we're using in our test server with the a version of webcomponents-lite.js in the master locally.

Even with the debugger off, the components break on my page and don't work.

Let me know what other type of extra info you need.

jay8t6 · 2016-12-06T14:35:33Z

@justinfagnani @bbsteventill when is this gonna be tagged?

omsmith · 2016-12-20T20:43:56Z

Seeing very big improvements in IE11 with this PR. Thanks very much @bbsteventill :)

bbsteventill added 3 commits October 21, 2016 14:40

fixed memory leak while importing html elements in IE

a2382fd

fixed broken tests, works when testing locally

d29f3ac

fixed broken tests

6d8fe16

bbsteventill force-pushed the 541-memory-leak-ie11 branch from 08529e2 to 6d8fe16 Compare October 31, 2016 16:09

541 - added comments about ie leaking when you observe root nodes, etc

547cddf

justinfagnani suggested changes Nov 1, 2016

View reviewed changes

updated comments to make clearer what is going on with the memory lea…

e2d563a

…ks in IE

bbsteventill force-pushed the 541-memory-leak-ie11 branch from fd00c30 to e2d563a Compare November 15, 2016 18:01

bbsteventill mentioned this pull request Nov 21, 2016

3430 - ie memory leak fixes - disable event caching, fixed resolver u… Polymer/polymer#3964

Merged

justinfagnani approved these changes Nov 22, 2016

View reviewed changes

justinfagnani merged commit 0d6b57b into webcomponents:master Nov 22, 2016

ergo mentioned this pull request Dec 13, 2016

Memory Leak in IE11 even after refreshing the whole page #541

Closed

sorvell mentioned this pull request Jan 26, 2017

Revert "fixed memory leak while importing html elements in IE" #665

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixed memory leak while importing html elements in IE #644

fixed memory leak while importing html elements in IE #644

bbsteventill commented Oct 28, 2016 •

edited by ebidel

Loading

justinfagnani commented Nov 1, 2016

justinfagnani Nov 1, 2016

justinfagnani Nov 1, 2016

justinfagnani Nov 1, 2016

bbsteventill Nov 1, 2016 •

edited

Loading

justinfagnani Nov 1, 2016

bbsteventill Nov 1, 2016

justinfagnani Nov 1, 2016

davidmedina-contact commented Dec 1, 2016

bbsteventill commented Dec 1, 2016

davidmedina-contact commented Dec 1, 2016 •

edited

Loading

jay8t6 commented Dec 6, 2016

omsmith commented Dec 20, 2016

fixed memory leak while importing html elements in IE #644

fixed memory leak while importing html elements in IE #644

Conversation

bbsteventill commented Oct 28, 2016 • edited by ebidel Loading

justinfagnani commented Nov 1, 2016

justinfagnani Nov 1, 2016

Choose a reason for hiding this comment

justinfagnani Nov 1, 2016

Choose a reason for hiding this comment

justinfagnani Nov 1, 2016

Choose a reason for hiding this comment

bbsteventill Nov 1, 2016 • edited Loading

Choose a reason for hiding this comment

justinfagnani Nov 1, 2016

Choose a reason for hiding this comment

bbsteventill Nov 1, 2016

Choose a reason for hiding this comment

justinfagnani Nov 1, 2016

Choose a reason for hiding this comment

davidmedina-contact commented Dec 1, 2016

bbsteventill commented Dec 1, 2016

davidmedina-contact commented Dec 1, 2016 • edited Loading

jay8t6 commented Dec 6, 2016

omsmith commented Dec 20, 2016

bbsteventill commented Oct 28, 2016 •

edited by ebidel

Loading

bbsteventill Nov 1, 2016 •

edited

Loading

davidmedina-contact commented Dec 1, 2016 •

edited

Loading