ko.utils.domData storage scheme requires explicit cleanup #2141

mbest · 2017-07-21T05:50:39Z

This problem has been previously brought up in the rejected Issue #1695. Our case idifferent, however: we have no way of ensuring that the explicit ko.cleanNode() is always invoked for the removed element.
Also note that jQuery has switched to storing data on the expando - jquery/jquery#1734. And they had long been advocating for the same data storage solution as Knockout's...

Or use case is as follows:

We rely on custom element support (part of the web component spec) to activate our composite component (see https://developer.mozilla.org/en-US/docs/Web/API/Document/registerElement)
We are using Knockout to manage the composite component's template.
The template will only reference observables stored with the element's properties. No external observables will be used. This means that the custom element should be garbage-collectable when it is removed.
Custom elements are designed to rely on garbage collection for their cleanup, and we cannot require our users to call the explicit cleanup method.

Unfortunately, Knockout's domData maintains references to to all DOM nodes where bindings were applied, so all DOM nodes within the Knockout template remains in memory heap.

Here is a jsFiddle that demonstrates the issue: https://jsfiddle.net/mstarets/4nfsebec/5/. It needs to be run in Chrome (otherwise you will need to include the Custom Element polyfill). After you run the page, click on 'Create Custom Element button, then on 'Remove Custom Element' button. Repeat this sequence 2-3 times. Open Developer Tools, go to the 'Profiles' tab and take a heap snapshot. Note that there are several hundred of HTMLButtonElements that have been leaked. Now uncomment the line calling clearDataForButtons() and repeat the same test. Note that the button elements are no longer leaked.

mstarets · 2016-11-02T19:35:17Z

The fix could require an explicit opt-in (the old behavior would still be the default):

@@ -685,17 +685,30 @@
     var uniqueId = 0;
     var dataStoreKeyExpandoPropertyName = "__ko__" + (new Date).getTime();
     var dataStore = {};
+    var dataStoreInUse = false;

     function getAll(node, createIfNotFound) {
         var dataStoreKey = node[dataStoreKeyExpandoPropertyName];
-        var hasExistingDataStore = dataStoreKey && (dataStoreKey !== "null") && dataStore[dataStoreKey];
-        if (!hasExistingDataStore) {
-            if (!createIfNotFound)
-                return undefined;
-            dataStoreKey = node[dataStoreKeyExpandoPropertyName] = "ko" + uniqueId++;
-            dataStore[dataStoreKey] = {};
+        
+        if (dataStore) {
+          dataStoreInUse = true;
+          var hasExistingDataStore = dataStoreKey && (dataStoreKey !== "null") && dataStore[dataStoreKey];
+          if (!hasExistingDataStore) {
+              if (!createIfNotFound)
+                  return undefined;
+              dataStoreKey = node[dataStoreKeyExpandoPropertyName] = "ko" + uniqueId++;
+              dataStore[dataStoreKey] = {};
+          }
+          return dataStore[dataStoreKey];
         }
-        return dataStore[dataStoreKey];
+        else
+        {
+          var data = dataStoreKey;
+          if (!data && createIfNotFound) {
+            data = node[dataStoreKeyExpandoPropertyName] = {};
+          }
+          return data;
+        }
     }

     return {
@@ -715,7 +728,8 @@
         clear: function (node) {
             var dataStoreKey = node[dataStoreKeyExpandoPropertyName];
             if (dataStoreKey) {
-                delete dataStore[dataStoreKey];
+                if (dataStore)
+                  delete dataStore[dataStoreKey];
                 node[dataStoreKeyExpandoPropertyName] = null;
                 return true; // Exposing "did clean" flag purely so specs can infer whether things have been cleaned up as intended
             }
@@ -724,12 +738,18 @@

         nextKey: function () {
             return (uniqueId++) + dataStoreKeyExpandoPropertyName;
+        },
+        storeOnNode: function() {
+          if (dataStoreInUse)
+            throw "Cannot change data storage scheme after it has been initialized";
+           dataStore = null; 
         }
     };
 })();

 ko.exportSymbol('utils.domData', ko.utils.domData);
 ko.exportSymbol('utils.domData.clear', ko.utils.domData.clear); // Exporting only so specs can clear up after themselves fully
+ko.exportSymbol('utils.domData.storeOnNode', ko.utils.domData.storeOnNode);

 ko.utils.domNodeDisposal = new (function () {
     var domDataKey = ko.utils.domData.nextKey()

mstarets · 2016-11-02T19:38:51Z

Also, would you guys recommend entering this problem as a tko issue (the same code exists there)?

brianmhunt · 2016-11-02T22:45:41Z

I'd certainly like this to not be an issue in tko 😀

... but I still don't quite understand the nature of the issue -- will need to stare at it a bit before I can make a meaningful comment.

brianmhunt · 2016-11-02T22:47:49Z

... why not use ko.removeNode? Does that solve the issue?

mstarets · 2016-11-03T13:47:21Z

Hey Brian,

ko.removeNode() would certainly clean up DOM data, but it won't solve the issue for us. We are using Knockout as a template engine for the internal template within a custom element/web component. The page author using the component is not supposed to know or care that KO is managing the template (that decision was made by the web component author). They are just seeing a custom element in the DOM (for example, . Web component spec does not call for any explicit cleanup, and the assumption is that the garbage collection will just work after the custom element node is removed from the document.

I agree that current Knockout users are aware that they need to clean up Knockout explicitly. However, I also think that that it would be great if KO supported being used as a template engine within web components, and for that use case supporting cleanup by garbage collection is a must have.

Thanks,
Max

brianmhunt · 2016-11-03T18:05:13Z

Got it. Is using MutationObservers to see when elements are removed an option?

mstarets · 2016-11-03T18:24:48Z

Thanks, Brian,

Custom elements are already being notified when they are being attached or detached (the custom component polyfill uses mutation observers to implement that). The problem is that detach is not equivalent to destroy - it could easily be a temporary detach or a move. Allowing garbage collection to take care of things is the best solution, as nobody will be holding references to the custom element if they are not planning to re-insert it into the document.

Max

mbest · 2016-11-03T21:08:24Z

The current ko.utils.domData is part of the old-IE compatibility. Perhaps we can include an option to use a different method.

brianmhunt · 2016-11-04T00:18:38Z

Maybe using WeakMap @mbest?

mstarets · 2016-11-04T13:27:53Z

+1 to adding an option to switch domData implementation for those who do not care about supporting IE 6 and 7.

Regarding the WeakMap: I am not sure it provides much advantage over storing data in the node's expando. The keys in WeakMap have to be objects, so you would be forced to store an object on the DOM node in any case.
jQuery's implementation may be useful: jquery/jquery#1734. They do not support associating data with comments nodes, but AFAIK this was done a long time ago due to a reason that is not related to their switch away from the global data storage.

brianmhunt · 2016-11-04T14:09:18Z

If using a WeakMap, the DOM node would be the key.

The problem with WeakMap is browser support ... IE < 11.

brianmhunt · 2016-11-04T14:12:57Z

... another option I toyed with is to use a Symbol property on the node.

brianmhunt · 2016-11-10T12:25:29Z

So TKO prefers WeakSet if supported, hopefully resolving this issue.

mstarets · 2016-11-15T21:23:21Z

Thanks, Brian. The tko change looks good.

mstarets · 2016-12-09T21:58:15Z

Hey Brian,

Any chance of backporting the tko fix to the classic Knockout? We have a project that depends on using KO as a template manager with Web Components/Custom Elements, and the only other avenue would be forking.

brianmhunt · 2016-12-09T23:15:49Z

It's a simple patch and I've no issue with it being in 3.5 -- @mbest ?

mbest · 2016-12-09T23:29:07Z

I'll see what it would involve.

brianmhunt · 2016-12-10T03:36:05Z

@mbest This is the patch for tko:

knockout/tko.utils@a1ec331

The API stays the same.

mstarets · 2016-12-19T16:00:42Z

@mbest,

We would settle for an opt-in API for this change too if needed.

Thanks,
Max

mbest · 2017-07-21T04:29:13Z

@brianmhunt

I'm trying to use your code in Knockout and ran into this issue in IE 11:

Cross-window support Should work in another window.

TypeError: WeakMap.prototype.set: 'key' is not an object

Any ideas?

mbest · 2017-07-21T05:06:55Z

It looks like IE and Edge don't support using a node from another window in a WeakMap. Here is a workaround (needed on every access):

var data = node.ownerDocument[dataStoreKeyExpandoPropertyName] ||
    (node.ownerDocument[dataStoreKeyExpandoPropertyName] = new node.ownerDocument.defaultView.WeakMap());

brianmhunt · 2017-07-21T13:56:11Z

Thanks @mbest — I've opened an issue in TKO and will mull & merge a PR.

brianmhunt · 2017-07-21T14:20:56Z

src/utils.domData.js

+    if (window['WeakMap']) {
+        getDataForNode = function (node, createIfNotFound) {
+            var ownerDoc = node.ownerDocument,
+                dataStore = ownerDoc[dataStoreKeyExpandoPropertyName] || (ownerDoc[dataStoreKeyExpandoPropertyName] = new ownerDoc.defaultView['WeakMap']());


Good job finding this edge case (pardon the pun)

mbest · 2017-07-21T18:24:23Z

Maybe it's just simper to use @mstarets's technique of storing the data on the node itself. The old method would only need to be used in old-IE.

brianmhunt · 2017-07-21T21:28:38Z

Yeah an IE test is simpler than this ownerdox logic. Does latest Edge fail too? Is it a known issue?

mbest · 2017-07-21T21:48:45Z

Is it a known issue?

There doesn't seem to be a report of it.

https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/?page=1&q=weakmap

…eakMap.

mbest · 2017-07-22T04:10:02Z

This version doesn't use WeakMap. What do you think?

brianmhunt · 2017-07-22T13:11:21Z

I think this is the most practical approach.

I think we should be explicit about what happens with Edge. I wonder if there's a bug report / reporting facility / this would be considered a bug.

brianmhunt · 2017-07-22T13:12:04Z

src/utils.domData.js

-                if (getAll(node, false) === undefined)
-                    return;
+    var getDataForNode, clear;
+    if (!ko.utils.ieVersion) {


Perhaps this should be

if (!ko.utils.ieVersion && 'WeakMap' in window) {

We should also probably note the bug report here or explain why IE's WeakMap is broken.

mbest · 2017-07-22T16:03:17Z

I've reported the WeakMap issue: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/12891461/

brianmhunt · 2017-07-26T01:42:31Z

Noting chakra-core/ChakraCore#2983

brianmhunt added a commit to knockout/tko.utils that referenced this pull request Nov 10, 2016

Implement a WeakSet domData map see knockout/knockout#2141

a1ec331

mbest modified the milestone: Not assigned Dec 2, 2016

mbest added affected: all severity: minor type: feature labels Dec 9, 2016

mbest modified the milestones: 3.5.0, Not assigned Dec 9, 2016

brianmhunt added a commit to knockout/tko that referenced this pull request May 8, 2017

Implement a WeakSet domData map see knockout/knockout#2141

995df92

Use WeakMap is available for domData storage.

199fdb2

brianmhunt mentioned this pull request Jul 21, 2017

IE/Edge WeakMap cannot use cross-window nodes as keys knockout/tko#35

Closed

brianmhunt reviewed Jul 21, 2017

View reviewed changes

Store data on the node itself (except in old IE), rather than using W…

6c02fe1

…eakMap.

brianmhunt reviewed Jul 22, 2017

View reviewed changes

Add comments about domData choices.

c87ad68

mbest merged commit e57500a into master Jul 23, 2017

mbest deleted the 2141-domdata-weakmap branch July 23, 2017 19:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ko.utils.domData storage scheme requires explicit cleanup #2141

ko.utils.domData storage scheme requires explicit cleanup #2141

mbest commented Jul 21, 2017 •

edited by mstarets

Loading

mstarets commented Nov 2, 2016

mstarets commented Nov 2, 2016

brianmhunt commented Nov 2, 2016

brianmhunt commented Nov 2, 2016

mstarets commented Nov 3, 2016

brianmhunt commented Nov 3, 2016

mstarets commented Nov 3, 2016

mbest commented Nov 3, 2016

brianmhunt commented Nov 4, 2016

mstarets commented Nov 4, 2016

brianmhunt commented Nov 4, 2016

brianmhunt commented Nov 4, 2016

brianmhunt commented Nov 10, 2016

mstarets commented Nov 15, 2016

mstarets commented Dec 9, 2016

brianmhunt commented Dec 9, 2016

mbest commented Dec 9, 2016

brianmhunt commented Dec 10, 2016

mstarets commented Dec 19, 2016

mbest commented Jul 21, 2017

mbest commented Jul 21, 2017 •

edited

Loading

brianmhunt commented Jul 21, 2017 •

edited

Loading

brianmhunt Jul 21, 2017

mbest commented Jul 21, 2017

brianmhunt commented Jul 21, 2017

mbest commented Jul 21, 2017

mbest commented Jul 22, 2017

brianmhunt commented Jul 22, 2017

brianmhunt Jul 22, 2017

brianmhunt Jul 22, 2017

mbest commented Jul 22, 2017

brianmhunt commented Jul 26, 2017

ko.utils.domData storage scheme requires explicit cleanup #2141

ko.utils.domData storage scheme requires explicit cleanup #2141

Conversation

mbest commented Jul 21, 2017 • edited by mstarets Loading

mstarets commented Nov 2, 2016

mstarets commented Nov 2, 2016

brianmhunt commented Nov 2, 2016

brianmhunt commented Nov 2, 2016

mstarets commented Nov 3, 2016

brianmhunt commented Nov 3, 2016

mstarets commented Nov 3, 2016

mbest commented Nov 3, 2016

brianmhunt commented Nov 4, 2016

mstarets commented Nov 4, 2016

brianmhunt commented Nov 4, 2016

brianmhunt commented Nov 4, 2016

brianmhunt commented Nov 10, 2016

mstarets commented Nov 15, 2016

mstarets commented Dec 9, 2016

brianmhunt commented Dec 9, 2016

mbest commented Dec 9, 2016

brianmhunt commented Dec 10, 2016

mstarets commented Dec 19, 2016

mbest commented Jul 21, 2017

mbest commented Jul 21, 2017 • edited Loading

brianmhunt commented Jul 21, 2017 • edited Loading

brianmhunt Jul 21, 2017

Choose a reason for hiding this comment

mbest commented Jul 21, 2017

brianmhunt commented Jul 21, 2017

mbest commented Jul 21, 2017

mbest commented Jul 22, 2017

brianmhunt commented Jul 22, 2017

brianmhunt Jul 22, 2017

Choose a reason for hiding this comment

brianmhunt Jul 22, 2017

Choose a reason for hiding this comment

mbest commented Jul 22, 2017

brianmhunt commented Jul 26, 2017

mbest commented Jul 21, 2017 •

edited by mstarets

Loading

mbest commented Jul 21, 2017 •

edited

Loading

brianmhunt commented Jul 21, 2017 •

edited

Loading