Adding the Copresence RpcHandler and HttpPost helper.

components/copresence/copresence_client.cc

Index: components/copresence/copresence_client.cc
diff --git a/components/copresence/copresence_client.cc b/components/copresence/copresence_client.cc
index 60e45e15d2156368eddf0e575f1812e6339e9dc6..17f906b59f3516e8cddd2bd9cf328cf7c77e5477 100644
--- a/components/copresence/copresence_client.cc
+++ b/components/copresence/copresence_client.cc
@@ -26,26 +26,22 @@ CopresenceClient::CopresenceClient(CopresenceClientDelegate* delegate)
     : delegate_(delegate), init_failed_(false), pending_init_operations_(0) {
   DVLOG(3) << "Initializing client.";
   pending_init_operations_++;
-  rpc_handler_.reset(
-      new RpcHandler(delegate,
-                     base::Bind(&CopresenceClient::InitStepComplete,
-                                AsWeakPtr(),
-                                "Copresence device registration")));
+  rpc_handler_.reset(new RpcHandler(delegate));
+  // We own the RpcHandler, so it won't outlive us.
+  rpc_handler_->Initialize(base::Bind(&CopresenceClient::InitStepComplete,
+                                      base::Unretained(this),
+                                      "Copresence device registration"));
 
   pending_init_operations_++;
   delegate_->GetWhispernetClient()->Initialize(
       base::Bind(&CopresenceClient::InitStepComplete,
-                 AsWeakPtr(),
+                 // The WhispernetClient must discard this on Shutdown.

[willchan no longer on Chromium, 2014/08/08 01:05:58]

I like that you got rid of the AsWeakPtr(). It's interesting to see that the
WhispernetClient must discard this on Shutdown. Is this always true? Do they
always have such a tight coupling? If so, maybe it's best to expose a single
public API that encapsulates both of them, rather than let the embedder screw
this up. What do you think?

[rkc, 2014/08/08 01:29:07]

So this is actually a bug. If the whispernet client takes longer to initialize
than the lifetime of CopresenceClient (for example, in case the whispernet init
is stalled during which we are done with the CopresenceClient and destruct it),
this will be a user-after-free.

I actually thought we were getting rid of the weak pointer only in the RPC
handler, I didn't realize we removed them from here too. I believe a WeakPtr is
a better solution here than imposing lifetime requirements on the Whispernet
client.

This is one of the few cases that a WeakPtr is actually good for (when you are
given a object that you do not have the control on the lifetime of). I would
recommend that this code keep using WeakPtrs. This 'can' be done without it by
specifically adding code to the whispernet client to abort an initialization but
the resulting code will be uglier than simply using a WeakPtr here.


(btw, I am not completely sure what you mean by a single public API? If you're
talking about combining the RPC handler and the whispernet client, that is not
feasible since the whispernet client is created in the browser since it is
dependent on it; the RPC client has no reason to live in the browser.)

[willchan no longer on Chromium, 2014/08/08 18:01:20]

I chatted with rkc@ offline about this, but I can't review this without
understanding the expected lifetime of the ChromeWhispernetClient. Please link
me to that code.

[Charlie, 2014/08/08 21:57:52]

Putting just this one back to AsWeakPtr(), and getting rid of the
WhispernetClient::Shutdown() call. In theory we could instead add a
CancelInitialize() method to cancel the callback, but that seems unnecessary
when WeakPtr already solves the problem.

[willchan no longer on Chromium, 2014/08/08 22:53:33]

Please don't use SupportsWeakPtr(), there's reasonably strong consensus that
it's a bad idea and we should kill it
-https://groups.google.com/a/chromium.org/d/msg/chromium-dev/N6w_CkKEwvE/evs2pRDD0loJ.
If you must use WeakPtrs, use a WeakPtrFactory instead.

[Charlie, 2014/08/08 23:38:30]

Noted. Will fix after this is in.

+                 base::Unretained(this),
                  "Whispernet proxy initialization"));
 }
 
 CopresenceClient::~CopresenceClient() {
-}
-
-void CopresenceClient::Shutdown() {
-  DVLOG(3) << "Shutting down client.";
   delegate_->GetWhispernetClient()->Shutdown();
-  rpc_handler_->DisconnectFromWhispernet();
 }
 
 // Returns false if any operations were malformed.
@@ -76,7 +72,7 @@ void CopresenceClient::CompleteInitialization() {
     return;
 
   if (!init_failed_)
-    rpc_handler_->ConnectToWhispernet(delegate_->GetWhispernetClient());
+    rpc_handler_->ConnectToWhispernet();
 
   for (std::vector<PendingRequest>::iterator request =
            pending_requests_queue_.begin();