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

ios/net/crn_http_protocol_handler.mm

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import "ios/net/crn_http_protocol_handler.h"
 
 #include <stdint.h>
 
 #include <memory>
 #include <utility>
@@ skipping 32 matching lines @@
 #if !defined(__has_feature) || !__has_feature(objc_arc)
 #error "This file requires ARC support."
 #endif
 
 namespace net {
 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;
 
 // Empty callback.
 void DoNothing(bool flag) {}
 
 }  // namespace
 
 // Bridge class to forward NSStream events to the HttpProtocolHandlerCore.
 // Lives on the IO thread.
@@ skipping 441 matching lines @@
 }
 
 void HttpProtocolHandlerCore::OnReadCompleted(URLRequest* request,
                                               int bytes_read) {
   DCHECK_NE(net::ERR_IO_PENDING, bytes_read);
   DCHECK(thread_checker_.CalledOnValidThread());
 
   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
   // WrappedIOBuffer for example).
   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);
-  }
+    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);
+    }
 
-  if (tracker_)
-    tracker_->CaptureReceivedBytes(request, total_byte_read);
-
-  // 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
                << base::SysNSStringToUTF8([[NSString alloc]
                       initWithData:data
                           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);
   }
 }
 
 HttpProtocolHandlerCore::~HttpProtocolHandlerCore() {
   DCHECK(thread_checker_.CalledOnValidThread());
   [client_ cancelAuthRequest];
   DCHECK(!net_request_);
   DCHECK(!stream_delegate_);
@@ skipping 394 matching lines @@
                  base::Bind(&net::HttpProtocolHandlerCore::Cancel, _core));
   [_protocolProxy invalidate];
 }
 
 - (void)stopLoading {
   [self cancelRequest];
   _protocolProxy.reset();
 }
 
 @end