Titanium JIRA Archive
Titanium SDK/CLI (TIMOB)

[TIMOB-24364] iOS: HTTP Client leaks memory during upload using kroll-thread, eventually causing a force close

GitHub Issuen/a
TypeBug
PriorityCritical
StatusClosed
ResolutionFixed
Resolution Date2017-04-12T15:02:06.000+0000
Affected Version/sn/a
Fix Version/sRelease 6.0.4
ComponentsiOS
LabelsIdeagen, kroll-thread, leak
ReporterJoe Finnigan
AssigneeHans Knöchel
Created2017-02-01T15:11:29.000+0000
Updated2017-04-24T13:00:32.000+0000

Description

I have a serious memory leak to report that only happens on uploading of a large amount of images. If you try to upload ~120 large images to a server with xcode instruments running you'll see the memory climb after each upload and eventually crash the app. I've isolated out the area of the code into a sample app that proves the leak is with the HTTP client. This only has 10 images being uploaded, but if you watch the memory usage, it climbs from 20 to 100mb and never releases. If you run the sample app, and have instruments on 'Allocations' and note the total starting memory and then click upload. After the 10 images upload, notice the memory usage. Sample app: https://www.dropbox.com/s/c2gsab0jkot0n9w/Upload%20Leak%20Demo.zip?dl=0

Attachments

FileDateSize
Leak-Fix-Trace.trace.zip2017-03-19T23:18:09.000+00002737451

Comments

  1. Sharif AbuDarda 2017-02-01

    Hello, Can you try with updated SDK 6.0.1.GA?
  2. Hans Knöchel 2017-02-01

    Looks like a valid issue, and since we didn't change something related in the HTTP-handling between 5.5.0 and 6.0.1, I'm ok with moving it for further investigation.
  3. Sharif AbuDarda 2017-02-01

    Hello, I tested your sample app in SDK 6.0.1.GA. I was able to reproduce the crash after 10 attempts of upload. I see this error after crash. Tested in iOS 10.2 simulator.
       [ERROR] Script Error {
       [ERROR]     column = 39;
       [ERROR]     line = 74;
       [ERROR]     message = "undefined is not an object (evaluating 'chunkDataString.t
       oString')";
       [ERROR]     sourceURL = "file:///Users/sharifabudarda/Library/Developer/CoreSimu
       lator/Devices/C0512E31-7C48-4B29-94FA-7F9844B2ABFA/data/Containers/Bundle/Applic
       ation/94D71FF0-DB7F-4FB2-8988-CE63F75ED484/Upload%20Leak%20Demo.app/alloy/contro
       llers/index.js";
       [ERROR]     stack = "upload@file:///Users/sharifabudarda/Library/Developer/CoreS
       imulator/Devices/C0512E31-7C48-4B29-94FA-7F9844B2ABFA/data/Containers/Bundle/App
       lication/94D71FF0-DB7F-4FB2-8988-CE63F75ED484/Upload%20Leak%20Demo.app/alloy/con
       trollers/index.js:74:39\ndoClick@file:///Users/sharifabudarda/Library/Developer/
       CoreSimulator/Devices/C0512E31-7C48-4B29-94FA-7F9844B2ABFA/data/Containers/Bundl
       e/Application/94D71FF0-DB7F-4FB2-8988-CE63F75ED484/Upload%20Leak%20Demo.app/allo
       y/controllers/index.js:12:15";
       [ERROR] } 
       
       Operating System
         Name                        = Mac OS X
         Version                     = 10.12.2
         Architecture                = 64bit
         # CPUs                      = 4
         Memory                      = 8589934592
       
       Node.js
         Node.js Version             = 4.7.0
         npm Version                 = 2.15.11
       
       Titanium CLI
         CLI Version                 = 5.0.11
       
       Titanium SDK
         SDK Version                 = 6.0.1.GA
         SDK Path                    = /Users/sharifabudarda/Library/Application Suppor
       t/Titanium/mobilesdk/osx/6.0.1.GA
         Target Platform             = iphone
       
    The app start with 95mb, I see the memory raise happens on 1-3 attempts, the rest of the time (4-9 attempts) the memory raise was negligible. But after 10 attempts the app crash and here the memory raise was almost 300mb. weirdly memory never releases after each attempts. Thanks.
  4. Joe Finnigan 2017-02-01

    Thanks, not tried it in 6.0.1 as I came to the same conclusion that this is a long standing issue that's not been noticed. Please ignore that error above "undefined is not an object (evaluating 'chunkDataString.t" this is just a lack of error handling in my sample app, not the actual issue. The main issue is the force closure (crash) due to the memory not releasing from the large uploads. I get to about 85 images in our production app, which is about 1GB of memory before force closing on an iPad Air. Also noticed that uploading the same image multiple times doesn't cause the issue, but uploading 100 distinctly unique images causes it to happen faster. I'd appreciate a high priority fix for this, as it is effecting one of our important airline customers (Emirates), and is stopping them from completing an important safety audit.
  5. Hans Knöchel 2017-02-01

    I'll investigate this week and update this ticket, all good! *EDIT*: The leak is caused by the setStringData: method not releasing the memory (string data) again. Need to check the details.
  6. Hans Knöchel 2017-02-01

    Hey [~joef], I don't want to call it a final solution so far, but can you try wrapping [this snippet](https://github.com/appcelerator/titanium_mobile/blob/master/iphone/Classes/TiNetworkHTTPClientProxy.m#L166-L249) with an autorelease-pool and check it again:
       NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
       // Code
       [pool release];
       
    I just tested it locally and the result looks way better and the memory stays around 21 MB. P.S.: You can backup and edit the core files in ~/Library/Application Support/Titanium/mobilesdk/osx/6.0.1.GA/iphone/Classes and clean-build again.
  7. Joe Finnigan 2017-02-08

    Sorry for the delay in replying. We had a quite a few compatibility problems with modules we use upgrading to 6.0. I was finally able to try your fix, however after adding the release pool and cleaning / building i get the following build error: [ERROR] : ** BUILD FAILED ** [ERROR] : The following build commands failed: [ERROR] : CompileC build/Intermediates/Q-Pulse.build/Debug-iphonesimulator/Q-Pulse.build/Objects-normal/x86_64/TiNetworkHTTPClientProxy.o Classes/TiNetworkHTTPClientProxy.m normal x86_64 objective-c com.apple.compilers.llvm.clang.1_0.compiler [ERROR] : (1 failure) I tried upper and lowercase in ReleasePool just encase that was the problem. Any advice? Are there any headers that are required to be added too?
  8. Hans Knöchel 2017-02-08

    I just did a minor other change and attached my Instruments log to the ticket. After uploading 10 images, the app uses 15,93 MB of memory, which seems quite ok :-) I'll do some more tests before submitting a PR, but [this branch](https://github.com/appcelerator/titanium_mobile/compare/master...hansemannn:TIMOB-24364?expand=1) basically contains the changes. I've also checked the code with the Xcode Analyser as well. First we release the pool and then the form reference. [~vijaysingh] thoughts? Also: The HTTPClient also logs the chunks in the debug mode. While this is fine for test-purposes, it won't be done for release builds, so the logs regarding NSLogv leaks are not relevant for this. Check it out! My app.js (a bit modified to catch the last chunk):
       var x = 1;
       
       var win = Ti.UI.createWindow({
           backgroundColor: '#fff'
       });
       
       var btn = Ti.UI.createButton({
           title: 'Trigger'
       });
       
       btn.addEventListener('click', function() {
           upload();
       });
       
       win.add(btn);
       win.open();
       
       
       function upload() {
       	
           var XHR = require("/xhr");
       	var xhr = new XHR();
           var filename = "UploadImage-"+x+".data";
       
       	var failed = function(e){
       		console.log("Fail: " + filename);
       		xhr = null;
       		XHR = null;
       		upload();
       	};
       	var success = function(e){
       		console.log("Success: " + filename);
       		xhr = null;
       		XHR = null;
       		upload();
       	};
       
       	var file = Titanium.Filesystem.getFile(Ti.Filesystem.resourcesDirectory, filename);
       	var blob = file.read();
           
           if (!file.exists()) {
               file = null;
               blob = null;
               
               Ti.API.info('Done!');
               
               return;
           }
           
       	var chunkDataString = Ti.Utils.base64encode(blob);
       	var uploadRequest = {ChunkData:chunkDataString.toString()};
       	var params = { "authenticationToken": "LD8ibszgWIycO62cKn7pq7smtGg6j1xPCC9urwb6V0k=", "uploadRequest": uploadRequest };
       
       	xhr.post("http://52.19.216.46/qpulse5offlineauditserver/AttachmentService.svc/json/PutAttachmentItemChunkWithStringData", JSON.stringify(params), success, failed, null);
           
           file = null;
           blob = null;
           chunkDataString = null;
           
           console.log("Upload: " + filename);
           x++;
       }
       
  9. Vijay Singh 2017-02-09

    [~hansknoechel] Few pointers- 1. Rather using NSAutorelease instance , we should use @autoreleasepool block. As per apple docs, @autoreleasepool block is more efficient. And while moving to ARC, we will not have to change it, as it is available with ARC. 2. If we use NSAutorelease instance then following should be there- if(form != nil) { [httpRequest setPostForm:form]; } [pool release]; RELEASE_TO_NIL(form); Because, if due to any reason, form is nil, pool will not get release. 3. I am suspected that if we use the JSCORE framework, memory leak will be there.( Not due to this change. The leak which we are facing in TIMOB-23868).
  10. Joe Finnigan 2017-02-09

    @Hans Knöchel I've tried updating my HTTP Client to match your branch, however with the sample I'm still seeing it rise from 10mb to 100mb after completion and never releasing. Am I missing something? Copied the TiNetworkHTTPClientProxy.m, cleaned and built the sample.
  11. Hans Knöchel 2017-02-09

    Where did you copy it? And did you use my test-case? I moved some of you null-assignments to a different point to cover the file-lifecycle better.
  12. Joe Finnigan 2017-02-09

    /Library/Application Support/Titanium/mobilesdk/osx/6.0.1.GA/iphone/Classes/TiNetworkHTTPClientProxy.m Replaced that files contents with the one from your branch. And yes, using your test case. https://www.dropbox.com/s/7oqep66ppl1bohp/Screenrecording.mov?dl=0
  13. Joe Finnigan 2017-02-21

    Any updates on this?
  14. Joe Finnigan 2017-03-06

    @hansknoechel Any update on this? Not heard anything back for almost a month now. Our customer Emirates are pushing really hard for a fix on this issue. Can you please give me an update on progress?
  15. Hans Knöchel 2017-03-06

    Hey Joe, we've been busy with the recent 6.0.2 and important other fixes in different areas that are mostly in place now. I will go back to this one this week and we are trying hard to include in in the 6.0.3 release in two weeks.
  16. Hans Knöchel 2017-03-19

    Hey Joe, sorry for the long delay. I was actively working it on in between the 6.0.3 release for this week. And finally, after infinite solutions I tried, I guess I have something to show (I end up with 2,66 MB in memory after the image has been sent, that's great!). The fix is not in the SDK but in the APSHTTPClient library we use are vendor of. PR: https://github.com/appcelerator/APSHTTPClient/pull/39 SDK-PR: Coming as soon as the fix is verified. Test-steps:

    Download my test-project from [here](https://www.dropbox.com/s/b0saoqoqd7vlaaj/test_http.zip?dl=1)

    Run the project on the Simulator

    Track the memory in Instruments by using the "Allocations" tool

    Click "Trigger" and watch the allocations

    After the "Done!" log appears, verify that all send data-chunks are deallocated

    More background: From what I see, the issue was that the data-chunks that were held in a NSMutableData instance were not properly released after the request was finished (deallocated). For [~joef] specifically: You can grab the updated APSHTTPClient library from [here](https://www.dropbox.com/s/ecu6frtnb9xq644/libAPSHTTPClient.a?dl=1) and replace it in ~/Library/Application Support/Titanium/mobilesdk/osx/6.0.2.GA/iphone/Classes/APSHTTPClient/libAPSHTTPClient.a.
  17. Hans Knöchel 2017-03-20

    Hey there, another quick update: I had a detailed meeting with Vijay this morning and we came to the conclusion that, using the patch or not, does not change the behavior. However, we noticed that using my demo-project about does not cause any leaks, but your "Upload Leak Demo" app does. As this issue is the top-prio this whole week, I'd like you to send me your actual Instruments logs to see the exact leaks that climb while uploading. In our test-cases (with both kroll- and main-thread) the memory got released after every successfull upload. Please check it out. And in addition, I noticed that your "xhr.js" tries to persist the response data, which is also a very bad idea. Rather write the file to the filesystem instead of any Ti.App.Properties. We can work close on your project, so let me know what fit's best for you. My e-mail is hknoechel@axway.com.
  18. Joe Finnigan 2017-03-20

    Great Hans, thanks for your effort on this one. I'm just running through it all just now and I'll drop you an email very soon!
  19. Vijay Singh 2017-03-22

    In following PR problem of high memory usage is solved. But it still have one problem that after uploading images it is holding the memory of approx. last image size . But it will not et app crash. https://github.com/appcelerator/titanium_mobile/pull/8902 [~hansknoechel] Can you please verify the same at your end.
  20. Hans Knöchel 2017-03-22

    Thanks Vijay! For the general public: We have worked on this over the last days and are in direct contact with Joe to get this done. Latest findings from Joe (thanks man!) indicate that the issue was only happening when running on kroll-thread. So Joe, please try to patch your TiNetworkHTTPClient with the above proposal and I'll do the same, also focussing on the memory lifecycle when running on kroll-thread. Thanks everyone!
  21. Vijay Singh 2017-03-23

    Verified the app shared by Joe using the changes, as I have made in above PR, I found that in last memory usage is only 3.71MiB(On Kroll_Thread). So this solution is perfect for Kroll_Thread as well .
  22. Hans Knöchel 2017-03-25

    Updated PR with a different solution, based on tracking down the issues to the callback-dispatch of the kroll-proxy: https://github.com/appcelerator/titanium_mobile/pull/8909
  23. Joe Finnigan 2017-03-29

    This updated solution does indeed fix the memory leak, however doing any HTTP GETs that return blob data e.g. images / files etc.. The responseData is always empty. This is a major issue that stops many functions of our app from working.
  24. Hans Knöchel 2017-03-29

    Demo-Code to reproduce that issue:
        var win = Ti.UI.createWindow({
            backgroundColor: '#fff'
        });
        
        var btn = Ti.UI.createButton({
            title: 'Trigger'
        });
        
        btn.addEventListener('click', function() {
            var url = "http://www.appcelerator.com";
            var client = Ti.Network.createHTTPClient({
                // function called when the response data is available
                onload : function(e) {
                    Ti.API.info("Received text: " + this.responseData);
                    alert('success');
                },
                // function called when an error occurs, including a timeout
                onerror : function(e) {
                    Ti.API.debug(e.error);
                    alert('error');
                },
                timeout : 5000  // in milliseconds
            });
            // Prepare the connection.
            client.open("GET", url);
            // Send the request.
            client.send();
        });
        
        win.add(btn);
        win.open();
        
    Special note: Looks like the old callback added all http-client properties to the event. Using client.responseData in the above example will make it work for the patch as well, but we should find out how to attach the whole client again. Or exactly that was the issue and caused the leaks.
  25. Hans Knöchel 2017-03-29

    Updated PR's (to be checked by Joe): - master: https://github.com/appcelerator/titanium_mobile/pull/8918 - 6_1_X: https://github.com/appcelerator/titanium_mobile/pull/8927 - 6_0_X: https://github.com/appcelerator/titanium_mobile/pull/8919 Please test against both of the above test-cases (send 10 files, receive data using a get request) and let me know if I can help setting up the test-environment.
  26. Hans Knöchel 2017-04-05

    Side note: [~emerriman] bumped master to 6.2.0 this week, so I should do a backport for 6_1_X as well. All currently open 6.0.4 PR's need to do the same.

JSON Source