CrNet: add pauseable NSURLProtocol and switch to using it

ios/net/crn_http_protocol_handler_proxy_with_client_thread.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_proxy_with_client_thread.h"
 
 #include "base/logging.h"
 #import "base/mac/scoped_nsobject.h"
 #include "base/time/time.h"
 #import "ios/net/protocol_handler_util.h"
@@ skipping 18 matching lines @@
   __weak NSThread* _clientThread;
   // The run loop modes to use when posting tasks to |clientThread_|.
   base::scoped_nsobject<NSArray> _runLoopModes;
   // The request URL.
   base::scoped_nsobject<NSString> _url;
   // The creation time of the request.
   base::Time _creationTime;
   // |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
 
 @implementation CRNHTTPProtocolHandlerProxyWithClientThread
 
 - (instancetype)initWithProtocol:(NSURLProtocol*)protocol
                     clientThread:(NSThread*)clientThread
                      runLoopMode:(NSString*)mode {
   DCHECK(protocol);
   DCHECK(clientThread);
   if ((self = [super init])) {
     _protocol = protocol;
     _url.reset([[[[protocol request] URL] absoluteString] copy]);
     _creationTime = base::Time::Now();
     _clientThread = clientThread;
     // Use the common run loop mode in addition to the client thread mode, in
     // hope that our tasks are executed even if the client thread changes mode
     // later on.
     if ([mode isEqualToString:NSRunLoopCommonModes])
       _runLoopModes.reset([@[ NSRunLoopCommonModes ] retain]);
     else
       _runLoopModes.reset([@[ mode, NSRunLoopCommonModes ] retain]);
+    _queuedInvocations.reset([[NSMutableArray alloc] init]);
   }
   return self;
 }
 
 - (void)invalidate {
   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)performSelectorOnClientThread:(SEL)aSelector withObject:(id)arg {
-  [self performSelector:aSelector
+- (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)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
                   netErrorCode:(int)netErrorCode {
   DCHECK(_clientThread);
   if (!_protocol)

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

Optional:

I realize that this check is not trivial.
Not related to your CL but maybe add a comment like:
Test |_protocol| from the wrong thread to possibly avoid posting the task. It
will be checked from the right thread later.

     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
                  nativeRequest:(net::URLRequest*)nativeRequest
               redirectResponse:(NSURLResponse*)redirectResponse {
   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.
 - (void)didCreateNativeRequest:(net::URLRequest*)nativeRequest {
   // no-op.
 }
 
 - (void)didRecieveAuthChallenge:(net::AuthChallengeInfo*)authInfo
                   nativeRequest:(const net::URLRequest&)nativeRequest
                        callback:(const network_client::AuthCallback&)callback {
   // If we get this far, authentication has failed.
   base::string16 empty;
   callback.Run(false, empty, empty);
 }
 
 - (void)cancelAuthRequest {
   // no-op.
 }
 
 - (void)setUnderlyingClient:(id<CRNNetworkClientProtocol>)underlyingClient {
   // This is the lowest level.
   DCHECK(!underlyingClient);
 }
 
 #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