[ios] Do not JSON encode messages sent from WebView to native code.

ios/web/web_state/ui/crw_web_controller.mm

 // Copyright 2012 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.
 
 #import "ios/web/web_state/ui/crw_web_controller.h"
 
 #import <WebKit/WebKit.h>
 
 #import <objc/runtime.h>
 #include <stddef.h>
 
 #include <cmath>
 #include <memory>
 #include <utility>
 
 #include "base/callback.h"
 #include "base/containers/mru_cache.h"
 #include "base/ios/block_types.h"
 #include "base/ios/ios_util.h"
 #import "base/ios/ns_error_util.h"
 #include "base/ios/weak_nsobject.h"
-#include "base/json/json_reader.h"
 #include "base/json/string_escape.h"
 #include "base/logging.h"
 #include "base/mac/bind_objc_block.h"
 #include "base/mac/bundle_locations.h"
 #include "base/mac/foundation_util.h"
 #include "base/mac/objc_property_releaser.h"
 #include "base/mac/scoped_cftyperef.h"
 #include "base/mac/scoped_nsobject.h"
 #include "base/metrics/histogram.h"
 #include "base/metrics/user_metrics.h"
@@ skipping 2665 matching lines @@
   return [NSString
       stringWithFormat:kTemplate, [_windowIDJSManager windowID], script];
 }
 
 - (BOOL)respondToWKScriptMessage:(WKScriptMessage*)scriptMessage {
   if (!scriptMessage.frameInfo.mainFrame) {
     // Messages from iframes are not currently supported.
     return NO;
   }
 
-  int errorCode = 0;
-  std::string errorMessage;
-  std::unique_ptr<base::Value> inputJSONData(
-      base::JSONReader::ReadAndReturnError(
-          base::SysNSStringToUTF8(scriptMessage.body), false, &errorCode,
-          &errorMessage));
-  if (errorCode) {
-    DLOG(WARNING) << "JSON parse error: %s" << errorMessage.c_str();
-    return NO;
-  }
+  std::unique_ptr<base::Value> messageAsValue =
+      web::ValueResultFromWKResult(scriptMessage.body);
   base::DictionaryValue* message = nullptr;
-  if (!inputJSONData->GetAsDictionary(&message)) {
+  if (!messageAsValue || !messageAsValue->GetAsDictionary(&message)) {
     return NO;
   }
   std::string windowID;
   message->GetString("crwWindowId", &windowID);
   // Check for correct windowID
   if (base::SysNSStringToUTF8([_windowIDJSManager windowID]) != windowID) {
     DLOG(WARNING) << "Message from JS ignored due to non-matching windowID: " <<
         [_windowIDJSManager windowID]
                   << " != " << base::SysUTF8ToNSString(windowID);
     return NO;
@@ skipping 191 matching lines @@
   std::string value;
   int keyCode = web::WebStateObserver::kInvalidFormKeyCode;
   bool inputMissing = false;
   if (!message->GetString("formName", &formName) ||
       !message->GetString("fieldName", &fieldName) ||
       !message->GetString("type", &type) ||
       !message->GetString("value", &value)) {
     inputMissing = true;
   }
 
-  if (!message->GetInteger("keyCode", &keyCode) || keyCode < 0)
+  double keyCodeAsDouble = 0;
+  if (!message->GetDouble("keyCode", &keyCodeAsDouble) || keyCodeAsDouble < 0) {
     keyCode = web::WebStateObserver::kInvalidFormKeyCode;
+  } else {
+    keyCode = static_cast<double>(keyCode);

[sdefresne, 2016/12/21 13:05:38]

I think this should instead be:

    keyCode = static_cast<int>(keyCodeAsDouble);

And then I'm surprised that no test caught this error as keyCode is initialized
to web::WebStateObserver::kInvalidFormKeyCode. Can we add a test that fails when
this code is incorrect (or should we instead remove keyCode?).

[Eugene But (OOO till 7-30), 2016/12/21 15:58:46]

Good catch. I don't think that casting to |int| will change anything here in
terms of program correctness, so I can't think about test with would catch this
issue.

If I understand casting correctly then assigning double to int does implicit
cast anyways. Am I missing something?


Removing |keyCode| will not be possible without code duplication like this:
if (!message->GetDouble("keyCode", &keyCodeAsDouble) || keyCodeAsDouble < 0) {
  _webStateImpl->OnFormActivityRegistered(formName, fieldName, type, value,
web::WebStateObserver::kInvalidFormKeyCode, inputMissing);
} else {
  _webStateImpl->OnFormActivityRegistered(formName, fieldName, type, value,
static_cast<int>(keyCode), inputMissing);
}
And I'm not sure if that's a better way :)
}

+  }
   _webStateImpl->OnFormActivityRegistered(formName, fieldName, type, value,
                                           keyCode, inputMissing);
   return YES;
 }
 
 - (BOOL)handleCredentialsRequestedMessage:(base::DictionaryValue*)message
                                   context:(NSDictionary*)context {
-  int request_id = -1;
-  if (!message->GetInteger("requestId", &request_id)) {
+  double request_id = -1;
+  if (!message->GetDouble("requestId", &request_id)) {
     DLOG(WARNING) << "JS message parameter not found: requestId";
     return NO;
   }
   bool unmediated = false;
   if (!message->GetBoolean("unmediated", &unmediated)) {
     DLOG(WARNING) << "JS message parameter not found: unmediated";
     return NO;
   }
   base::ListValue* federations_value = nullptr;
   if (!message->GetList("federations", &federations_value)) {
     DLOG(WARNING) << "JS message parameter not found: federations";
     return NO;
   }
   std::vector<std::string> federations;
   for (const auto& federation_value : *federations_value) {
     std::string federation;
     if (!federation_value->GetAsString(&federation)) {
       DLOG(WARNING) << "JS message parameter 'federations' contains wrong type";
       return NO;
     }
     federations.push_back(federation);
   }
   DCHECK(context[web::kUserIsInteractingKey]);
   _webStateImpl->OnCredentialsRequested(
-      request_id, net::GURLWithNSURL(context[web::kOriginURLKey]), unmediated,
-      federations, [context[web::kUserIsInteractingKey] boolValue]);
+      static_cast<int>(request_id),
+      net::GURLWithNSURL(context[web::kOriginURLKey]), unmediated, federations,
+      [context[web::kUserIsInteractingKey] boolValue]);
   return YES;
 }
 
 - (BOOL)handleSignedInMessage:(base::DictionaryValue*)message
                       context:(NSDictionary*)context {
-  int request_id = -1;
-  if (!message->GetInteger("requestId", &request_id)) {
+  double request_id = -1;
+  if (!message->GetDouble("requestId", &request_id)) {
     DLOG(WARNING) << "JS message parameter not found: requestId";
     return NO;
   }
   base::DictionaryValue* credential_data = nullptr;
   web::Credential credential;
   if (message->GetDictionary("credential", &credential_data)) {
     if (!web::DictionaryValueToCredential(*credential_data, &credential)) {
       DLOG(WARNING) << "JS message parameter 'credential' is invalid";
       return NO;
     }
-    _webStateImpl->OnSignedIn(request_id,
+    _webStateImpl->OnSignedIn(static_cast<int>(request_id),
                               net::GURLWithNSURL(context[web::kOriginURLKey]),
                               credential);
   } else {
-    _webStateImpl->OnSignedIn(request_id,
+    _webStateImpl->OnSignedIn(static_cast<int>(request_id),
                               net::GURLWithNSURL(context[web::kOriginURLKey]));
   }
   return YES;
 }
 
 - (BOOL)handleSignedOutMessage:(base::DictionaryValue*)message
                        context:(NSDictionary*)context {
-  int request_id = -1;
-  if (!message->GetInteger("requestId", &request_id)) {
+  double request_id = -1;
+  if (!message->GetDouble("requestId", &request_id)) {
     DLOG(WARNING) << "JS message parameter not found: requestId";
     return NO;
   }
-  _webStateImpl->OnSignedOut(request_id,
+  _webStateImpl->OnSignedOut(static_cast<int>(request_id),
                              net::GURLWithNSURL(context[web::kOriginURLKey]));
   return YES;
 }
 
 - (BOOL)handleSignInFailedMessage:(base::DictionaryValue*)message
                           context:(NSDictionary*)context {
-  int request_id = -1;
-  if (!message->GetInteger("requestId", &request_id)) {
+  double request_id = -1;
+  if (!message->GetDouble("requestId", &request_id)) {
     DLOG(WARNING) << "JS message parameter not found: requestId";
     return NO;
   }
   base::DictionaryValue* credential_data = nullptr;
   web::Credential credential;
   if (message->GetDictionary("credential", &credential_data)) {
     if (!web::DictionaryValueToCredential(*credential_data, &credential)) {
       DLOG(WARNING) << "JS message parameter 'credential' is invalid";
       return NO;
     }
     _webStateImpl->OnSignInFailed(
-        request_id, net::GURLWithNSURL(context[web::kOriginURLKey]),
-        credential);
+        static_cast<int>(request_id),
+        net::GURLWithNSURL(context[web::kOriginURLKey]), credential);
   } else {
     _webStateImpl->OnSignInFailed(
-        request_id, net::GURLWithNSURL(context[web::kOriginURLKey]));
+        static_cast<int>(request_id),
+        net::GURLWithNSURL(context[web::kOriginURLKey]));
   }
   return YES;
 }
 
 - (BOOL)handleResetExternalRequestMessage:(base::DictionaryValue*)message
                                   context:(NSDictionary*)context {
   _externalRequest.reset();
   return YES;
 }
 
@@ skipping 40 matching lines @@
 }
 
 - (BOOL)handleWindowHistoryForwardMessage:(base::DictionaryValue*)message
                                   context:(NSDictionary*)context {
   [self goDelta:1];
   return YES;
 }
 
 - (BOOL)handleWindowHistoryGoMessage:(base::DictionaryValue*)message
                              context:(NSDictionary*)context {
-  int delta = 0;
-  if (message->GetInteger("value", &delta)) {
-    [self goDelta:delta];
+  double delta = 0;
+  if (message->GetDouble("value", &delta)) {
+    [self goDelta:static_cast<int>(delta)];
     return YES;
   }
   return NO;
 }
 
 - (BOOL)handleWindowHistoryWillChangeStateMessage:(base::DictionaryValue*)unused
                                           context:(NSDictionary*)unusedContext {
   _changingHistoryState = YES;
   return YES;
 }
@@ skipping 2692 matching lines @@
   }
 
   return web::WEB_VIEW_DOCUMENT_TYPE_GENERIC;
 }
 
 - (NSString*)refererFromNavigationAction:(WKNavigationAction*)action {
   return [action.request valueForHTTPHeaderField:@"Referer"];
 }
 
 @end