CrNet: add pauseable NSURLProtocol and switch to using it

ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm

Index: ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm
diff --git a/ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm b/ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm
index 0289e81d630f848a358f9dac2a4aae8fb0025f61..60c51323670eb494514216679ad09706a829cac4 100644
--- a/ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm
+++ b/ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm
@@ -36,16 +36,23 @@
   // |requestComplete_| is used in debug to check that the client is not called
   // after completion.
   BOOL _requestComplete;
+  BOOL _paused;
+
+  base::scoped_nsobject<NSMutableArray> _queuedInvocations;
 }
 
 // Performs the selector on |clientThread_| using |runLoopModes_|.
-- (void)performSelectorOnClientThread:(SEL)aSelector withObject:(id)arg;
-// These functions are just wrappers around the corresponding
+- (void)runInvocationQueueOnClientThread;
+- (void)postToClientThread:(SEL)aSelector, ...
+    NS_REQUIRES_NIL_TERMINATION;
+- (void)invokeOnClientThread:(NSInvocation*)invocation;
+// hese functions are just wrappers around the corresponding
 // NSURLProtocolClient methods, used for task posting.
 - (void)didFailWithErrorOnClientThread:(NSError*)error;
 - (void)didLoadDataOnClientThread:(NSData*)data;
 - (void)didReceiveResponseOnClientThread:(NSURLResponse*)response;
-- (void)wasRedirectedToRequestOnClientThread:(NSArray*)params;
+- (void)wasRedirectedToRequestOnClientThread:(NSURLRequest*)request
+                            redirectResponse:(NSURLResponse*)response;
 - (void)didFinishLoadingOnClientThread;
 @end
 
@@ -68,6 +75,7 @@
       _runLoopModes.reset([@[ NSRunLoopCommonModes ] retain]);
     else
       _runLoopModes.reset([@[ mode, NSRunLoopCommonModes ] retain]);
+    _queuedInvocations.reset([[NSMutableArray alloc] init]);
   }
   return self;
 }
@@ -76,16 +84,70 @@
   DCHECK([NSThread currentThread] == _clientThread);
   _protocol = nil;
   _requestComplete = YES;
+  // TODO(ellyjones): This DCHECK fails sometimes. What this implies is that a
+  // callback is coming from the chrome network stack, being enqueued, and then
+  // this proxy is being invalidated before that callback has ever been
+  // delivered. Disturbingly, in practice these seem to be didLoadData and
+  // similar...
+  DCHECK(_queuedInvocations.get().count == 0);

[droger, 2015/06/11 08:56:19]

I think it's expected that the queue may not be empty. For example in the case
where iOS cancels or pauses the request, but the network stack keeps loading
data which enqueues invocations. And then the system destroys the request.

Or did I misunderstood?


I would just do:
_queuedInvocations.reset();
to drop all the pending invocations.

[Elly Fong-Jones, 2015/06/11 13:49:33]

Done.

+}
+
+- (void)runInvocationQueueOnClientThread {
+  DCHECK([NSThread currentThread] == _clientThread);
+  DCHECK(!_requestComplete || !_protocol);
+  while (!_paused && _queuedInvocations.get().count > 0) {

[droger, 2015/06/11 08:56:20]

I don't understand this. Do you expect _paused or the queue to be mutated during
the iteration?

I would have expected a code like:

DCHECK(!_paused);
for (NSInvocation* invocation in _queuedInvocations) {
  [inv invoke];
}
_queuedInvocations.reset();


What did I miss?

[Elly Fong-Jones, 2015/06/11 13:49:33]

Yes, that is the crux of it. A callback can lead the system stack to pause the
request again, after which we cannot keep delivering invocations.
I'll put an explanatory comment.

+    NSInvocation* inv = [_queuedInvocations objectAtIndex:0];
+    // Since |_queuedInvocations| owns the only reference to each queued
+    // invocation, this function has to retain another reference before removing
+    // the queued invocation from the array.
+    [inv retain];
+    [_queuedInvocations removeObjectAtIndex:0];
+    [inv invoke];
+    [inv release];
+  }
 }
 
-- (void)performSelectorOnClientThread:(SEL)aSelector withObject:(id)arg {
-  [self performSelector:aSelector
+- (void)postToClientThread:(SEL)aSelector, ... {
+  // Build an NSInvocation representing an invocation of |aSelector| on |self|
+  // with the supplied varargs passed as arguments to the invocation.
+  NSMethodSignature* sig = [self methodSignatureForSelector:aSelector];
+  DCHECK(sig != nil);
+  NSInvocation* inv = [NSInvocation invocationWithMethodSignature:sig];
+  [inv setTarget:self];
+  [inv setSelector:aSelector];
+  [inv retainArguments];
+
+  size_t arg_index = 2;
+  va_list args;
+  va_start(args, aSelector);
+  NSObject* arg = va_arg(args, NSObject*);
+  while (arg != nil) {
+    [inv setArgument:&arg atIndex:arg_index];
+    arg = va_arg(args, NSObject*);
+    arg_index++;
+  }
+  va_end(args);
+
+  DCHECK(arg_index == sig.numberOfArguments);
+  [inv retain];

[droger, 2015/06/11 08:56:20]

Can we remove this retain and the corresponding releases in
runInvocationQueueOnClientThread and invokeOnClientThread?
I would expect performSelector to do it automatically.

[Elly Fong-Jones, 2015/06/11 13:49:33]

Done.

+  [self performSelector:@selector(invokeOnClientThread:)
                onThread:_clientThread
-             withObject:arg
+             withObject:inv
           waitUntilDone:NO
                   modes:_runLoopModes];
 }
 
+- (void)invokeOnClientThread:(NSInvocation*)invocation {
+  DCHECK([NSThread currentThread] == _clientThread);
+  DCHECK(!_requestComplete || !_protocol);
+  if (!_paused) {
+    [invocation invoke];
+  } else {
+    [_queuedInvocations addObject:invocation];
+  }
+  [invocation release];
+}
+
 #pragma mark Proxy methods called from any thread.
 
 - (void)didFailWithNSErrorCode:(NSInteger)nsErrorCode
@@ -95,25 +157,24 @@
     return;
   NSError* error =
       net::GetIOSError(nsErrorCode, netErrorCode, _url, _creationTime);
-  [self performSelectorOnClientThread:@selector(didFailWithErrorOnClientThread:)
-                           withObject:error];
+  [self postToClientThread:@selector(didFailWithErrorOnClientThread:),
+      error, nil];
 }
 
 - (void)didLoadData:(NSData*)data {
   DCHECK(_clientThread);
   if (!_protocol)
     return;
-  [self performSelectorOnClientThread:@selector(didLoadDataOnClientThread:)
-                           withObject:data];
+  [self postToClientThread:@selector(didLoadDataOnClientThread:),
+      data, nil];
 }
 
 - (void)didReceiveResponse:(NSURLResponse*)response {
   DCHECK(_clientThread);
   if (!_protocol)
     return;
-  [self
-      performSelectorOnClientThread:@selector(didReceiveResponseOnClientThread:)
-                         withObject:response];
+  [self postToClientThread:@selector(didReceiveResponseOnClientThread:),
+      response, nil];
 }
 
 - (void)wasRedirectedToRequest:(NSURLRequest*)request
@@ -122,17 +183,16 @@
   DCHECK(_clientThread);
   if (!_protocol)
     return;
-  [self performSelectorOnClientThread:@selector(
-                                          wasRedirectedToRequestOnClientThread:)
-                           withObject:@[ request, redirectResponse ]];
+  [self postToClientThread:@selector(wasRedirectedToRequestOnClientThread:),
+      request, redirectResponse, nil];
 }
 
 - (void)didFinishLoading {
   DCHECK(_clientThread);
   if (!_protocol)
     return;
-  [self performSelectorOnClientThread:@selector(didFinishLoadingOnClientThread)
-                           withObject:nil];
+  [self postToClientThread:@selector(didFinishLoadingOnClientThread),
+      nil];
 }
 
 // Feature support methods that don't forward to the NSURLProtocolClient.
@@ -160,42 +220,44 @@
 #pragma mark Proxy methods called from the client thread.
 
 - (void)didFailWithErrorOnClientThread:(NSError*)error {
-  DCHECK([NSThread currentThread] == _clientThread);
-  DCHECK(!_requestComplete || !_protocol);
   _requestComplete = YES;
   [[_protocol client] URLProtocol:_protocol didFailWithError:error];
 }
 
 - (void)didLoadDataOnClientThread:(NSData*)data {
-  DCHECK([NSThread currentThread] == _clientThread);
-  DCHECK(!_requestComplete || !_protocol);
   [[_protocol client] URLProtocol:_protocol didLoadData:data];
 }
 
 - (void)didReceiveResponseOnClientThread:(NSURLResponse*)response {
-  DCHECK([NSThread currentThread] == _clientThread);
-  DCHECK(!_requestComplete || !_protocol);
   [[_protocol client] URLProtocol:_protocol
                didReceiveResponse:response
                cacheStoragePolicy:NSURLCacheStorageNotAllowed];
 }
 
-- (void)wasRedirectedToRequestOnClientThread:(NSArray*)params {
-  DCHECK([NSThread currentThread] == _clientThread);
-  DCHECK_EQ(2u, [params count]);
-  DCHECK([params[0] isKindOfClass:[NSURLRequest class]]);
-  DCHECK([params[1] isKindOfClass:[NSURLResponse class]]);
-  DCHECK(!_requestComplete || !_protocol);
+- (void)wasRedirectedToRequestOnClientThread:(NSURLRequest*)request
+                            redirectResponse:(NSURLResponse*)redirectResponse {
   [[_protocol client] URLProtocol:_protocol
-           wasRedirectedToRequest:params[0]
-                 redirectResponse:params[1]];
+           wasRedirectedToRequest:request
+                 redirectResponse:redirectResponse];
 }
 
 - (void)didFinishLoadingOnClientThread {
-  DCHECK([NSThread currentThread] == _clientThread);
-  DCHECK(!_requestComplete || !_protocol);
   _requestComplete = YES;
   [[_protocol client] URLProtocolDidFinishLoading:_protocol];
 }
 
+- (void)pause {
+  DCHECK([NSThread currentThread] == _clientThread);
+  // It's legal (in fact, required) for |pause| to be called after the request
+  // has already finished, so the usual invalidation DCHECK is missing here.
+  _paused = YES;
+}
+
+- (void)resume {
+  DCHECK([NSThread currentThread] == _clientThread);
+  DCHECK(!_requestComplete || !_protocol);
+  _paused = NO;
+  [self runInvocationQueueOnClientThread];
+}
+
 @end