Adding the Copresence RpcHandler and HttpPost helper.

components/copresence/copresence_client.cc

 // Copyright 2014 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.
 
 #include "components/copresence/public/copresence_client.h"
 
 #include "base/bind.h"
 #include "components/copresence/public/copresence_client_delegate.h"
 #include "components/copresence/public/whispernet_client.h"
 #include "components/copresence/rpc/rpc_handler.h"
 
 namespace copresence {
 
 PendingRequest::PendingRequest(const copresence::ReportRequest& report,
                                const std::string app_id,
                                const StatusCallback& callback)
     : report(report), app_id(app_id), callback(callback) {
 }
 
 PendingRequest::~PendingRequest() {
 }
 
 // Public methods
 
 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.
 void CopresenceClient::ExecuteReportRequest(copresence::ReportRequest request,
                                             const std::string& app_id,
                                             const StatusCallback& callback) {
   // Don't take on any more requests, we can't execute any, init failed.
   if (init_failed_) {
     callback.Run(FAIL);
     return;
@@ skipping 10 matching lines @@
   }
 }
 
 // Private methods
 
 void CopresenceClient::CompleteInitialization() {
   if (pending_init_operations_)
     return;
 
   if (!init_failed_)
-    rpc_handler_->ConnectToWhispernet(delegate_->GetWhispernetClient());
+    rpc_handler_->ConnectToWhispernet();
 
   for (std::vector<PendingRequest>::iterator request =
            pending_requests_queue_.begin();
        request != pending_requests_queue_.end();
        ++request) {
     if (init_failed_) {
       request->callback.Run(FAIL);
     } else {
       rpc_handler_->SendReportRequest(
           make_scoped_ptr(new copresence::ReportRequest(request->report)),
           request->app_id,
           request->callback);
     }
   }
   pending_requests_queue_.clear();
 }
 
 void CopresenceClient::InitStepComplete(const std::string& step, bool success) {
   if (!success) {
     LOG(ERROR) << step << " failed!";
     init_failed_ = true;
   }
 
   DVLOG(3) << "Init step: " << step << " complete.";
   pending_init_operations_--;
   CompleteInitialization();
 }
 
 }  // namespace copresence