U4-4979 - Client side memory leak for dialogs and not correctly removing their scopes

Created by Sebastiaan Janssen 23 May 2014, 11:38:52 Updated by Sebastiaan Janssen 23 May 2014, 12:28:51

Relates to: U4-4958

The fix for this includes a breaking change for people who created their own dialogs expect their passed in $scope to survive (it will not), we now destroy the scope when the dialog is closing.

The problem was that:

  • Before we were not destroying scopes when we removed the dialog element from DOM so there were extra $scope objects just hanging around in mem everytime a dialog was closed
  • Next, we store each dialog in a dialogs[] array, which when a dialog was closed the instance was not being removed from this array and therefore just hanging about in mem

After these fixes another reported issue: http://issues.umbraco.org/issue/U4-4952 The problem here is that in some cases when opening a dialog a custom $scope object is passed in - since we are now destroying scopes when the dialog closes, this means that the scope passed in is destroyed too

The real underlying problem is that the dialog service has turned out to be real messy. The fact that were passing around scopes and modifying scopes from other services. IMO this isn't an ideal practice, we should not be modifying any scope instances apart from where they get created (i.e. the controller) otherwise we'll end up with stuff like this and things become near impossible to debug. There's currently 3 ways to pass in data to a dialog when only one should exist and worse, all three of these mechanisms are actively in use.

The only reason we have an option to pass in a $scope to the dialogService is if we want to pass in data to the dialog which will be available on it's $scope - what we should do is just pass in a value object - in fact this functionality already exists with the 'dialogData' option when opening a dialog which get's attached to the new $scope object created specifically for the dialog.... so really there is zero reason to need to pass in a $scope to the dialog service. Unfortunately though, we are stuck with this setup because the navigationService ensures that currentNode and currentAction are attached to a custom scope for each dialog and if that is no longer there it is a breaking change.

Then we have a 3rd way to pass in data to a dialog, called 'dialogOptions' which is ultra confusing because in fact dialogData = the dialog instance, which is re-attached back to the scope object that the dialog instance created in the first place (circular reference). That said, this is currently the more predominant way of using custom data in dialogs. Though it does provide extra meta data to the dialog controllers if they need it, I think we should avoid passing in arbitrary option parameters to dialogs this way and instead use dialogData.

Moving forward, no $scope objects should be passed to the dialog service (or really any service) and scope's should not be manipulated outside of where they came from. This was also being done in dialogs which is not required since the $scope of the dialog controller inherits from the $scope created in the dialog service.

var dialogOptions = $scope.$parent.dialogOptions; you can just do: var dialogOptions = $scope.dialogOptions;

The fixes for all this is in 9f16bee040622652ef9f05276ba55f9684b74bb8

Comments

Priority: Normal

Type: Bug

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: False

Fix Submitted:

Affected versions:

Due in version: 7.1.3, 7.1.4

Sprint:

Story Points:

Cycle: