Cronet: Memory leak in iOS CRN HTTP Protocol handler |
||||||||
Issue descriptionThere is a memory leak in Cronet for iOS. Two objects (CRNPausableHTTPProtocolHandler & CRNHTTPProtocolHandlerWithClientThread) are never deallocated. The objects are created and leaked on every request. The leak occurs due to the strong reference cycle between CRNPausableHTTPProtocolHandler & CRNHTTPProtocolHandlerWithClientThread that is never broken. The cycle is supposed to be broken by calling [CRNHTTPProtocolHandlerProxyWithClientThread invalidate] method; however, it is never called in Cronet case. One of the reasons why the invalidate() method is not called is because Cronet uses CRNPauseableHTTPProtocolHandler instead of CRNHTTPProtocolHandler. CRNPauseableHTTPProtocolHandler doesn't invalidate CRNHTTPProtocolHandlerProxyWithClientThread when stopLoading() is called by CFNetwork, instead it only marks the proxy object as "paused"; thus, preserving the strong reference cycle. One simple solution to fix the the leak is to switch Cronet to use CRNHTTPProtocolHandler; however, I assume that CRNPauseableHTTPProtocolHandler was chosen for a reason. CCing eugenebut@ who implemented CRNPauseableHTTPProtocolHandler and may have some ideas whether the class should be used by Cronet. If CRNPauseableHTTPProtocolHandler is the right handler then, besides fixing the leak, we should implement new Cronet tests that rely on the handler's "pausable" functionality.
,
Apr 17 2017
I looked at the file history, the CRNPauseableHTTPProtocolHandler class was actually created by ellyjones@ here: https://codereview.chromium.org/1142383006.
,
Apr 17 2017
CRNPauseableHTTPProtocolHandler is not used in Chrome code anymore, so this should probably be owned by networking team. Marking Untriaged to let network folks triage this.
,
Apr 17 2017
I will take a look at it.
,
Apr 19 2017
I've chatted with ellyjones@ and CRNPauseableHTTPProtocolHandler has beed used to work around an issue specific to iOS 8. If CRNHTTPProtocolHandler works fine with iOS 9 and above, then we should be able to just remove CRNPauseableHTTPProtocolHandler.
,
Apr 19 2017
Was there a specific scenario that didn't work for iOS 8? We can try to replay it to see if iOS 9/10 has the same problem. I didn't have time to check but I think it may be related to correct handling of the NSURLSessionTask suspend() & resume() methods.
,
Apr 20 2017
I've found related bugs. The original issue has been tracked internally as b/16544410 to expose protocol handlers from CrNet API. I think you are right that CRNPauseableHTTPProtocolHandler is introduced to properly handle suspend() and resume() methods, but unfortunately the original CL didn't seem to have specific tests of that functionality.
,
Apr 28 2017
Found this one, probably related: http://www.openradar.me/19494690
,
May 1 2017
So far I found the following: 1) NSURLProtocol startLoading() & stopLoading() is called only once regardless whether the [dataTask suspend] is called or regardless the number of [NSURLDelegate didReceiveData] invocations. Checked on iOS 9 & 10. Thus, CRNPauseableHTTPProtocolHandler doesn't provide any benefits here. 2) According to the spec, [dataTask suspend] "should produce no network traffic and is not subject to timeouts". I checked that this behavior is implemented by the platform iOS stack. Currently, Cronet doesn't do it. Calling [dataTask suspend] has no effect on the traffic. I filed Issue 717037 to address it.
,
May 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4d000d1bfbbf3965f8c13f968fb6847a9c27c221 commit 4d000d1bfbbf3965f8c13f968fb6847a9c27c221 Author: kapishnikov <kapishnikov@chromium.org> Date: Tue May 02 15:12:55 2017 Cronet iOS: Delete CRNPauseableHTTPProtocolHandler BUG= 711783 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2856693002 Cr-Commit-Position: refs/heads/master@{#468639} [modify] https://crrev.com/4d000d1bfbbf3965f8c13f968fb6847a9c27c221/components/cronet/ios/Cronet.mm [modify] https://crrev.com/4d000d1bfbbf3965f8c13f968fb6847a9c27c221/ios/crnet/crnet_environment.mm [modify] https://crrev.com/4d000d1bfbbf3965f8c13f968fb6847a9c27c221/ios/net/crn_http_protocol_handler.h [modify] https://crrev.com/4d000d1bfbbf3965f8c13f968fb6847a9c27c221/ios/net/crn_http_protocol_handler.mm
,
May 3 2017
|
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by edchin@chromium.org
, Apr 17 2017Owner: eugene...@chromium.org
Status: Assigned (was: Untriaged)