Manually created $scope should $destroy on end editing, or $watchers will leak

[S-YOU]

[DiegoZoracKy]

Came here to open this same issue. Was reported here on #108 too. Any news about that?

[omarw]

What Chrome extension is that in your screenshot?

[S-YOU]

angular watchers - https://chrome.google.com/webstore/detail/angular-watchers/nlmjblobloedpmkmmckeehnbfalnjnjk?hl=en

[NickAb]

Is there are any workaround or a quick fix to this?

[NickAb]

I made a temporary fix setting onhide to (typescript code)

function () {
            var last = this.scope.$$watchers.pop();
            this.scope.$$watchers = [last];
        };

But i am new to angular and not sure if it is a correct way that will free memory, etc.

Also, based on code, I think there are might be a problems not only with watchers, but with binds too. Directive bind to some events, but never class unbind.

[kenfdev]

I think this leak would be fixed with these commits.

Fix scope memory leak 46b71a7cadc169093a8464261b9c5545b87b0155 Change $data on scope parent to propagate change 7e73a3b9d459cab3960455bede46af7c3f043e71

But the pull request (#158) is mixed with many other fixes and is not merged.

These changes at least worked for my situation.

First, change the self.render function.

// src/js/editable-element/controller.js
self.render = function() {
...
      self.inputEl.addClass('editable-input');
      self.inputEl.attr('ng-model', '$data'); // <- change this to self.inputEl.attr('ng-model', '$parent.$data');

      // add directiveName class to editor, e.g. `editable-text`
      self.editorEl.addClass(editableUtils.camelToDash(self.directiveName));
...

and then change the self.show function.

// src/js/editable-element/controller.js
     //show
    self.show = function() {
...

      // compile (needed to attach ng-* events from markup)
      var newScope = $scope.$new(); // <- add this line
      $compile(self.editorEl)(newScope);
...
    };

and finally destroy the created scope inside self.hide

// src/js/editable-element/controller.js
    //hide
    self.hide = function() {

      // destroy the scope to prevent memory leak
      self.editorEl.scope().$destroy(); // <- add this line

      self.controlsEl.remove();
      self.editorEl.remove();
      $element.removeClass('editable-hide');
...
    };

I haven't checked the side effects but it seems to be working. If anybody can confirm this change, I'll create a pull request.

Updated(2016/06/20) Struck out my comment. The above approach does not work.

[jawadatgithub]

Dear participants,

I have deadline after 10 hours from now, I need this leak bug fixed and released, I have part of the SPA that uses form with ~400 xeditable fields (that's ~ 2000 Watches everytime user click Edit), please update ASAP, otherwise I will be forced to make custom changes in my local version of the component which I prefer not to happen.

thanks for the awesome component.

[eugef]

@jawadatgithub You are more than welcome to do changes in your fork and submit the PR

[kenfdev]

The modification I made above does not work. It is referenced in issue #265 and the fix has been reverted in this commit (1fcdd97ab886b67fee7e47cfe7d0a7e2da9cc6da) . It's probably because creating a new scope with $scope.new() creates an additional layer and access from directives like editableRadioList have to be modified to ng-model="$parent.$parent.$data" . This modification has pretty much a big impact and I'm not sure if it's even the right approach. Does anybody have any ideas on how we can fix this??

For example, I'm thinking that we have to change all the directives to reference another $parent above to access the right $data when we create a new $scope.

// radiolist
angular.module('xeditable').directive('editableRadiolist', [
  'editableDirectiveFactory',
  'editableNgOptionsParser',
  function(editableDirectiveFactory, editableNgOptionsParser) {
    return editableDirectiveFactory({
      directiveName: 'editableRadiolist',
      inputTpl: '<span></span>',
      render: function() {
        this.parent.render.call(this);
        var parsed = editableNgOptionsParser(this.attrs.eNgOptions);
        // changed the `ng-model` below to `$parent.$parent.$data`
        var html = '<label ng-repeat="'+parsed.ngRepeat+'">'+
          '<input type="radio" ng-disabled="' + this.attrs.eNgDisabled + '" ng-model="$parent.$parent.$data" value="{{'+parsed.locals.valueFn+'}}">'+
          '<span ng-bind="'+parsed.locals.displayFn+'"></span></label>';

        this.inputEl.removeAttr('ng-model');
        this.inputEl.removeAttr('ng-options');
        this.inputEl.html(html);
      },
      autosubmit: function() {
        var self = this;
        self.inputEl.bind('change', function() {
          setTimeout(function() {
            self.scope.$apply(function() {
              self.scope.$form.$submit();
            });
          }, 500);
        });
      }
    });
}]);

Am I on the right road or am I missing something??

Thanks in advance.

[ckosloski]

@kenfdev have you tried building the latest code? I have put in some changes that reduced the memory leak that haven't been released yet. You could see if that helps you out.

[kenfdev]

@ckosloski thx for the reply. Yes, I've built the latest with grunt build and I believe the code you're mentioning is as below.

      // Manually remove the watcher on 'has-error' to prevent a memory leak on it.
      for (var i = 0, len = $scope.$$watchers.length; i < len; i++) {
        if ($scope.$$watchers[i] !== undefined && $scope.$$watchers[i].last && $scope.$$watchers[i].last !== undefined &&
            typeof $scope.$$watchers[i].last === 'object' && "has-error" in $scope.$$watchers[i].last) {
          $scope.$$watchers.splice(i, 1);
          break;
        }
      }

This indeed reduces a few $$watchers but not all. For example, the view used in my project creates about 400 $$watchers and ending editable mode only reduces about 40 of them. Creating a new scope and destroying the scope reduces the same amount of $$watchers created.

[kenfdev]

@ckosloski This is an example of the $$watchers leaking in the newest build .

The code is as below.

<div ng-app="app" ng-controller="RadiolistCtrl" style="padding: 50px;">
  <div>
    <a href="#" editable-radiolist="user.status" e-ng-options="s.value as s.text for s in statuses">
      user status: {{ user.status }}
    </a>
  </div>

  <div>
    <a href="#" editable-radiolist="user.status2" e-ng-options="s.value as s.text for s in statuses">
      user status2: {{ user.status2 }}
    </a>
  </div>

</div>
<script>
  var app = angular.module("app", ["xeditable"]);

  app.run(function (editableOptions) {
    editableOptions.theme = 'bs3';
  });

  app.controller('RadiolistCtrl', function ($scope, $filter) {
    $scope.user = {
      status: 2,
      status2: 1
    };

    $scope.statuses = [{
      value: 1,
      text: 'status1'
    }, {
      value: 2,
      text: 'status2'
    }];
  });
</script>

[kenfdev]

I've created a PR #489 . I hope this problem gets a step closer to being solved.

[jawadatgithub]

Another bug might be caused by this bug is $data content keep reference of previous edit items:

i.e., html: <form editable-form name="editableForm" onshow="pauseVideo()" onbeforesave="beforeSavingEditedText($data)"> <span ng-repeat="word in tsWordsArrayForEdit track by $index" e-style="width: 110px;height: inherit;" e-name="{{$index}}" editable-text="word"></span> ......</form> js:

scope.beforeSavingEditedText = function(data) { //data object contains items of previous scope.tsWordsArrayForEdit array items. };

one first edit: if scope.tsWordsArrayForEdit.length = 100 and on second edit, scope.tsWordsArrayForEdit.length = 80

then after form submit $data return 100 items, in which last 20 items came from scope.tsWordsArrayForEdit old content! in below image, every item after 318, is coming from previous content!

I think this should make this opened issue get higher priority because it highlight data corruption not only performance issue.

meanwhile, is there a way to destroy the form and recreate it on each edit? can you share an example? or in case there is better workaround till this get fixed please share?

[kenfdev]

@jawadatgithub Maybe I can look at the problem. Could you create a plunkr??? I'll try and reproduce it. I have a feeling that this should be a different issue though.

[ckosloski]

Is this issue fixed now that #489 has been merged?

[ckosloski]

@eugef this can be closed, it was fixed by #489