Reporting of policy errors via host-offline-reason: part 3

remoting/host/minimum_heartbeat_supporter.cc

Index: remoting/host/minimum_heartbeat_supporter.cc
diff --git a/remoting/host/minimum_heartbeat_supporter.cc b/remoting/host/minimum_heartbeat_supporter.cc
new file mode 100644
index 0000000000000000000000000000000000000000..76f5e5bafeb27f7d1e14b3ad8b5eb93c81a74085
--- /dev/null
+++ b/remoting/host/minimum_heartbeat_supporter.cc
@@ -0,0 +1,203 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.

[Lambros, 2014/11/14 01:24:57]

2014, no '(c)'

[Łukasz Anforowicz, 2014/11/17 18:17:02]

Thanks. Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "remoting/host/minimum_heartbeat_supporter.h"
+
+#include "base/cancelable_callback.h"
+#include "base/message_loop/message_loop.h"
+#include "base/message_loop/message_loop_proxy.h"
+#include "base/time/time.h"
+#include "net/base/network_change_notifier.h"
+#include "net/socket/client_socket_factory.h"
+#include "remoting/base/logging.h"
+#include "remoting/host/chromoting_host_context.h"
+#include "remoting/host/dns_blackhole_checker.h"
+#include "remoting/host/heartbeat_sender.h"
+#include "remoting/host/signaling_connector.h"
+#include "remoting/signaling/xmpp_signal_strategy.h"
+
+namespace remoting {
+
+MinimumHeartbeatSupporter::MinimumHeartbeatSupporter(
+    /* TODO(lukasza): code review please: I heard that when passing

[Lambros, 2014/11/14 01:24:57]

Remove TODO

[Łukasz Anforowicz, 2014/11/17 18:17:02]

Done.

+     * a non-null pointer, one should use a reference rather than
+     * a pointer in C++.  I still used a pointer below, because
+     * I was not quite sure how to convert a scoped_ptr<T> into T&.
+     * Should I repent? */

[Lambros, 2014/11/14 01:24:57]

I don't feel strongly. Passing by const reference is better I think. At the
call-site, you would pass in
*context
instead of
context.get()

scoped_ptr::operator* will assert() if the object is null.

[Łukasz Anforowicz, 2014/11/17 18:17:02]

Ah, ok.  For some reason I thought that I would have to do (*(context_.get())),
rather than just (*_context).  I'll switch to a const reference.

+    const ChromotingHostContext* host_context,
+    const XmppSignalStrategy::XmppServerConfig& xmpp_server_config,
+    const std::string& talkgadget_prefix,
+    const std::string& host_id,
+    scoped_refptr<RsaKeyPair> key_pair,
+    const std::string& directory_bot_jid,
+    const std::string& oauth_refresh_token,
+    bool use_service_account)
+    : base::RefCountedDeleteOnMessageLoop<MinimumHeartbeatSupporter>(
+        base::MessageLoopProxy::current()),
+      network_task_runner_(host_context->network_task_runner()),
+      listener_(nullptr) {
+  DCHECK(network_task_runner_->BelongsToCurrentThread());
+
+  // Create a NetworkChangeNotifier for use by the signaling connector.
+  network_change_notifier_.reset(
+      net::NetworkChangeNotifier::Create());

[Lambros, 2014/11/14 01:24:57]

nit: Fits on one line.

[Łukasz Anforowicz, 2014/11/17 18:17:02]

Done.

+
+  signal_strategy_.reset(
+      new XmppSignalStrategy(
+          net::ClientSocketFactory::GetDefaultFactory(),
+          host_context->url_request_context_getter(),
+          xmpp_server_config));
+
+  scoped_ptr<DnsBlackholeChecker> dns_blackhole_checker(
+      new DnsBlackholeChecker(
+          host_context->url_request_context_getter(),
+          talkgadget_prefix));
+
+  signaling_connector_.reset(
+      new SignalingConnector(
+        signal_strategy_.get(),
+        dns_blackhole_checker.Pass(),
+        base::Bind(
+            &MinimumHeartbeatSupporter::OnAuthFailed,
+            base::Unretained(this))));
+
+  if (!oauth_refresh_token.empty()) {
+    scoped_ptr<OAuthTokenGetter::OAuthCredentials> oauth_credentials(
+        new OAuthTokenGetter::OAuthCredentials(
+            xmpp_server_config.username, oauth_refresh_token,
+            use_service_account));
+
+    oauth_token_getter_.reset(
+        new OAuthTokenGetter(
+          oauth_credentials.Pass(),

[Lambros, 2014/11/14 01:24:57]

nit: Indentation should be +4, not +2.

[Łukasz Anforowicz, 2014/11/17 18:17:02]

Done.

+          host_context->url_request_context_getter(),
+          false));
+
+    signaling_connector_->EnableOAuth(oauth_token_getter_.get());
+  }
+
+  heartbeat_sender_.reset(new HeartbeatSender(
+      this,
+      host_id,
+      signal_strategy_.get(),
+      key_pair,
+      directory_bot_jid));
+}
+
+MinimumHeartbeatSupporter::~MinimumHeartbeatSupporter() {
+  DCHECK(network_task_runner_->BelongsToCurrentThread());
+
+  // TODO(lukasza): code review please: anything we can do here?

[Lambros, 2014/11/14 01:24:57]

Remove TODO.

[Łukasz Anforowicz, 2014/11/17 18:17:02]

Done.

+  //                We also need to ensure that fields we own
+  //                are not used by frames higher on the callstack

[Lambros, 2014/11/14 01:24:57]

Are these fields exposed to things higher up in the call stack? Why is this
class owning them? This goes back to my earlier question about who should own
the things?

Either:
This object should be the owner, and should say to callers "you can look at my
fields, but don't use them beyond my lifetime".
Or:
Something else should be the owner, and this class should respect lifetime
requirements imposed by the owner.

[Łukasz Anforowicz, 2014/11/17 18:17:02]

MinimumHeartbeatSupporter object is the owner, but it owns itself and an
instance method is on the callstack when the MinimumHeartbeatSupporter object
drops the final refcount to itself.  Let's discuss this in the comment you had
about AddRef+ReleaseSoon pattern.

+  //                (so we don't pull the rug from under them
+  //                by reclaiming the memory they might be actively using).
+  //                In theory we could abuse thread_collision_warner.h,
+  //                but I am not convinced this is a very good idea.
+
+  // order of destroying fields below is important
+  heartbeat_sender_.reset();
+  signaling_connector_.reset();
+  oauth_token_getter_.reset();
+  signal_strategy_.reset();
+  network_change_notifier_.reset();
+
+  HOST_LOG << "MinimumHeartbeatSupporter is ready to die "
+           << "and allow the host process to exit";
+}
+
+SignalStrategy* MinimumHeartbeatSupporter::GetSignalStrategy() {
+  return signal_strategy_.get();
+}
+
+void MinimumHeartbeatSupporter::SetListener(Listener* listener) {
+  DCHECK(network_task_runner_->BelongsToCurrentThread());
+  listener_ = listener;
+}
+
+void MinimumHeartbeatSupporter::OnAuthFailed() {
+  DCHECK(network_task_runner_->BelongsToCurrentThread());
+  if (listener_ != nullptr)
+  {
+    listener_->OnAuthFailed();
+  }
+}
+
+void MinimumHeartbeatSupporter::OnHeartbeatSuccessful() {
+  DCHECK(network_task_runner_->BelongsToCurrentThread());
+  if (listener_ != nullptr)
+  {
+    listener_->OnHeartbeatSuccessful();
+  }
+}
+
+void MinimumHeartbeatSupporter::OnUnknownHostIdError() {
+  DCHECK(network_task_runner_->BelongsToCurrentThread());
+  if (listener_ != nullptr)
+  {
+    listener_->OnUnknownHostIdError();
+  }
+}
+
+void MinimumHeartbeatSupporter::SendHostOfflineReason(
+    const char* host_offline_reason,
+    const base::TimeDelta& timeout) {
+  DCHECK(network_task_runner_->BelongsToCurrentThread());
+  DCHECK(!timeout_callback_.get());
+  DCHECK(!ack_callback_.get());
+
+  HOST_LOG << "SendHostOfflineReason: trying to send "
+           << host_offline_reason
+           << " to the bot";
+
+  // the closures will own 1 refcount each to |this|
+  // so the closures will keep us alive until either a timeout or an ack happen
+  timeout_callback_.reset(new base::CancelableClosure(
+      base::Bind(
+          &MinimumHeartbeatSupporter::OnTimeout,
+          this)));
+  ack_callback_.reset(new base::CancelableClosure(
+      base::Bind(
+          &MinimumHeartbeatSupporter::OnAck,
+          this)));
+
+  // need to get the closures first, as our callbacks might
+  // get called as soon as we pass them out and this can potentially
+  // invalidate timeout_callback_ and ack_callback_
+  base::Closure local_timeout_callback = timeout_callback_->callback();
+  base::Closure local_ack_callback = ack_callback_->callback();
+
+  network_task_runner_->PostDelayedTask(
+      FROM_HERE, local_timeout_callback, timeout);
+  heartbeat_sender_->SetHostOfflineReason(
+      host_offline_reason, local_ack_callback);
+}
+
+void MinimumHeartbeatSupporter::OnAck() {
+  HOST_LOG << "SendHostOfflineReason: success - got ack";
+  OnAckOrTimeout();
+}
+
+void MinimumHeartbeatSupporter::OnTimeout() {
+  HOST_LOG << "SendHostOfflineReason: timeout - will die silently...";
+  OnAckOrTimeout();
+}
+
+void MinimumHeartbeatSupporter::OnAckOrTimeout() {
+  DCHECK(network_task_runner_->BelongsToCurrentThread());
+  DCHECK(timeout_callback_.get());
+  DCHECK(ack_callback_.get());
+
+  // |this| shouldn't be released until all our methods have safely
+  // unwound from the callstack
+  this->AddRef();

[Lambros, 2014/11/14 01:24:57]

This is smelly! :)  Is there some nicer way we can add a reference to |this|, in
a way that we can't accidentally forget about it?

[Łukasz Anforowicz, 2014/11/17 18:17:02]

Initially I planned to respond by adding a comment that explains what is going
on:

  // When OnAck or OnTimeout callback is called, it is possible that
  // the callbacks are the only objects holding a reference to
  // MinimumHeartgeatSupporter and keeping it alive (i.e. the HostProcess
  // object might have been destroyed already).  If we simply destroyed
  // the callbacks (by calling ack/timeout_callback_.reset()) below,
  // then it would destroy |this| as well, which would mean that memory
  // referenced by implementation of CancelableCallback.reset would no longer be
valid.
  // So - we have to extend our lifetime a little bit more - until all
  // our frames unwind / until the callstack unwinds to the message pump.
  // FWIW, a comment in base/message_loop/message_loop.h says that
  // the pattern we use below is a "common pattern".
 
But... after extracting this code into ack_or_timeout_reporter.cc I realized
that I don't need to use CancelableCallback and when directly using
WeakPtrFactory, the lifetime gimmicks are no longer necessary.

+  network_task_runner_->ReleaseSoon(FROM_HERE, this);
+
+  // cancel the callbacks + release them = free-up our 2 ref-counts
+  timeout_callback_.reset();
+  ack_callback_.reset();
+}
+
+}  // namespace remoting
+