Restructure chromoting_jni_instance handling of Java strings

remoting/client/jni/chromoting_jni_instance.cc

 // Copyright 2013 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 "remoting/client/jni/chromoting_jni_instance.h"
 
 #include "base/android/base_jni_registrar.h"
 #include "base/android/jni_android.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/logging.h"
 #include "base/memory/singleton.h"
 #include "net/android/net_jni_registrar.h"
 #include "remoting/base/url_request_context.h"
 #include "remoting/client/audio_player.h"
 #include "remoting/protocol/libjingle_transport_factory.h"
 
 namespace remoting {
 
 // static
 ChromotingJNIInstance* ChromotingJNIInstance::GetInstance() {
   return Singleton<ChromotingJNIInstance>::get();
 }
 
 ChromotingJNIInstance::ChromotingJNIInstance()
-    : username_cstr_(NULL),
-      auth_token_cstr_(NULL),
-      host_jid_cstr_(NULL),
-      host_id_cstr_(NULL),
-      host_pubkey_cstr_(NULL),
-      pin_cstr_(NULL) {
+    : connected_(false) {
   JNIEnv* env = base::android::AttachCurrentThread();
 
+  // The base and networks stacks must be registered with JNI in order to work
+  // on Android. An AtExitManager cleans this up at world's end.
   collector_.reset(new base::AtExitManager());
   base::android::RegisterJni(env);
   net::android::RegisterJni(env);
 
-  LOG(INFO) << "starting main message loop";
+  // On Android, the UI thread is managed by Java, so we need to attach and
+  // start a special type of message loop to allow Chromium code to run tasks.
+  LOG(INFO) << "Starting main message loop";
   ui_loop_.reset(new base::MessageLoopForUI());
   ui_loop_->Start();
 
-  LOG(INFO) << "spawning additional threads";
-  ui_runner_ = new AutoThreadTaskRunner(ui_loop_->message_loop_proxy(),
+  LOG(INFO) << "Spawning additional threads";
+  ui_task_runner_ = new AutoThreadTaskRunner(ui_loop_->message_loop_proxy(),
                                         base::MessageLoop::QuitClosure());

[Wez, 2013/07/12 00:24:48]

AutoThreads don't exit until there are no more references to them, but you're
holding a reference to them from a global singleton that won't be torn down
until the main thread exits, so haven't you created a cyclical dependency here?

[solb, 2013/07/12 01:14:22]

I don't believe so. Unlike the threads that are created here, the main (UI)
thread is managed; thus, the singleton gets torn down as the shared library is
unloaded, which is before the managed thread could exit.

[Wez, 2013/07/12 01:25:44]

But you are wrapping the main MessageLoopForUI in an AutoThreadTaskRunner,
aren't you? Is that not controlling that MessageLoop's lifetime?

-  net_runner_ = AutoThread::CreateWithType("native_net",
-                                           ui_runner_,
+  network_task_runner_ = AutoThread::CreateWithType("native_net",
+                                           ui_task_runner_,
                                            base::MessageLoop::TYPE_IO);
-  disp_runner_ = AutoThread::CreateWithType("native_disp",
-                                            ui_runner_,
+  display_task_runner_ = AutoThread::CreateWithType("native_disp",
+                                            ui_task_runner_,
                                             base::MessageLoop::TYPE_DEFAULT);
 
-  url_requester_ = new URLRequestContextGetter(ui_runner_, net_runner_);
+  url_requester_ = new URLRequestContextGetter(ui_task_runner_,
+                                               network_task_runner_);
 
   class_ = static_cast<jclass>(env->NewGlobalRef(env->FindClass(JAVA_CLASS)));
 }
 
 ChromotingJNIInstance::~ChromotingJNIInstance() {
+  DCHECK(!connected_);
+
   JNIEnv* env = base::android::AttachCurrentThread();
   env->DeleteGlobalRef(class_);
   // TODO(solb) detach all threads from JVM
 }
 
-void ChromotingJNIInstance::ConnectToHost(jstring username,
-                                          jstring auth_token,
-                                          jstring host_jid,
-                                          jstring host_id,
-                                          jstring host_pubkey) {
-  JNIEnv* env = base::android::AttachCurrentThread();
+void ChromotingJNIInstance::ConnectToHost(const char* username,
+                                          const char* auth_token,
+                                          const char* host_jid,
+                                          const char* host_id,
+                                          const char* host_pubkey) {
+  DCHECK(!connected_);
+  connected_ = true;
 
-  username_jstr_ = static_cast<jstring>(env->NewGlobalRef(username));
-  auth_token_jstr_ = static_cast<jstring>(env->NewGlobalRef(auth_token));
-  host_jid_jstr_ = static_cast<jstring>(env->NewGlobalRef(host_jid));
-  host_id_jstr_ = static_cast<jstring>(env->NewGlobalRef(host_id));
-  host_pubkey_jstr_ = static_cast<jstring>(env->NewGlobalRef(host_pubkey));
-
-  username_cstr_ = env->GetStringUTFChars(username_jstr_, NULL);
-  auth_token_cstr_ = env->GetStringUTFChars(auth_token_jstr_, NULL);
-  host_jid_cstr_ = env->GetStringUTFChars(host_jid_jstr_, NULL);
-  host_id_cstr_ = env->GetStringUTFChars(host_id_jstr_, NULL);
-  host_pubkey_cstr_ = env->GetStringUTFChars(host_pubkey_jstr_, NULL);
+  username_ = username;
+  auth_token_ = auth_token;
+  host_jid_ = host_jid;
+  host_id_ = host_id;
+  host_pubkey_ = host_pubkey;
 
   // We're a singleton, so Unretained is safe here.
-  disp_runner_->PostTask(FROM_HERE, base::Bind(
+  display_task_runner_->PostTask(FROM_HERE, base::Bind(
       &ChromotingJNIInstance::ConnectToHostOnDisplayThread,
       base::Unretained(this)));
 }
 
 void ChromotingJNIInstance::DisconnectFromHost() {
-  JNIEnv* env = base::android::AttachCurrentThread();
+  DCHECK(connected_);
+  connected_ = false;
 
-  env->ReleaseStringUTFChars(username_jstr_, username_cstr_);
-  env->ReleaseStringUTFChars(auth_token_jstr_, auth_token_cstr_);
-  env->ReleaseStringUTFChars(host_jid_jstr_, host_jid_cstr_);
-  env->ReleaseStringUTFChars(host_id_jstr_, host_id_cstr_);
-  env->ReleaseStringUTFChars(host_pubkey_jstr_, host_pubkey_cstr_);
-
-  username_cstr_ = NULL;
-  auth_token_cstr_ = NULL;
-  host_jid_cstr_ = NULL;
-  host_id_cstr_ = NULL;
-  host_pubkey_cstr_ = NULL;
-
-  env->DeleteGlobalRef(username_jstr_);
-  env->DeleteGlobalRef(auth_token_jstr_);
-  env->DeleteGlobalRef(host_jid_jstr_);
-  env->DeleteGlobalRef(host_id_jstr_);
-  env->DeleteGlobalRef(host_pubkey_jstr_);
-
-  if (pin_cstr_) {
-    // AuthenticatedWithPin() has been called.
-    env->ReleaseStringUTFChars(pin_jstr_, pin_cstr_);
-    pin_cstr_ = NULL;
-    env->DeleteGlobalRef(pin_jstr_);
-  }
+  username_ = "";
+  auth_token_ = "";
+  host_jid_ = "";
+  host_id_ = "";
+  host_pubkey_ = "";
+  pin_ = "";

[Wez, 2013/07/12 00:24:48]

This is racy if the caller calls ConnectToHost and then immediately
DisconnectFromHost then this will crash it, since you're writing these values on
one thread but reading from the other.

[solb, 2013/07/12 01:14:22]

This is always called from the same thread as ConnectToHost(), but I'll DCHECK()
that now that you mention it.

[Wez, 2013/07/12 01:25:44]

That's not the point - ConnectToHost and DisconnectFromHost write to these
fields but e.g. ConnectToHostOnNetworkThread _reads_ them.

 
   // We're a singleton, so Unretained is safe here.
-  net_runner_->PostTask(FROM_HERE, base::Bind(
+  network_task_runner_->PostTask(FROM_HERE, base::Bind(
       &ChromotingJNIInstance::DisconnectFromHostOnNetworkThread,
       base::Unretained(this)));
 }
 
-void ChromotingJNIInstance::AuthenticateWithPin(jstring pin) {
-  JNIEnv* env = base::android::AttachCurrentThread();
-
-  pin_jstr_ = static_cast<jstring>(env->NewGlobalRef(pin));
-  pin_cstr_ = env->GetStringUTFChars(pin_jstr_, NULL);
-
-  net_runner_->PostTask(FROM_HERE, base::Bind(announce_secret_, pin_cstr_));
+void ChromotingJNIInstance::AuthenticateWithPin(const char* pin) {
+  pin_ = pin;
+  network_task_runner_->PostTask(FROM_HERE, base::Bind(pin_callback_, pin_));
 }
 
 void ChromotingJNIInstance::FetchSecret(
     bool pairable,
-    const protocol::SecretFetchedCallback& callback_encore) {
-  // All our work must be done on the UI thread.
-  if (!ui_runner_->BelongsToCurrentThread()) {
+    const protocol::SecretFetchedCallback& callback) {
+  // Although we're invoked on the network thread, we must work on the UI one.

[Wez, 2013/07/12 00:24:48]

This comment is superfluous, since it's clear that that's what we're doing.

[solb, 2013/07/12 01:14:22]

Done.

+  if (!ui_task_runner_->BelongsToCurrentThread()) {
     // We're a singleton, so Unretained is safe here.

[Wez, 2013/07/12 00:24:48]

Based on the discussion around the singleton on other comments, I don't think
this comment is necessary either, just put a comment e.g. in the destructor to
indicate that since we're only torn down on exit, we needn't be retained.

[solb, 2013/07/12 01:14:22]

Done.

-    ui_runner_->PostTask(FROM_HERE, base::Bind(
+    ui_task_runner_->PostTask(FROM_HERE, base::Bind(
         &ChromotingJNIInstance::FetchSecret,
         base::Unretained(this),
         pairable,
-        callback_encore));
+        callback));
     return;
   }
 
-  announce_secret_ = callback_encore;
+  pin_callback_ = callback;
   JNIEnv* env = base::android::AttachCurrentThread();
   env->CallStaticVoidMethod(
       class_,
       env->GetStaticMethodID(class_, "displayAuthenticationPrompt", "()V"));
 }
 
 void ChromotingJNIInstance::OnConnectionState(
     protocol::ConnectionToHost::State state,
     protocol::ErrorCode error) {
-  // All our work must be done on the UI thread.
-  if (!ui_runner_->BelongsToCurrentThread()) {
+  // Although we're invoked on the network thread, we must work on the UI one.
+  if (!ui_task_runner_->BelongsToCurrentThread()) {
     // We're a singleton, so Unretained is safe here.
-    ui_runner_->PostTask(FROM_HERE, base::Bind(
+    ui_task_runner_->PostTask(FROM_HERE, base::Bind(
         &ChromotingJNIInstance::OnConnectionState,
         base::Unretained(this),
         state,
         error));
     return;
   }
 
   JNIEnv* env = base::android::AttachCurrentThread();
   env->CallStaticVoidMethod(
     class_,
     env->GetStaticMethodID(class_, "reportConnectionStatus", "(II)V"),
     state,
     error);
 }
 
 void ChromotingJNIInstance::OnConnectionReady(bool ready) {
+  // We ignore this message, since OnConnectionState() tells us the same thing.
 }
 
 void ChromotingJNIInstance::SetCapabilities(const std::string& capabilities) {}
 
 void ChromotingJNIInstance::SetPairingResponse(
-    const protocol::PairingResponse& response) {}
+    const protocol::PairingResponse& response) {
+  // We ignore this message because OnConnectionState() reveals the same info.

[Wez, 2013/07/12 00:24:48]

This comment actually seems wrong; we just don't support pairing, right? We
won't see this called, since we don't ask for pairing, so NOTIMPLEMENTED instead
of the comment.

[solb, 2013/07/12 01:14:22]

Sorry; I guess I didn't pay much attention to what this was for.

+}
 
 protocol::ClipboardStub* ChromotingJNIInstance::GetClipboardStub() {
   NOTIMPLEMENTED();
   return NULL;
 }
 
 protocol::CursorShapeStub* ChromotingJNIInstance::GetCursorShapeStub() {
   NOTIMPLEMENTED();
   return NULL;
 }
 
-// We don't use NOTIMPLEMENTED() here because NegotiatingClientAuthenticator
-// calls this even if it doesn't use the configuration method, and we don't
-// want to print an error on every run.
 scoped_ptr<protocol::ThirdPartyClientAuthenticator::TokenFetcher>
     ChromotingJNIInstance::GetTokenFetcher(const std::string& host_public_key) {
-  LOG(INFO) << "ChromotingJNIInstance::GetTokenFetcher(...) [unimplemented]";
+  // Return null to indicate that third-party authentication is unsupported.
   return scoped_ptr<protocol::ThirdPartyClientAuthenticator::TokenFetcher>();
 }
 
 void ChromotingJNIInstance::ConnectToHostOnDisplayThread() {
-  DCHECK(disp_runner_->BelongsToCurrentThread());
+  DCHECK(display_task_runner_->BelongsToCurrentThread());
 
-  if (!frames_.get()) {
-    frames_ = new FrameConsumerProxy(disp_runner_);
+  if (!frame_consumer_.get()) {
+    frame_consumer_ = new FrameConsumerProxy(display_task_runner_);
     // TODO(solb) Instantiate some FrameConsumer implementation and attach it.
   }
 
   // We're a singleton, so Unretained is safe here.
-  net_runner_->PostTask(FROM_HERE, base::Bind(
+  network_task_runner_->PostTask(FROM_HERE, base::Bind(
       &ChromotingJNIInstance::ConnectToHostOnNetworkThread,
       base::Unretained(this)));
 }
 
 void ChromotingJNIInstance::ConnectToHostOnNetworkThread() {
-  DCHECK(net_runner_->BelongsToCurrentThread());
+  DCHECK(network_task_runner_->BelongsToCurrentThread());
 
   client_config_.reset(new ClientConfig());
-  client_config_->host_jid = host_jid_cstr_;
-  client_config_->host_public_key = host_pubkey_cstr_;
+  client_config_->host_jid = host_jid_;
+  client_config_->host_public_key = host_pubkey_;
+
   // We're a singleton, so Unretained is safe here.
   client_config_->fetch_secret_callback = base::Bind(
       &ChromotingJNIInstance::FetchSecret,
       base::Unretained(this));
-  client_config_->authentication_tag = host_id_cstr_;
+  client_config_->authentication_tag = host_id_;
 
   // TODO(solb) Move these hardcoded values elsewhere:
   client_config_->authentication_methods.push_back(
       protocol::AuthenticationMethod::FromString("spake2_pair"));

[Wez, 2013/07/12 00:24:48]

This object doesn't implement pairing, so don't ask for spake2_pair

[solb, 2013/07/12 01:14:22]

Done.

   client_config_->authentication_methods.push_back(
       protocol::AuthenticationMethod::FromString("spake2_hmac"));
   client_config_->authentication_methods.push_back(
       protocol::AuthenticationMethod::FromString("spake2_plain"));
 
-  client_context_.reset(new ClientContext(net_runner_.get()));
+  client_context_.reset(new ClientContext(network_task_runner_.get()));
   client_context_->Start();
 
   connection_.reset(new protocol::ConnectionToHost(true));
 
   client_.reset(new ChromotingClient(*client_config_,
                                      client_context_.get(),
                                      connection_.get(),
                                      this,
-                                     frames_,
+                                     frame_consumer_,
                                      scoped_ptr<AudioPlayer>()));
 
-  chat_config_.reset(new XmppSignalStrategy::XmppServerConfig());
-  chat_config_->host = CHAT_SERVER;
-  chat_config_->port = CHAT_PORT;
-  chat_config_->use_tls = CHAT_USE_TLS;
+  signaling_config_.reset(new XmppSignalStrategy::XmppServerConfig());
+  signaling_config_->host = CHAT_SERVER;
+  signaling_config_->port = CHAT_PORT;
+  signaling_config_->use_tls = CHAT_USE_TLS;
 
-  chat_.reset(new XmppSignalStrategy(url_requester_,
-                                     username_cstr_,
-                                     auth_token_cstr_,
-                                     CHAT_AUTH_METHOD,
-                                     *chat_config_));
+  signaling_.reset(new XmppSignalStrategy(url_requester_,
+                                          username_,
+                                          auth_token_,
+                                          CHAT_AUTH_METHOD,
+                                          *signaling_config_));
 
-  netset_.reset(new NetworkSettings(NetworkSettings::NAT_TRAVERSAL_OUTGOING));
+  network_settings_.reset(new NetworkSettings(
+      NetworkSettings::NAT_TRAVERSAL_OUTGOING));
   scoped_ptr<protocol::TransportFactory> fact(
-      protocol::LibjingleTransportFactory::Create(*netset_, url_requester_));
+      protocol::LibjingleTransportFactory::Create(*network_settings_,
+                                                  url_requester_));
 
-  client_->Start(chat_.get(), fact.Pass());
+  client_->Start(signaling_.get(), fact.Pass());
 }
 
 void ChromotingJNIInstance::DisconnectFromHostOnNetworkThread() {
-  DCHECK(net_runner_->BelongsToCurrentThread());
+  DCHECK(network_task_runner_->BelongsToCurrentThread());
 
+  // |client_| must be torn down before |signaling_|.
   client_.reset();
   connection_.reset();
   client_context_.reset();
   client_config_.reset();
-  chat_.reset();  // This object must outlive client_.
-  chat_config_.reset();  // TODO(solb) Restructure to reuse between sessions.
-  netset_.reset();
+  signaling_.reset();
+  signaling_config_.reset();
+  network_settings_.reset();
 }
 
 }  // namespace remoting