Dialog does not destroy scope/watchers on close

[anton-dev-ua]

Hi

I found next issue in a dialog module.
When dialog is opening it creates new scope. But it does not destroy this scope when it is closing. So amount of scopes (and watchers, etc.) increases with every reopening of the dialog.

I created the Plunker to illustrate the issue: http://plnkr.co/edit/sPhpu9?p=preview

I think adding something like this.$scope.$destroy() at the end of _onCloseComplete() can resolve the issue, if I don't miss something:

Dialog.prototype._onCloseComplete = function(result) {
      this._removeElementsFromDom();
      this._unbindEvents();

      this.deferred.resolve(result);

      this.$scope.$destroy();
    };

Thanks

Regards,
Anton

[pkozlowski-opensource]

Good catch @anton-dev-ua .

In fact we need to still tweak dialog a bit to make it perfect. The reason of the current behavior is that resolve section containing data is not part of the open() call. This is something that was briefly touched upon in:
#170

[anton-dev-ua]

I realised that scope cannot be simply destroyed as it can come from locals (from resolve). But even if scope is passed into the dialog, it creates new watchers in the scope every time. I created one more plunker to demonstrate this: http://plnkr.co/edit/nwbzD4?p=preview

So in case of incoming scope, only watchers should be destroyed. Maybe somthing like this:

Dialog.prototype._onCloseComplete = function(result) {

      ...

      if(this.scopeFromLocals){
        this.$scope.$$watchers = [];
      }else{
        this.$scope.$destroy();
      }
   };

Sorry, if I say obvious things, I am new to angularJS and angularUI, but already love them and want them to be perfect :)

[pkozlowski-opensource]

@anton-dev-ua I really appreciate you looking into this! You are right in your analysis, pretty amazing for a person just starting with AngularJS!

Now, for the problem at hand - it is a bit more complex than this - in fact we should be destroying scopes only if we create them. The fact that the modal directive is passing a scope is a bit unfortunate but we were trying to harmonize 2 code bases.

Would love to fix this asap but won't be able to work on this topic for a couple of next days. @SidhNor would you mind giving a hand to @anton-dev-ua ?

[petebacondarwin]

What you really need here is to always create a new scope. Then you are free to delete it when you are done with it. This is how scopes are designed to work. So on line https://github.com/angular-ui/bootstrap/blob/master/src/dialog/dialog.js#L119, we should be doing something like:

        // Make a copy of locals so that we don't mess up the object passed in
        locals = angular.copy(locals);
        var $scope = locals.$scope = self.$scope = locals.$scope ? locals.$scope.$new() : $rootScope.$new();

Notice that if locals has a scope then we replace it with a child scope: locals.$scope.$new().

Then you can safely call self.$scope.$destroy() in the onClose handler.

[anton-dev-ua]

I have implemented suggestion of @petebacondarwin. I only didn't get why we should copy locals as they are created every time dialog is opening.

[SidhNor]

Hi @anton-dev-ua ,
Because modal reuses dialog service, it gets the modal scope passed in. In short time, the resolves will be passed to each dialog.open call instead of global options, thus we would need each time to handle any possible new values.

Gleb

[anton-dev-ua]

Hi @SidhNor ,
I meant angular.copy(locals) in @petebacondarwin's suggestion. Locals are rebuilt every time on opening to handle new values if any, as you mentioned. So I don't get why we should make angular.copy of locals.

[gamb]

+1 having this same issue with $watch's

[petebacondarwin]

@anton-dev-ua - I guess I was just being safe since this function is modifying the passed in locals object, I didn't want to have any unwanted side-effects. But if you are sure that locals is always generated freshly each time then it is safe just to use the reference.

[anton-dev-ua]

I am not familiar with the code enough, of course, but as I can see locals come from promise returned by _loadResolves(): https://github.com/angular-ui/bootstrap/blob/master/src/dialog/dialog.js#L118
And there in _loadResolves(), locals are created every time: https://github.com/angular-ui/bootstrap/blob/master/src/dialog/dialog.js#L244
So, if I don't miss something, it always creates new instance of locals.

Btw, looks like angular can't copy locals, at least I got error in tests when tried to do this:

RangeError: Maximum call stack size exceeded
at new Boolean ()
at Boolean.toString (native)
at isArray (/Users/anton/Documents/Projects/open-source/angular/ui-bootstrap/misc/test-lib/angular.js:378:19)
at copy (/Users/anton/Documents/Projects/open-source/angular/ui-bootstrap/misc/test-lib/angular.js:564:11)
at copy (/Users/anton/Documents/Projects/open-source/angular/ui-bootstrap/misc/test-lib/angular.js:584:28)
at copy (/Users/anton/Documents/Projects/open-source/angular/ui-bootstrap/misc/test-lib/angular.js:569:23)
at copy (/Users/anton/Documents/Projects/open-source/angular/ui-bootstrap/misc/test-lib/angular.js:584:28)
at copy (/Users/anton/Documents/Projects/open-source/angular/ui-bootstrap/misc/test-lib/angular.js:569:23)
at copy (/Users/anton/Documents/Projects/open-source/angular/ui-bootstrap/misc/test-lib/angular.js:584:28)
at copy (/Users/anton/Documents/Projects/open-source/angular/ui-bootstrap/misc/test-lib/angular.js:569:23)

[antonellopasella]

you can safely call self.$scope.$destroy() in the onClose handler.

This works for me

[pkozlowski-opensource]

It will be handled in #441

[kwehrle]

This issue is still not solved (Version 0.5.0).

I've updated the plnkr from anton_dev_ua: http://plnkr.co/edit/5EuMfH?p=preview

With the modifications from stevebacondarwin and anton-dev-ua it is working as expected.

[pkozlowski-opensource]

@kwehrle The fix (in fact - complete rewrite) will be only part of 0.6.0 so it is normal that it is not fixed in 0.5.0. We should release 0.6.0 over the weekend.