Fixes metrics crash when a NSURLSession is created without a delegate

BUG=834401

Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I431f280cb95db8b924a826740590eeb8fb1c4399
Reviewed-on: https://chromium-review.googlesource.com/1021600Reviewed-by: Misha Efimov <mef@chromium.org>
Commit-Queue: Andrei Kapishnikov <kapishnikov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552853}

components/cronet/ios/Cronet.mm

@@ -535,6 +535,12 @@ class CronetHttpProtocolHandlerDelegate
                          userInfo:userInfo];
 }
 
+// Used by tests to query the size of the map that contains metrics for
+// individual NSURLSession tasks.
++ (size_t)getMetricsMapSize {
+  return cronet::CronetMetricsDelegate::GetMetricsMapSize();
+}
+
 // Static class initializer.
 + (void)initialize {
   gChromeNet.Get().reset();

components/cronet/ios/cronet_metrics.h

@@ -96,6 +96,10 @@ class CronetMetricsDelegate : public net::MetricsDelegate {
   // the client. If there is no metrics data for the passed task, this returns
   // nullptr.
   static std::unique_ptr<Metrics> MetricsForTask(NSURLSessionTask* task);
+
+  // Used by tests to query the size of the |task_metrics_map_| map.
+  static size_t GetMetricsMapSize();
+
  private:
   static NSObject* task_metrics_map_lock_;
   static std::map<NSURLSessionTask*, std::unique_ptr<Metrics>>

components/cronet/ios/cronet_metrics.mm

@@ -176,6 +176,15 @@ CronetTransactionMetrics* NativeToIOSMetrics(Metrics& metrics)
 
 }  // namespace
 
+// A blank implementation of NSURLSessionDelegate that contains no methods.
+// It is used as a substitution for a session delegate when the client
+// either creates a session without a delegate or passes 'nil' as its value.
+@interface BlankNSURLSessionDelegate : NSObject<NSURLSessionDelegate>
+@end
+
+@implementation BlankNSURLSessionDelegate : NSObject
+@end
+
 // In order for Cronet to use the iOS metrics collection API, it needs to
 // replace the normal NSURLSession mechanism for calling into the delegate
 // (so it can provide metrics from net/, instead of the empty metrics that iOS
@@ -199,7 +208,15 @@ CronetTransactionMetrics* NativeToIOSMetrics(Metrics& metrics)
 // As this is a proxy delegate, it needs to be initialized with a real client
 // delegate, to whom all of the method invocations will eventually get passed.
 - (instancetype)initWithDelegate:(id<NSURLSessionDelegate>)delegate {
-  _delegate = delegate;
+  // If the client passed a real delegate, use it. Otherwise, create a blank
+  // delegate that will handle method invocations that are forwarded by this
+  // proxy implementation. It is incorrect to forward calls to a 'nil' object.
+  if (delegate) {
+    _delegate = delegate;
+  } else {
+    _delegate = [[BlankNSURLSessionDelegate alloc] init];
+  }
+
   _respondsToDidFinishCollectingMetrics =
       [_delegate respondsToSelector:@selector
                  (URLSession:task:didFinishCollectingMetrics:)];
@@ -251,6 +268,17 @@ CronetTransactionMetrics* NativeToIOSMetrics(Metrics& metrics)
   }
 }
 
+- (BOOL)respondsToSelector:(SEL)aSelector {
+  // Regardless whether the underlying session delegate handles
+  // URLSession:task:didFinishCollectingMetrics: or not, always
+  // return 'YES' for that selector. Otherwise, the method may
+  // not be called, causing unbounded growth of |task_metrics_map_|.
+  if (aSelector == @selector(URLSession:task:didFinishCollectingMetrics:)) {
+    return YES;
+  }
+  return [_delegate respondsToSelector:aSelector];
+}
+
 @end
 
 @implementation NSURLSession (Cronet)
@@ -315,6 +343,12 @@ void CronetMetricsDelegate::OnStopNetRequest(std::unique_ptr<Metrics> metrics) {
   }
 }
 
+size_t CronetMetricsDelegate::GetMetricsMapSize() {
+  @synchronized(task_metrics_map_lock_) {
+    return task_metrics_map_.size();
+  }
+}
+
 #pragma mark - Swizzle
 
 void SwizzleSessionWithConfiguration() {

components/cronet/ios/test/BUILD.gn

@@ -16,6 +16,7 @@ test("cronet_test") {
     "cronet_pkp_test.mm",
     "cronet_prefs_test.mm",
     "cronet_quic_test.mm",
+    "cronet_test_base.h",
     "cronet_test_base.mm",
     "cronet_test_runner.mm",
     "get_stream_engine.mm",

components/cronet/ios/test/cronet_metrics_test.mm

@@ -319,6 +319,50 @@ TEST_F(CronetEnabledMetricsTest, ReusedConnection) {
   }
 }
 
+// Checks that there is no crash if the session delegate is not set when a
+// NSURLSession is created. Also checks that the internal metrics map is cleaned
+// and contains 0 records at the end of the request. This is a regression test
+// for http://crbug/834401.
+TEST_F(CronetEnabledMetricsTest, SessionWithoutDelegate) {
+  if (@available(iOS 10, *)) {
+    NSURLSessionConfiguration* default_config =
+        [NSURLSessionConfiguration defaultSessionConfiguration];
+    [Cronet installIntoSessionConfiguration:default_config];
+    NSURLSession* default_session =
+        [NSURLSession sessionWithConfiguration:default_config];
+    NSURL* url = net::NSURLWithGURL(net::QuicSimpleTestServer::GetSimpleURL());
+    NSURLRequest* request = [NSURLRequest requestWithURL:url];
+
+    __block BOOL no_error = NO;
+    dispatch_semaphore_t semaphore = dispatch_semaphore_create(0);
+    NSURLSessionDataTask* task = [default_session
+        dataTaskWithRequest:request
+          completionHandler:^(NSData* data, NSURLResponse* response,
+                              NSError* error) {
+            EXPECT_TRUE(error == nil)
+                << base::SysNSStringToUTF8([error description]);
+            no_error = YES;
+            dispatch_semaphore_signal(semaphore);
+          }];
+    __block BOOL block_used = NO;
+    [Cronet setRequestFilterBlock:^(NSURLRequest* request) {
+      block_used = YES;
+      EXPECT_EQ(request.URL, url);
+      return YES;
+    }];
+
+    [task resume];
+    long wait_result = dispatch_semaphore_wait(
+        semaphore, dispatch_time(DISPATCH_TIME_NOW, 10 * NSEC_PER_SEC));
+
+    // Check results
+    EXPECT_EQ(0, wait_result);
+    EXPECT_TRUE(block_used);
+    EXPECT_TRUE(no_error);
+    EXPECT_EQ(0UL, [Cronet getMetricsMapSize]);
+  }
+}
+
 // Tests that the metrics disable switch works.
 TEST_F(CronetDisabledMetricsTest, MetricsDisabled) {
   if (@available(iOS 10, *)) {

components/cronet/ios/test/cronet_test_base.h

@@ -32,6 +32,7 @@ typedef void (^BlockType)(void);
 + (void)setEnablePublicKeyPinningBypassForLocalTrustAnchors:(BOOL)enable;
 + (base::SingleThreadTaskRunner*)getFileThreadRunnerForTesting;
 + (base::SingleThreadTaskRunner*)getNetworkThreadRunnerForTesting;
++ (size_t)getMetricsMapSize;
 @end
 
 // NSURLSessionDataDelegate delegate implementation used by the tests to