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) {
+ChromotingJNIInstance::ChromotingJNIInstance() {
   JNIEnv* env = base::android::AttachCurrentThread();
 
+  // The base and networks stacks must be registered with the JNI in order to

[Wez, 2013/07/11 22:32:08]

nit: the JNI -> JNI

[solb, 2013/07/11 23:59:09]

Done.

+  // work on Android. A (required) AtExitManager cleans this up at world's end.

[Wez, 2013/07/11 22:32:08]

nit: why is (required) needed here?

[solb, 2013/07/11 23:59:09]

Done.

   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 the Java side of things, so we

[Wez, 2013/07/11 22:32:08]

nit: by the Java side of things -> by Java

[solb, 2013/07/11 23:59:09]

Done.

+  // need to attach and start a special type of message loop in order to allow
+  // Chromium code to queue tasks there.
+  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());
-  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() {
   JNIEnv* env = base::android::AttachCurrentThread();
   env->DeleteGlobalRef(class_);
   // TODO(solb) detach all threads from JVM

[Wez, 2013/07/11 22:32:08]

Why the TODO?

[solb, 2013/07/11 23:59:09]

As I mentioned in a prior CL, it's something I plan to figure out a proper
solution to, but I just haven't had a chance yet.

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

OK, please file a bug for that work and refer to it in the comment. Also, prefer
the form:

"TODO: crbug.com/<bug#> Detach threads from JVM here."

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

Ping.

 }
 
-void ChromotingJNIInstance::ConnectToHost(jstring username,
-                                          jstring auth_token,
-                                          jstring host_jid,
-                                          jstring host_id,
-                                          jstring host_pubkey) {
-  JNIEnv* env = base::android::AttachCurrentThread();
-
-  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);
+void ChromotingJNIInstance::ConnectToHost(const char* username,
+                                          const char* auth_token,
+                                          const char* host_jid,
+                                          const char* host_id,
+                                          const char* host_pubkey) {

[Wez, 2013/07/11 22:32:08]

nit: DCHECK() that you're called on the expected thread.

[solb, 2013/07/11 23:59:09]

The threading of this method has become less important as the code has
developed. I'll DCHECK() on whether we're currently "connected" instead.

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

Don't other threads read these values, though? Are you sure that they can never
do that while the object is disconnected?

+  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();
-
-  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_);
+  // All our work must be done on the network thread.

[Wez, 2013/07/11 22:32:08]

ConnectToHost seems to store the strings while on the calling, Java thread, so
this comment seems wrong?

[solb, 2013/07/11 23:59:09]

Done.

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

[Wez, 2013/07/11 22:32:08]

Why does being a singleton make it safe? Who deletes the singleton?

[solb, 2013/07/11 23:59:09]

The singleton is deleted automatically at exit, which is after
DisconnectFromHost has been invoked. Thus, all callbacks will have stopped
before destruction. I'm adding a DCHECK() and comment on the destructor to this
effect.

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

Ah, OK, so the guarantee is that the object will only be torn down on exit, by
which time this object will be gone.

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

Blah... by which time there can be no further pending tasks, by definition.

+    network_task_runner_->PostTask(FROM_HERE, base::Bind(
+        &ChromotingJNIInstance::DisconnectFromHost,
+        base::Unretained(this)));
+    return;
   }
 
-  // We're a singleton, so Unretained is safe here.
-  net_runner_->PostTask(FROM_HERE, base::Bind(
-      &ChromotingJNIInstance::DisconnectFromHostOnNetworkThread,
-      base::Unretained(this)));
+  client_.reset();
+  connection_.reset();
+  client_context_.reset();
+  client_config_.reset();
+  chat_.reset();  // This object must outlive client_.

[Wez, 2013/07/11 22:32:08]

nit: Suggest putting a comment before |client_| "|client_| must be torn down
before |signalling_|." instead. And please put these sorts of comments before
the code they refer to, rather than at the end of the line.

[solb, 2013/07/11 23:59:09]

Done.

+  chat_config_.reset();  // TODO(solb) Restructure to reuse between sessions.

[Wez, 2013/07/11 22:32:08]

Either remove this TODO or elaborate on what you intend to re-use.

[solb, 2013/07/11 23:59:09]

Done.

+  netset_.reset();
 }
 
-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(announce_secret_, pin_));
 }
 
 void ChromotingJNIInstance::FetchSecret(
     bool pairable,
-    const protocol::SecretFetchedCallback& callback_encore) {
+    const protocol::SecretFetchedCallback& callback) {
   // All our work must be done on the UI thread.

[Wez, 2013/07/11 22:32:08]

Previous method says network thread, not UI thread. Which is it?

[solb, 2013/07/11 23:59:09]

I believe it's correct as written. This method is invoked by the
NegotiatingClientAuthenticator on the network thread. It trampolines the call to
the UI thread and makes a JNI call into Java to prompt the user for her PIN.
Upon entry of this information, Java makes a JNI call back to
AuthenticateWithPin(), which invokes the callback (the one that was received as
an argument to this method) on the network thread.

-  if (!ui_runner_->BelongsToCurrentThread()) {
+  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::FetchSecret,
         base::Unretained(this),
         pairable,
-        callback_encore));
+        callback));
     return;
   }
 
-  announce_secret_ = callback_encore;
+  announce_secret_ = 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.

[Wez, 2013/07/11 22:32:08]

Here too

[solb, 2013/07/11 23:59:09]

Once again, we're invoked on the network thread but must operate on the UI one.

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

See comment above; no need for this comment at all, then.

-  if (!ui_runner_->BelongsToCurrentThread()) {
+  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.

[Wez, 2013/07/11 22:32:08]

nit: Move this into the body of the method

[solb, 2013/07/11 23:59:09]

Done.

+void ChromotingJNIInstance::OnConnectionReady(bool ready) {}

[Wez, 2013/07/11 22:32:08]

Why do both exist, then?

[solb, 2013/07/11 23:59:09]

Done.

 
+// We ignore this message because we don't deal with capabilities.

[Wez, 2013/07/11 22:32:08]

You don't really need this comment; it's clear from the method name that we just
ignore capabilities.

[solb, 2013/07/11 23:59:09]

Done.

 void ChromotingJNIInstance::SetCapabilities(const std::string& capabilities) {}
 
+// We ignore this message because OnConnectionState() reveals the same info.

[Wez, 2013/07/11 22:32:08]

nit: Move this comment into the body of the method

[solb, 2013/07/11 23:59:09]

Done.

 void ChromotingJNIInstance::SetPairingResponse(
-    const protocol::PairingResponse& response) {}
+    const protocol::PairingResponse& response) {
+}
 
 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.
+// We don't support third party authentication, so we return a null pointer.

[Wez, 2013/07/11 22:32:08]

nit: Move this inside the method, and reword e.g. "Return null to indicate that
third party authentication is not supported."

[solb, 2013/07/11 23:59:09]

Done.

 scoped_ptr<protocol::ThirdPartyClientAuthenticator::TokenFetcher>
     ChromotingJNIInstance::GetTokenFetcher(const std::string& host_public_key) {
-  LOG(INFO) << "ChromotingJNIInstance::GetTokenFetcher(...) [unimplemented]";
   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_);
+    frames_ = 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.

[Wez, 2013/07/11 22:32:08]

nit: Blank line before this comment.

[solb, 2013/07/11 23:59:09]

Done.

   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"));
   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_,
                                      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;
 
   chat_.reset(new XmppSignalStrategy(url_requester_,
-                                     username_cstr_,
-                                     auth_token_cstr_,
+                                     username_,
+                                     auth_token_,
                                      CHAT_AUTH_METHOD,
                                      *chat_config_));
 
   netset_.reset(new NetworkSettings(NetworkSettings::NAT_TRAVERSAL_OUTGOING));
   scoped_ptr<protocol::TransportFactory> fact(
       protocol::LibjingleTransportFactory::Create(*netset_, url_requester_));
 
   client_->Start(chat_.get(), fact.Pass());
 }
 
-void ChromotingJNIInstance::DisconnectFromHostOnNetworkThread() {
-  DCHECK(net_runner_->BelongsToCurrentThread());
-
-  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();
-}
-
 }  // namespace remoting