Make adbconnection validate jdwpOptions and change defaults

The (libjdwp) default argument values are not entirely compatible with
adbconnection. This means that if you manually ran dalvikvm with
-XjdwpProvider:adbconnection and no -XjdwpOptions the jdwp thread
would deadlock. In order to prevent this from happening adbconnection
will now validate that the explictly passed in parameters are usable
and will add in required arguments automatically if they are not
present.

Test: dalvikvm -XjdwpProvider:adbconnection -cp testing.dex loop &
      adb forward tcp:12345 jdwp:<pid of dalvikvm>
      jdb -attach localhost:12345
Test: dalvikvm -XjdwpProvider:adbconnection \
               -XjdwpOptions:server=n       \
               -cp testing.dex loop;
      ensure process exits and check logcat for error message.

Change-Id: I8e26a9f4479ea8c324f741b9835bbc1fe8407047

1 files changed, 34 insertions, 5 deletions

diff --git a/adbconnection/adbconnection.cc b/adbconnection/adbconnection.cc
index 56677c9be6e..d8db923a289 100644
--- a/adbconnection/adbconnection.cc
+++ b/adbconnection/adbconnection.cc
@@ -827,12 +827,41 @@ void AdbConnectionState::AttachJdwpAgent(art::Thread* self) {
   agent_loaded_ = true;
 }
 
+bool ContainsArgument(const std::string& opts, const char* arg) {
+  return opts.find(arg) != std::string::npos;
+}
+
+bool ValidateJdwpOptions(const std::string& opts) {
+  bool res = true;
+  // The adbconnection plugin requires that the jdwp agent be configured as a 'server' because that
+  // is what adb expects and otherwise we will hit a deadlock as the poll loop thread stops waiting
+  // for the fd's to be passed down.
+  if (ContainsArgument(opts, "server=n")) {
+    res = false;
+    LOG(ERROR) << "Cannot start jdwp debugging with server=n from adbconnection.";
+  }
+  // We don't start the jdwp agent until threads are already running. It is far too late to suspend
+  // everything.
+  if (ContainsArgument(opts, "suspend=y")) {
+    res = false;
+    LOG(ERROR) << "Cannot use suspend=y with late-init jdwp.";
+  }
+  return res;
+}
+
 std::string AdbConnectionState::MakeAgentArg() {
-  // TODO Get this from something user settable?
   const std::string& opts = art::Runtime::Current()->GetJdwpOptions();
-  return agent_name_ + "=" + opts + (opts.empty() ? "" : ",")
-      + "ddm_already_active=" + (notified_ddm_active_ ? "y" : "n") + ","
-      + "transport=dt_fd_forward,address=" + std::to_string(remote_agent_control_sock_);
+  DCHECK(ValidateJdwpOptions(opts));
+  // TODO Get agent_name_ from something user settable?
+  return agent_name_ + "=" + opts + (opts.empty() ? "" : ",") +
+      "ddm_already_active=" + (notified_ddm_active_ ? "y" : "n") + "," +
+      // See the comment above for why we need to be server=y. Since the agent defaults to server=n
+      // we will add it if it wasn't already present for the convenience of the user.
+      (ContainsArgument(opts, "server=y") ? "" : "server=y,") +
+      // See the comment above for why we need to be suspend=n. Since the agent defaults to
+      // suspend=y we will add it if it wasn't already present.
+      (ContainsArgument(opts, "suspend=n") ? "" : "suspend=n") +
+      "transport=dt_fd_forward,address=" + std::to_string(remote_agent_control_sock_);
 }
 
 void AdbConnectionState::StopDebuggerThreads() {
@@ -849,7 +878,7 @@ extern "C" bool ArtPlugin_Initialize() REQUIRES_SHARED(art::Locks::mutator_lock_
   // TODO Provide some way for apps to set this maybe?
   gState = new AdbConnectionState(kDefaultJdwpAgentName);
   CHECK(gState != nullptr);
-  return true;
+  return ValidateJdwpOptions(art::Runtime::Current()->GetJdwpOptions());
 }
 
 extern "C" bool ArtPlugin_Deinitialize() {