iOS Cronet: Put cap on the buffer size passed to onReadCompleted delegate

ios/net/crn_http_protocol_handler.mm

Index: ios/net/crn_http_protocol_handler.mm
diff --git a/ios/net/crn_http_protocol_handler.mm b/ios/net/crn_http_protocol_handler.mm
index 03ac857be8853089e1c3bf2ab051fbbcac41d799..4e43ae35bd0b952e511b0a42afa4639c08d04a7e 100644
--- a/ios/net/crn_http_protocol_handler.mm
+++ b/ios/net/crn_http_protocol_handler.mm
@@ -50,9 +50,14 @@ class HttpProtocolHandlerCore;
 
 namespace {
 
-// Size of the buffer used to read the net::URLRequest.
+// Size of the buffer used to read the net::URLRequest. This value must always
+// be less or equal to |kClientMaxBufferSize|.
 const int kIOBufferSize = 64 * 1024;
 
+// The maximum size of NSData that can be passed to the client 'didReceiveData'
+// callback. This value must always be greater or equal to |kIOBufferSize|.
+const int kClientMaxBufferSize = 256 * 1024;
+
 // Global instance of the HTTPProtocolHandlerDelegate.
 net::HTTPProtocolHandlerDelegate* g_protocol_handler_delegate = nullptr;
 
@@ -514,7 +519,7 @@ void HttpProtocolHandlerCore::OnReadCompleted(URLRequest* request,
   if (net_request_ == nullptr)
     return;
 
-  base::scoped_nsobject<NSMutableData> data([[NSMutableData alloc] init]);
+  DCHECK_EQ(net_request_, request);

[lilyhoughton, 2017/05/05 16:33:05]

Why does OnReadCompleted take request as an argument at all?

[mef, 2017/05/05 17:08:19]

It is implementing UrlRequest::Delegate, which allows single delegate to handle
multiple requests.

 
   // Read all we can from the socket and put it into data.
   // TODO(droger): It may be possible to avoid some of the copies (using
@@ -522,19 +527,21 @@ void HttpProtocolHandlerCore::OnReadCompleted(URLRequest* request,
   NSUInteger data_length;
   uint64_t total_byte_read = 0;

[mef, 2017/05/05 17:08:19]

nit: rename to total_bytes_read for consistency.

[kapishnikov, 2017/05/05 19:13:42]

Done.

   while (bytes_read > 0) {
-    total_byte_read += bytes_read;
-    data_length = [data length];  // Assumes that getting the length is fast.
-    [data increaseLengthBy:bytes_read];
-    memcpy(reinterpret_cast<char*>([data mutableBytes]) + data_length,
-           buffer_->data(), bytes_read);
-    bytes_read = request->Read(buffer_.get(), kIOBufferSize);
-  }
-
-  if (tracker_)
-    tracker_->CaptureReceivedBytes(request, total_byte_read);
+    base::scoped_nsobject<NSMutableData> data(
+        [[NSMutableData alloc] initWithCapacity:bytes_read]);
+    // |bytes_read| should always be less or equal to |kClientMaxBufferSize|.
+    // This is ensured by the fact that the max read buffer size (i.e.
+    // |kIOBufferSize|) is always smaller or equal to |kClientMaxBufferSize|.
+    while (bytes_read > 0 &&
+           [data length] + bytes_read <= kClientMaxBufferSize) {
+      total_byte_read += bytes_read;
+      data_length = [data length];

[mef, 2017/05/05 17:08:19]

move NSUInteger data_length declaration here to limit scope or just inline it
below?

[kapishnikov, 2017/05/05 19:13:42]

Done.

+      [data increaseLengthBy:bytes_read];
+      memcpy(reinterpret_cast<char*>([data mutableBytes]) + data_length,
+             buffer_->data(), bytes_read);
+      bytes_read = request->Read(buffer_.get(), kIOBufferSize);
+    }
 
-  // Notify the client.
-  if (bytes_read == net::OK || bytes_read == net::ERR_IO_PENDING) {
     if ([data length] > 0) {
       // If the data is not encoded in UTF8, the NSString is nil.
       DVLOG(3) << "To client:" << std::endl
@@ -543,14 +550,19 @@ void HttpProtocolHandlerCore::OnReadCompleted(URLRequest* request,
                           encoding:NSUTF8StringEncoding]);
       [client_ didLoadData:data];
     }
-    if (bytes_read == 0) {
-      DCHECK_EQ(net_request_, request);
-      // There is nothing more to read.
-      StopNetRequest();
-      [client_ didFinishLoading];
-    }
-  } else {
-    // Request failed (not canceled).
+  }
+
+  if (tracker_)
+    tracker_->CaptureReceivedBytes(request, total_byte_read);
+
+  // If there is nothing more to read.
+  if (bytes_read == 0) {

[lilyhoughton, 2017/05/05 16:33:05]

Does it make sense for this to be |if (bytes_read == net::OK)|?

[kapishnikov, 2017/05/05 19:13:42]

Changed it.

+    StopNetRequest();
+    [client_ didFinishLoading];
+  }
+
+  // If there was an error (not canceled).
+  if (bytes_read != net::OK && bytes_read != net::ERR_IO_PENDING) {

[lilyhoughton, 2017/05/05 16:33:05]

I think comparing bytes_read to both 0 and net::OK makes the control flow a
little confusing.

And since (bytes_read == 0) and (bytes_read != net::OK) are mutually exclusive
anyway, does it make more sense to have this be
    else if (bytes_read != net::ERR_IO_PENDING)
?

[kapishnikov, 2017/05/05 19:13:42]

Done.

     int error = bytes_read;
     StopRequestWithError(IOSErrorCode(error), error);
   }