Titanium JIRA Archive
Titanium SDK/CLI (TIMOB)

[TIMOB-18549] iOS: KrollBidge, reference and memory leak

GitHub Issuen/a
TypeBug
PriorityMedium
StatusClosed
ResolutionInvalid
Resolution Date2015-02-18T21:39:29.000+0000
Affected Version/sRelease 3.5.0
Fix Version/sn/a
ComponentsiOS
LabelsTCSupportTriage, function, ios, ipass1, kroll, leak
ReporterMartin Guillon
AssigneeIngo Muschenetz
Created2014-10-29T14:17:54.000+0000
Updated2017-05-31T22:45:52.000+0000

Description

To make it short it is really easy to create a retain cycle on iOS that will prevent proxies from being deallocated, thus creating huge memory leaks Very simple example: This won't create a retain cycle 
    var win = Ti.UI.createWindow({
        backgroundColor:'red'
    });
    win.addEventListener('click',function() {
        win.close();
    });
    win.open();
This will (example 1) 
    var win = Ti.UI.createWindow({
        backgroundColor:'red'
    });
    testMe = function() {
        win.close();
    }
    win.addEventListener('click',testMe);
    win.open();
Same as this (example 2) 
    var win = Ti.UI.createWindow({
        backgroundColor:'red'
    });
    win.testMe = function() {
        win.close();
    }
    win.addEventListener('click',win.testMe);
    win.open();
You can simply test it by adding a breakpoint in TiWindowProxy dealloc method. You will break in the first example after clicking the window. You won't in the second and third The reason is actually pretty simple: * example1 : the top context holds a reference to testMe testMe's context holds a reference to win. Thus win can't be dealloc, then testMe can't be dealloc .... * example2:win holds a reference to testMe. testMe's context holds a reference to win. => retain cycle Those 2 examples simply a very important retain cycle in the SDK. This should be handled to prevent such things. I have been wrapping my head around this one but it's not easy to fix

Comments

  1. Fokke Zandbergen 2014-10-29

    Is your second example missing the var for testMe on purpose or is that a typo? The third example is discouraged by both docs and community.
  2. Martin Guillon 2014-10-29

    @Fokke: Not on purpose no, but should have the same effect if i am not mistaken. The third example might be discouraged, but sometimes you can't go around it. Using the commonly used "var that = this" way. I think this could be handled by the framework. most browser now handles javascript scope and closure.
  3. Tim Poulsen 2014-10-29

    This is a standard JavaScript variable scoping issue, not a Titanium issue. You're creating a global variable (testMe in example1) that holds a function that references a local object. Garbage collection will never clean up testMe (globals can't be garbage collected). You don't set it to null anywhere. So, it remains valid and since it refers to win, that object must also remain valid throughout the execution of your app. Example 2 likewise sets a listener which refers to a local object, which you don't de-reference later. Thus, it too creates an un-garbage-collectible reference. (Though this one could be cleaned up depending on how that window is opened and closed. But in your simple one-window app it won't be.) I don't see how Titanium can (or should) catch such coding structural issues to fix them for you. You must manage scope, null variables, and remove listeners as appropriate to your code or you will create leaks. (The same is true in a Node.js or web JavaScript environment.)
  4. Martin Guillon 2014-10-29

    @Tim: I guess you are right about the first one. At first it was not part of that ticket, i added after but maybe i shouldn't have. As for the second, you understand that the "addEventListener" has nothing to do with it, right? It s just there to close the window. This would work too 
       var win = Ti.UI.createWindow({
       backgroundColor:'red'
   });
   win.testMe = function() {
       win.close();
   }
   win.addEventListener('click',function() {win.close();}); // this does NOT create a memory leak
   win.open();
    The retain cycle simplycomes from that code 
       win.testMe = function() {
       win.close();
   }
    And i think this one can be handled
  5. Tim Poulsen 2014-10-29

    You can handle the second by removing the event listener, like this: 
       var win = Ti.UI.createWindow({
       backgroundColor:'red'
   });
   var testMe = function() {
       win.removeEventListener('click',testMe);
       win.close();
   }
   win.addEventListener('click',testMe);
   win.open();
  6. Martin Guillon 2014-10-29

    Did you test it? Cause it should not work. addEventListener has nothing to do with the retain cycle. To actually fix it you need to set testMe to null, that way the global context releases testMe, then win can be released.
  7. Vladimir Tolstikov 2015-02-10

    Hey, we have the same issue here with new SDKs (using code near as in "example 2"), but *it definitely was ok in old SDKs, I can prove by video, if needed*. I checked through Xcode Instruments and found that in new SDKs after 3.1.3 (can't say from which one exactly it started, was on 3.1.3 for a long time) that assignment now creates memory leaks. In 3.1.3 and before it performed correctly and windows (and other proxies) have been deallocated properly. Moved back to 3.1.3 because of that issue and need to check and update huge amount of codebase where that handy stuff used before we can switch to latest SDK :(
  8. Vladimir Tolstikov 2015-02-11

    To be clear, my case, which worked in older SDKs, is: 
       function MyWindow()
   {
       var self = Ti.UI.createWindow({
           backgroundColor:'red'
       });
   
       // Assign external props and methods
       self.myMethod = myMethod; // <-- here's memory leak now. Was OK in 3.1.3
       
       return self;
   
       /**
        * Changes background color
        * @public
        */
       function myMethod()
       {
           self.backgroundColor = 'blue';
       }
   }
   
   module.exports = MyWindow;
    That was very handy, simple and easy-to-use pattern we used to write custom UI elements. Now it creates leaks and we will need to change a good amount of code across the app to avoid assigning public "methods" directly to proxies.
  9. Pedro Enrique 2015-02-18

    It has always been a bad practice to add javascript functions to Titanium objects directly. Although a proxy (in this case the window) has a JS object counterpart, this is not a regular JS object. We have some internal hooks to check for properties, methods, etc.. used for the binding to the native classes. That said, the problem is really what Tim mentioned about the circular reference. I did some testing and using "this" instead of the var itself works as expected. This would only work if the only variable that you're trying to use is the one attached. For example:

    Quick workaround

       var self = Ti.UI.createWindow({
       backgroundColor:'red'
   });
   self.myMethod = function() {
       this.backgroundColor = 'blue';
   };
    Now, the proper way to do this in JavaScript would be to create an object that contains those UI components and return that instead of the Window object. Something like this

    Better approach #1

       function MyWindow() {
       var self = {};
       var win = Ti.UI.createWindow();
       // more code...
       self.open = function() {
           win.open();
       };
       self.close = function() {
           win.close();
       };
       self.replaceColor = function(_color) {
           win.backgroundColor = _color;
       };
       return self;
   }
    Or you can always go a bit further an be more object-oriented

    Better approach #2

       // Create this window using "new MyWindow()"
   function MyWindow() {
   	this.win = Ti.UI.createWindow();
   }
   MyWindow.prototype.open = function() {
   	this.win.open();
   };
   MyWindow.prototype.close = function() {
   	this.win.close();
   };
   MyWindow.prototype.replaceColor = function(_color) {
   	this.win.backgroundColor = _color;
   };
  10. Pedro Enrique 2015-02-18

    Resolving this ticket as invalid. The issue presented is a due to the way JavaScript and GC works. Variables declared outside of a function and referenced in that function will be retained as long as that function is available for usage.
  11. Martin Guillon 2015-02-19

    @Pedro: How can you mark that as invalid :D Your examples solve Vladimir cases, but not what i presented. And it has nothing to do with variables declared outside a function. It s related to contexts and mutual retains of variables in context making it impossible to release an object.
  12. Vladimir Tolstikov 2015-02-19

    @Pedro, thanks for answer. Yes, currently we're going to switch on something near "Better approach #1", but I found a common case which needs to be solved in some way. The case is next: - we have list (TableView or ScrollView for example) of custom UI elements - we have one "click" event listener linked to whole list - from a list item clicked: need to get my custom instance from it or need to get additional information Before we used next concept: 
        function onClick(e)
    {
        Ti.API.info(e.source.getMyCustomData()); // e.source -- was my custom instance
    }
    but now, since assigning of functions to proxies creates memory leaks, how we should do that? If we go with "Better approach #1" and no able to assign any custom property to proxies, how could I find the information I need or to get my instance (self) from e.source? I seen that assigning of non-functions to proxies seems not create memory leaks (yet), but if we want to "go clear", that should not be used as well in any form, no? Also, another big disadvantage of new approach: no possibility to listen/fire events. There's no (or I not found) way to inherit your object from TiProxy or something that will provide addEventListener, removeEventListener and fireEvent. Currently, to do that possible, we're creating dummy TiUIView (as "self") which not used in UI, but only provide that events mechanism. Would be very grateful if you can drop some good sample code as answer on questions above. And some words about my initial comment: that would be good if that memory leak will be solved again as it was in old SDKs -- should be more stable for everybody, isn't that a good goal? :)
  13. Tim Poulsen 2015-02-19

    [~farfromrefuge] Please see my comment from 29/Oct/14. You're creating global variables that reference local variables and not cleaning up. As I stated, this is a coding error, not a Titanium issue. You need to clean up (null out) variables and avoid globals. I agree, the discussion later digressed to proxies and so forth. But I think I already addressed your initial concerns.
  14. Martin Guillon 2015-02-19

    Tim Poulsen: It doesnt change anything as i already answered: * i don't want to remove the event listener in "testMe" which means "win" will still hold a reference to testMe * even if i put testMe to null, testMe will still exist (as reference in addEventListener) and thus testMe will still hold a reference to win in its context -> The retain cycle is still there even if i put testMe to null. That ticket is not about how i format the javascript code. But you are right in the sense that the example code was not perfect :D
  15. Ingo Muschenetz 2015-06-23

    [~emerriman] removed fix version.
  16. Lee Morris 2017-05-31

    Closing ticket as invalid.

JSON Source