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..185e28b458638a5946c2fa4fd321a945938d5ca7 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,9 +36,14 @@
   // |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)runInvocationQueueOnClientThread;
+- (void)enqueueInvocationOnClientThread:(NSInvocation*)invocation;
 - (void)performSelectorOnClientThread:(SEL)aSelector withObject:(id)arg;
 // These functions are just wrappers around the corresponding
 // NSURLProtocolClient methods, used for task posting.
@@ -68,6 +73,8 @@
       _runLoopModes.reset([@[ NSRunLoopCommonModes ] retain]);
     else
       _runLoopModes.reset([@[ mode, NSRunLoopCommonModes ] retain]);
+    _paused = NO;

[droger, 2015/05/27 17:09:00]

Nit: in objective-C, instance variables are auto-zeroed.
The usual style is to not re-initialize them to zero.

[Elly Fong-Jones, 2015/06/10 17:06:35]

Done.

+    _queuedInvocations.reset([[NSMutableArray alloc] init]);
   }
   return self;
 }
@@ -76,14 +83,77 @@
   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/05/27 17:09:01]

This might be caused by the problem with |_paused| below.

[Elly Fong-Jones, 2015/06/10 17:06:35]

Done.

+}
+
+- (void)enqueueInvocationOnClientThread:(NSInvocation*)inv {
+  DCHECK([NSThread currentThread] == _clientThread);
+  DCHECK(!_requestComplete || !_protocol);
+  [_queuedInvocations addObject:inv];
+  // This function receives these two references from
+  // |performSelectorOnClientThread:| below.
+  [inv release];
+  [self release];
+}
+
+- (void)runInvocationQueueOnClientThread {
+  DCHECK([NSThread currentThread] == _clientThread);
+  DCHECK(!_requestComplete || !_protocol);
+  while (!_paused && _queuedInvocations.get().count > 0) {
+    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
-               onThread:_clientThread
-             withObject:arg
-          waitUntilDone:NO
-                  modes:_runLoopModes];
+  if (!_paused) {

[droger, 2015/05/27 17:09:00]

I don't think it is safe to access _paused here.

I don't know what is the best way to do it, but I think we should be able to do
it without locks, by catching the tasks on the other thread, instead of catching
them before posting them.

[Elly Fong-Jones, 2015/06/10 17:06:35]

Done.

+    [self performSelector:aSelector
+                 onThread:_clientThread
+               withObject:arg
+            waitUntilDone:NO
+                    modes:_runLoopModes];
+  } else {
+    // The memory management for this case is tricky. The |inv| NSInvocation
+    // object created here is autoreleased, so it would be destroyed by the
+    // autorelease pool if this function did not retain another reference to it.
+    // That reference is given to |enqueueInvocationOnClientThread:| via
+    // |performSelector:|, and |enqueueInvocationOnClientThread:| is responsible
+    // for releasing it.
+    //
+    // This function also needs an extra reference to |self|. Otherwise, this
+    // object can be destroyed by the time the |enqueueInvocationOnClientThread|
+    // function runs, with disastrous results. |enqueueInvocationOnClientThread|
+    // is responsible for releasing this extra reference as well; it receives
+    // ownership of it through |performSelector|.
+    NSMethodSignature* sig = [self methodSignatureForSelector:aSelector];
+    DCHECK(sig != nil);
+    NSInvocation* inv = [NSInvocation invocationWithMethodSignature:sig];
+    [inv setTarget:self];
+    if (sig.numberOfArguments > 2) {
+      [inv setArgument:&arg atIndex:2];
+    }
+    [inv setSelector:aSelector];
+    [inv retainArguments];
+    [inv retain];
+
+    [self retain];
+    [self performSelector:@selector(enqueueInvocationOnClientThread:)
+                 onThread:_clientThread
+               withObject:inv
+            waitUntilDone:NO
+                    modes:_runLoopModes];
+  }
 }
 
 #pragma mark Proxy methods called from any thread.
@@ -198,4 +268,13 @@
   [[_protocol client] URLProtocolDidFinishLoading:_protocol];
 }
 
+- (void)pause {
+  _paused = YES;

[droger, 2015/05/27 17:09:01]

We probably need thread checks here
DCHECK([NSThread currentThread] == _clientThread);

|_paused| should only be accessed from one thread.

[Elly Fong-Jones, 2015/06/10 17:06:35]

Done.

+}
+
+- (void)resume {
+  _paused = NO;

[droger, 2015/05/27 17:09:01]

Thread checks.

[Elly Fong-Jones, 2015/06/10 17:06:35]

Done.

+  [self runInvocationQueueOnClientThread];
+}
+
 @end