Don't serialize config dictionary in Native Messaging interface.

The config dictionary is no longer serialized when passing
it between the web-app and the native messaging host. It is
still serialized for the NPAPI plugin, however, since it's
difficult to return JavaScript objects from a NPAPI plugin.

BUG=232135

Review URL: https://chromiumcodereview.appspot.com/15623002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@202029 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 70 insertions, 86 deletions

diff --git a/remoting/host/setup/native_messaging_host.cc b/remoting/host/setup/native_messaging_host.cc
index e410f32..7ce09ea 100644
--- a/remoting/host/setup/native_messaging_host.cc
+++ b/remoting/host/setup/native_messaging_host.cc
@@ -8,8 +8,6 @@
 
 #include "base/bind.h"
 #include "base/callback.h"
-#include "base/json/json_reader.h"
-#include "base/json/json_writer.h"
 #include "base/location.h"
 #include "base/strings/stringize_macros.h"
 #include "base/thread_task_runner_handle.h"
@@ -24,23 +22,14 @@ namespace {
 // Returns NULL on failure, and logs an error message.
 scoped_ptr<base::DictionaryValue> ConfigDictionaryFromMessage(
     const base::DictionaryValue& message) {
-  scoped_ptr<base::DictionaryValue> config_dict;
-  std::string config_str;
-  if (!message.GetString("config", &config_str)) {
-    LOG(ERROR) << "'config' not found.";
-    return config_dict.Pass();
-  }
-
-  // TODO(lambroslambrou): Fix the webapp to embed the config dictionary
-  // directly into the request, rather than as a serialized JSON string.
-  scoped_ptr<base::Value> config(
-      base::JSONReader::Read(config_str, base::JSON_ALLOW_TRAILING_COMMAS));
-  if (!config || !config->IsType(base::Value::TYPE_DICTIONARY)) {
-    LOG(ERROR) << "Bad config parameter.";
-    return config_dict.Pass();
+  scoped_ptr<base::DictionaryValue> result;
+  const base::DictionaryValue* config_dict;
+  if (message.GetDictionary("config", &config_dict)) {
+    result.reset(config_dict->DeepCopy());
+  } else {
+    LOG(ERROR) << "'config' dictionary not found";
   }
-  config_dict.reset(reinterpret_cast<base::DictionaryValue*>(config.release()));
-  return config_dict.Pass();
+  return result.Pass();
 }
 
 }  // namespace
@@ -61,8 +50,7 @@ NativeMessagingHost::NativeMessagingHost(
   weak_ptr_ = weak_factory_.GetWeakPtr();
 }
 
-NativeMessagingHost::~NativeMessagingHost() {
-}
+NativeMessagingHost::~NativeMessagingHost() {}
 
 void NativeMessagingHost::Start() {
   DCHECK(caller_task_runner_->BelongsToCurrentThread());
@@ -198,18 +186,18 @@ bool NativeMessagingHost::ProcessUpdateDaemonConfig(
 bool NativeMessagingHost::ProcessGetDaemonConfig(
     const base::DictionaryValue& message,
     scoped_ptr<base::DictionaryValue> response) {
-  daemon_controller_->GetConfig(base::Bind(
-      &NativeMessagingHost::SendConfigResponse,
-      base::Unretained(this), base::Passed(&response)));
+  daemon_controller_->GetConfig(
+      base::Bind(&NativeMessagingHost::SendConfigResponse,
+                 base::Unretained(this), base::Passed(&response)));
   return true;
 }
 
 bool NativeMessagingHost::ProcessGetUsageStatsConsent(
     const base::DictionaryValue& message,
     scoped_ptr<base::DictionaryValue> response) {
-  daemon_controller_->GetUsageStatsConsent(base::Bind(
-      &NativeMessagingHost::SendUsageStatsConsentResponse,
-      base::Unretained(this), base::Passed(&response)));
+  daemon_controller_->GetUsageStatsConsent(
+      base::Bind(&NativeMessagingHost::SendUsageStatsConsentResponse,
+                 base::Unretained(this), base::Passed(&response)));
   return true;
 }
 
@@ -237,9 +225,9 @@ bool NativeMessagingHost::ProcessStartDaemon(
 bool NativeMessagingHost::ProcessStopDaemon(
     const base::DictionaryValue& message,
     scoped_ptr<base::DictionaryValue> response) {
-  daemon_controller_->Stop(base::Bind(
-      &NativeMessagingHost::SendAsyncResult, base::Unretained(this),
-      base::Passed(&response)));
+  daemon_controller_->Stop(
+      base::Bind(&NativeMessagingHost::SendAsyncResult, base::Unretained(this),
+                 base::Passed(&response)));
   return true;
 }
 
@@ -257,9 +245,9 @@ bool NativeMessagingHost::ProcessGetDaemonState(
 void NativeMessagingHost::SendResponse(
     scoped_ptr<base::DictionaryValue> response) {
   if (!caller_task_runner_->BelongsToCurrentThread()) {
-    caller_task_runner_->PostTask(FROM_HERE, base::Bind(
-        &NativeMessagingHost::SendResponse, weak_ptr_,
-        base::Passed(&response)));
+    caller_task_runner_->PostTask(
+        FROM_HERE, base::Bind(&NativeMessagingHost::SendResponse, weak_ptr_,
+                              base::Passed(&response)));
     return;
   }
 
@@ -270,12 +258,7 @@ void NativeMessagingHost::SendResponse(
 void NativeMessagingHost::SendConfigResponse(
     scoped_ptr<base::DictionaryValue> response,
     scoped_ptr<base::DictionaryValue> config) {
-  // TODO(lambroslambrou): Fix the web-app to accept the config dictionary
-  // directly embedded in the response, rather than as serialized JSON. See
-  // http://crbug.com/232135.
-  std::string config_json;
-  base::JSONWriter::Write(config.get(), &config_json);
-  response->SetString("config", config_json);
+  response->Set("config", config.release());
   SendResponse(response.Pass());
 }
 
diff --git a/remoting/webapp/host_controller.js b/remoting/webapp/host_controller.js
index 166017d..a92aec7 100644
--- a/remoting/webapp/host_controller.js
+++ b/remoting/webapp/host_controller.js
@@ -138,14 +138,14 @@ remoting.HostController.prototype.start = function(hostPin, consent, callback) {
    */
   function startHostWithHash(hostName, publicKey, privateKey, xhr,
                              hostSecretHash) {
-    var hostConfig = JSON.stringify({
+    var hostConfig = {
         xmpp_login: remoting.identity.getCachedEmail(),
         oauth_refresh_token: remoting.oauth2.exportRefreshToken(),
         host_id: newHostId,
         host_name: hostName,
         host_secret_hash: hostSecretHash,
         private_key: privateKey
-    });
+    };
     /** @param {remoting.HostController.AsyncResult} result */
     var onStartDaemon = function(result) {
       onStarted(callback, result, hostName, publicKey);
@@ -245,29 +245,14 @@ remoting.HostController.prototype.stop = function(callback) {
 };
 
 /**
- * Parse a stringified host configuration and return it as a dictionary if it
- * is well-formed and contains both host_id and xmpp_login keys.  null is
- * returned if either key is missing, or if the configuration is corrupt.
- * @param {string} configStr The host configuration, JSON encoded to a string.
- * @return {Object.<string,string>|null} The host configuration.
+ * Check the host configuration is valid (non-null, and contains both host_id
+ * and xmpp_login keys).
+ * @param {Object} config The host configuration.
+ * @return {boolean} True if it is valid.
  */
-function parseHostConfig_(configStr) {
-  var config = /** @type {Object.<string,string>} */ jsonParseSafe(configStr);
-  if (config &&
-      typeof config['host_id'] == 'string' &&
-      typeof config['xmpp_login'] == 'string') {
-    return config;
-  } else {
-    // {} means that host is not configured; '' means that the config file could
-    // not be read.
-    // TODO(jamiewalch): '' is expected if the host isn't installed, but should
-    // be reported as an error otherwise. Fix this once we have an event-based
-    // daemon state mechanism.
-    if (configStr != '{}' && configStr != '') {
-      console.error('Invalid getDaemonConfig response.');
-    }
-  }
-  return null;
+function isHostConfigValid_(config) {
+  return !!config && typeof config['host_id'] == 'string' &&
+      typeof config['xmpp_login'] == 'string';
 }
 
 /**
@@ -280,27 +265,27 @@ remoting.HostController.prototype.updatePin = function(newPin, callback) {
   /** @type {remoting.HostController} */
   var that = this;
 
-  /** @param {string} configStr */
-  function onConfig(configStr) {
-    var config = parseHostConfig_(configStr);
-    if (!config) {
+  /** @param {Object} config */
+  function onConfig(config) {
+    if (!isHostConfigValid_(config)) {
       callback(remoting.HostController.AsyncResult.FAILED);
       return;
     }
+    /** @type {string} */
     var hostId = config['host_id'];
     that.hostDispatcher_.getPinHash(hostId, newPin, updateDaemonConfigWithHash);
   }
 
   /** @param {string} pinHash */
   function updateDaemonConfigWithHash(pinHash) {
-    var newConfig = JSON.stringify({
-        host_secret_hash: pinHash
-      });
+    var newConfig = {
+      host_secret_hash: pinHash
+    };
     that.hostDispatcher_.updateDaemonConfig(newConfig, callback);
   }
 
   // TODO(sergeyu): When crbug.com/121518 is fixed: replace this call
-  // with an upriveleged version if that is necessary.
+  // with an unprivileged version if that is necessary.
   this.hostDispatcher_.getDaemonConfig(onConfig);
 };
 
@@ -322,12 +307,11 @@ remoting.HostController.prototype.getLocalHostState = function(onDone) {
 remoting.HostController.prototype.getLocalHostId = function(onDone) {
   /** @type {remoting.HostController} */
   var that = this;
-  /** @param {string} configStr */
-  function onConfig(configStr) {
-    var config = parseHostConfig_(configStr);
+  /** @param {Object} config */
+  function onConfig(config) {
     var hostId = null;
-    if (config) {
-      hostId = config['host_id'];
+    if (isHostConfigValid_(config)) {
+      hostId = /** @type {string} */ config['host_id'];
     }
     onDone(hostId);
   };
diff --git a/remoting/webapp/host_dispatcher.js b/remoting/webapp/host_dispatcher.js
index fe0ba38..41ed241 100644
--- a/remoting/webapp/host_dispatcher.js
+++ b/remoting/webapp/host_dispatcher.js
@@ -128,7 +128,7 @@ remoting.HostDispatcher.prototype.generateKeyPair = function(callback) {
 };
 
 /**
- * @param {string} config
+ * @param {Object} config
  * @param {function(remoting.HostController.AsyncResult):void} callback
  * @return {void}
  */
@@ -142,17 +142,34 @@ remoting.HostDispatcher.prototype.updateDaemonConfig = function(config,
       this.nativeMessagingHost_.updateDaemonConfig(config, callback);
       break;
     case remoting.HostDispatcher.State.NPAPI:
-      this.npapiHost_.updateDaemonConfig(config, callback);
+      this.npapiHost_.updateDaemonConfig(JSON.stringify(config), callback);
       break;
   }
-
 };
 
 /**
- * @param {function(string):void} callback
+ * @param {function(Object):void} callback
  * @return {void}
  */
 remoting.HostDispatcher.prototype.getDaemonConfig = function(callback) {
+  /**
+   * Converts the config string from the NPAPI plugin to an Object, to pass to
+   * |callback|.
+   * @param {string} configStr
+   * @return {void}
+   */
+  function callbackForNpapi(configStr) {
+    var config = null;
+    try {
+      config = JSON.parse(configStr);
+    } catch (err) {}
+    if (typeof(config) != 'object') {
+      // TODO(lambroslambrou): Call error handler here when that's implemented.
+      config = null;
+    }
+    callback(/** @type {Object} */ (config));
+  }
+
   switch (this.state_) {
     case remoting.HostDispatcher.State.UNKNOWN:
       this.pendingRequests_.push(this.getDaemonConfig.bind(this, callback));
@@ -161,7 +178,7 @@ remoting.HostDispatcher.prototype.getDaemonConfig = function(callback) {
       this.nativeMessagingHost_.getDaemonConfig(callback);
       break;
     case remoting.HostDispatcher.State.NPAPI:
-      this.npapiHost_.getDaemonConfig(callback);
+      this.npapiHost_.getDaemonConfig(callbackForNpapi);
       break;
   }
 };
@@ -204,7 +221,7 @@ remoting.HostDispatcher.prototype.getUsageStatsConsent = function(callback) {
 };
 
 /**
- * @param {string} config
+ * @param {Object} config
  * @param {boolean} consent
  * @param {function(remoting.HostController.AsyncResult):void} callback
  * @return {void}
@@ -220,7 +237,7 @@ remoting.HostDispatcher.prototype.startDaemon = function(config, consent,
       this.nativeMessagingHost_.startDaemon(config, consent, callback);
       break;
     case remoting.HostDispatcher.State.NPAPI:
-      this.npapiHost_.startDaemon(config, consent, callback);
+      this.npapiHost_.startDaemon(JSON.stringify(config), consent, callback);
       break;
   }
 };
diff --git a/remoting/webapp/host_native_messaging.js b/remoting/webapp/host_native_messaging.js
index 64c6cd0..8ed628c 100644
--- a/remoting/webapp/host_native_messaging.js
+++ b/remoting/webapp/host_native_messaging.js
@@ -252,7 +252,7 @@ remoting.HostNativeMessaging.prototype.onIncomingMessage_ = function(message) {
     case 'getDaemonConfigResponse':
       /** @type {*} */
       var config = message['config'];
-      if (checkType_('config', config, 'string')) {
+      if (checkType_('config', config, 'object')) {
         callback(config);
       }
       break;
@@ -372,7 +372,7 @@ remoting.HostNativeMessaging.prototype.generateKeyPair = function(callback) {
  * includes these parameters. Changes take effect before the callback
  * is called.
  *
- * @param {string} config The new config parameters, JSON encoded dictionary.
+ * @param {Object} config The new config parameters.
  * @param {function(remoting.HostController.AsyncResult):void} callback
  *     Callback to be called when finished.
  * @return {void} Nothing.
@@ -389,7 +389,7 @@ remoting.HostNativeMessaging.prototype.updateDaemonConfig =
  * Loads daemon config. The config is passed as a JSON formatted string to the
  * callback.
  *
- * @param {function(string):void} callback Callback.
+ * @param {function(Object):void} callback Callback.
  * @return {void} Nothing.
  */
 remoting.HostNativeMessaging.prototype.getDaemonConfig = function(callback) {
@@ -425,7 +425,7 @@ remoting.HostNativeMessaging.prototype.getUsageStatsConsent =
 /**
  * Starts the daemon process with the specified configuration.
  *
- * @param {string} config Host configuration.
+ * @param {Object} config Host configuration.
  * @param {boolean} consent Consent to report crash dumps.
  * @param {function(remoting.HostController.AsyncResult):void} callback
  *     Callback.