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

remoting/host/host_signaling_manager.h

+// 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.
+
+#ifndef REMOTING_HOST_HOST_SIGNALING_MANAGER_H_
+#define REMOTING_HOST_HOST_SIGNALING_MANAGER_H_
+
+#include <string>
+
+#include "base/callback.h"
+#include "base/memory/scoped_ptr.h"
+#include "remoting/base/auto_thread_task_runner.h"
+#include "remoting/base/rsa_key_pair.h"
+#include "remoting/host/heartbeat_sender.h"
+#include "remoting/signaling/xmpp_signal_strategy.h"
+
+namespace base {
+class TimeDelta;
+}
+
+namespace net {
+class NetworkChangeNotifier;
+}
+
+namespace remoting {
+
+class ChromotingHostContext;
+class DnsBlackholeChecker;
+class HeartbeatSender;
+class OAuthTokenGetter;
+class SignalStrategy;
+class SignalingConnector;
+
+// HostSignalingManager has 2 functions:
+// 1. Keep sending regular heartbeats to the service.
+// 2. Keep the host process alive while sending host-offline-reason heartbeat.
+class HostSignalingManager {

[Lambros, 2014/12/16 02:14:05]

Please update the CL description with the new class name.

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

Ooops.  Done.

+ public:
+  // TODO(lukasza): Refactor to limit the number of parameters below.
+  // Probably necessitates refactoring HostProcess to extract a new
+  // class to read and store config/policy/cmdline values.
+  static scoped_ptr<HostSignalingManager> Create(
+      const base::Closure& on_heartbeat_successful_callback,
+      const base::Closure& on_unknown_host_id_error_callback,
+      const base::Closure& on_auth_failed_callback,
+      const ChromotingHostContext& host_context,
+      const XmppSignalStrategy::XmppServerConfig& xmpp_server_config,
+      const std::string& talkgadget_prefix,
+      const std::string& host_id,
+      const scoped_refptr<const RsaKeyPair>& key_pair,
+      const std::string& directory_bot_jid,
+      const std::string& oauth_refresh_token,
+      bool use_service_account);
+
+  ~HostSignalingManager();
+
+  // Gets signal strategy used for talking to the Chromoting bot.
+  // Return value is valid until |this| gets deleted.
+  SignalStrategy* signal_strategy() { return signal_strategy_.get(); }
+
+  // Kicks off sending a heartbeat containing a host-offline-reason attribute.
+  //
+  // Will delete |this| once either the bot acks receiving the
+  // |host_offline_reason|, or the |timeout| is reached.  Deleting
+  // |this| will release |network_task_runner_| and allow the host
+  // process to exit.
+  void SendHostOfflineReasonAndDelete(const std::string& host_offline_reason,

[Lambros, 2014/12/16 02:14:05]

Won't you get double-deletion when the scoped_ptr returned from Create() goes
out of scope? If the object is self-owned, perhaps Create() shouldn't be
returning a scoped_ptr?
Also, if the object is responsible for its own deletion, the dtor should be
private?

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

The caller of Create takes ownership of the returned scoped_ptr.  Before calling
SendHostOfflineReasonAndDelete, the owner is supposed to release the scoped_ptr
first.

[Lambros, 2014/12/17 23:44:34]

So the caller owns the object until it calls SendHostOfflineReasonAndDelete()
which effectively takes back ownership from the caller?
In that case, I'd prefer if SendHostOfflineReasonAndDelete() accepted a
scoped_ptr to the object. It could be a static method, or perhaps a non-static
method with DCHECK(param.get() == this);
Admittedly, this seems strange, but I think it's the right thing to do. Check
with others on our team.
Chromium does use the pattern of ptr.release()->DoStuffAndDelete() a lot, but
bear in mind that such patterns were in common usage before Chromium gained a
scoped_ptr with moveable semantics.

[Łukasz Anforowicz, 2014/12/18 00:03:41]

I don't have a strong opinion here (with no recent C++ experience under my
belt).  The current code is an implementation of option #4 as recommended by
Wez:

Re the ownership hand-off, I agree that it's not ideal. I'd suggest:

1)
// Take a bare pointer to the XMPP manager before passing it to itself to
delete. (Essentially the same as your suggestion).
HostXmppManager* host_xmpp_manager_temp = host_xmpp_manager.get();
host_xmpp_manager_temp->SendShutdownReasonAndDeleteSelf(host_xmpp_manager.Pass());

or

2)
// Pass the pointer-to-scoped_ptr so that the function can perform the Pass()
internally.
host_xmpp_manager->SendShutdownReasonAndDeleteSelf(&host_xmpp_manager);

or

3)
// Variant of #2 in which we use a static class method, which internally does a
similar dance to #1, thereby keeping the caller simple.
HostXmppManager::SendShutdownReasonAndDeleteManager(host_xmpp_manager.Pass());

or

4)
// Release the manager as part of the call, so that its self-ownership becomes
implicit.
host_xmpp_manager.release()->SendShutdownReasonAndDelete();

#4 is probably the approach I'd take; I don't think having the object own itself
via a scoped_ptr<> is necessary (I'm not even sure it'll work, since IIRC the
object gets deleted by scoped_ptr<> before the contained pointer gets NULL'd, so
you'd end up using-after-free) - instead the object can just |delete this| when
it's done.

Thanks for persevering with this; I appreciate the temptation of ref-counting in
situations like this, but our experience in Chromium development has been that
it leads to more problems if not used sparingly!

+                                      const base::TimeDelta& timeout);
+
+ private:
+  HostSignalingManager(
+      scoped_refptr<AutoThreadTaskRunner> network_task_runner,
+      scoped_ptr<net::NetworkChangeNotifier> network_change_notifier,
+      scoped_ptr<SignalStrategy> signal_strategy,
+      scoped_ptr<SignalingConnector> signaling_connector,
+      scoped_ptr<HeartbeatSender> heartbeat_sender);
+
+  void OnAckOrTimeout(HeartbeatSender::AckOrTimeout ack_or_timeout);
+
+  // Order of fields below is important for destructing them in the right order.
+  // - |heartbeat_sender_| and |signaling_connector_| have to be destructed
+  //   before |signal_strategy_| because their destructors need to call
+  //   signal_strategy_->RemoveListener(this)
+  // - |signaling_connector_| has to be destructed before
+  //   |network_change_notifier_| because its destructor needs to deregister
+  //   network change notifications
+  // - |network_task_runner_| is used by all the other fields and has to be
+  //   destructed last.
+  scoped_refptr<AutoThreadTaskRunner> network_task_runner_;
+  scoped_ptr<net::NetworkChangeNotifier> network_change_notifier_;
+  scoped_ptr<SignalStrategy> signal_strategy_;
+  scoped_ptr<SignalingConnector> signaling_connector_;
+  scoped_ptr<HeartbeatSender> heartbeat_sender_;
+
+  // HostSignalingManager object owns itself via |self_| field while
+  // SendHostOfflineReasonAndDelete is waiting for an ack or timeout.
+  HostSignalingManager* self_;

[Lambros, 2014/12/16 02:14:05]

There's no ref-counting, so you don't need this.

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

I do.  I am calling "delete self_" in OnAckOrTimeout.

[Lambros, 2014/12/17 23:44:34]

Couldn't you do "delete this;" instead?

[Łukasz Anforowicz, 2014/12/18 00:03:41]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(HostSignalingManager);
+};
+
+}  // namespace remoting
+
+#endif  // REMOTING_HOST_HOST_SIGNALING_MANAGER_H_