Change the V8 proxy resolver bindings so that dnsResolve(XXX) returns null when XXX is not a string.

(The current behavior is to stringize XXX). This is consistent with other browsers.

BUG=42646
TEST=ProxyResolverV8Test.V8indings

Review URL: http://codereview.chromium.org/1954004

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

3 files changed, 32 insertions, 39 deletions

diff --git a/net/data/proxy_resolver_v8_unittest/bindings.js b/net/data/proxy_resolver_v8_unittest/bindings.js
index 3026569..7cf9f26 100644
--- a/net/data/proxy_resolver_v8_unittest/bindings.js
+++ b/net/data/proxy_resolver_v8_unittest/bindings.js
@@ -10,16 +10,25 @@ MyObject.prototype.toString = function() {
   throw "exception from calling toString()";
 }
 
+function expectEquals(expectation, actual) {
+  if (!(expectation === actual)) {
+    throw "FAIL: expected: " + expectation + ", actual: " + actual;
+  }
+}
+
 function FindProxyForURL(url, host) {
   // Call dnsResolve with some wonky arguments.
-  dnsResolve();
-  dnsResolve(null);
-  dnsResolve(undefined);
-  dnsResolve("");
-  dnsResolve({foo: 'bar'});
-  dnsResolve(fn);
-  dnsResolve(['3']);
-  dnsResolve("arg1", "arg2", "arg3", "arg4");
+  // Those expected to fail (because we have passed a non-string parameter)
+  // will return |null|, whereas those that have called through to the C++
+  // bindings will return '127.0.0.1'.
+  expectEquals(null, dnsResolve());
+  expectEquals(null, dnsResolve(null));
+  expectEquals(null, dnsResolve(undefined));
+  expectEquals('127.0.0.1', dnsResolve(""));
+  expectEquals(null, dnsResolve({foo: 'bar'}));
+  expectEquals(null, dnsResolve(fn));
+  expectEquals(null, dnsResolve(['3']));
+  expectEquals('127.0.0.1', dnsResolve("arg1", "arg2", "arg3", "arg4"));
 
   // Call alert with some wonky arguments.
   alert();
diff --git a/net/proxy/proxy_resolver_v8.cc b/net/proxy/proxy_resolver_v8.cc
index 3fb01c2b..3366a56 100644
--- a/net/proxy/proxy_resolver_v8.cc
+++ b/net/proxy/proxy_resolver_v8.cc
@@ -332,14 +332,10 @@ class ProxyResolverV8::Context {
     Context* context =
         static_cast<Context*>(v8::External::Cast(*args.Data())->Value());
 
-    // We need at least one argument.
-    std::string host;
-    if (args.Length() == 0) {
-      host = "undefined";
-    } else {
-      if (!V8ObjectToString(args[0], &host))
-        return v8::Undefined();
-    }
+    // We need at least one string argument.
+    if (args.Length() == 0 || args[0].IsEmpty() || !args[0]->IsString())
+      return v8::Null();
+    std::string host = V8StringToStdString(args[0]->ToString());
 
     context->current_request_net_log_.BeginEvent(
         NetLog::TYPE_PROXY_RESOLVER_V8_DNS_RESOLVE, NULL);
@@ -358,14 +354,10 @@ class ProxyResolverV8::Context {
     Context* context =
         static_cast<Context*>(v8::External::Cast(*args.Data())->Value());
 
-    // We need at least one argument.
-    std::string host;
-    if (args.Length() == 0) {
-      host = "undefined";
-    } else {
-      if (!V8ObjectToString(args[0], &host))
-        return v8::Undefined();
-    }
+    // We need at least one string argument.
+    if (args.Length() == 0 || args[0].IsEmpty() || !args[0]->IsString())
+      return v8::Undefined();
+    std::string host = V8StringToStdString(args[0]->ToString());
 
     context->current_request_net_log_.BeginEvent(
         NetLog::TYPE_PROXY_RESOLVER_V8_DNS_RESOLVE_EX, NULL);
diff --git a/net/proxy/proxy_resolver_v8_unittest.cc b/net/proxy/proxy_resolver_v8_unittest.cc
index e46b1cb..45e6e25 100644
--- a/net/proxy/proxy_resolver_v8_unittest.cc
+++ b/net/proxy/proxy_resolver_v8_unittest.cc
@@ -383,6 +383,8 @@ TEST(ProxyResolverV8Test, NoSetPacScript) {
 // Test marshalling/un-marshalling of values between C++/V8.
 TEST(ProxyResolverV8Test, V8Bindings) {
   ProxyResolverV8WithMockBindings resolver;
+  MockJSBindings* bindings = resolver.mock_js_bindings();
+  bindings->dns_resolve_result = "127.0.0.1";
   int result = resolver.SetPacScriptFromDisk("bindings.js");
   EXPECT_EQ(OK, result);
 
@@ -393,7 +395,6 @@ TEST(ProxyResolverV8Test, V8Bindings) {
   EXPECT_EQ(OK, result);
   EXPECT_TRUE(proxy_info.is_direct());
 
-  MockJSBindings* bindings = resolver.mock_js_bindings();
   EXPECT_EQ(0U, resolver.mock_js_bindings()->errors.size());
 
   // Alert was called 5 times.
@@ -404,20 +405,11 @@ TEST(ProxyResolverV8Test, V8Bindings) {
   EXPECT_EQ("[object Object]", bindings->alerts[3]);
   EXPECT_EQ("exception from calling toString()", bindings->alerts[4]);
 
-  // DnsResolve was called 8 times.
-  ASSERT_EQ(8U, bindings->dns_resolves.size());
-  EXPECT_EQ("undefined", bindings->dns_resolves[0]);
-  EXPECT_EQ("null", bindings->dns_resolves[1]);
-  EXPECT_EQ("undefined", bindings->dns_resolves[2]);
-  EXPECT_EQ("", bindings->dns_resolves[3]);
-  EXPECT_EQ("[object Object]", bindings->dns_resolves[4]);
-  EXPECT_EQ("function fn() {}", bindings->dns_resolves[5]);
-
-  // TODO(eroman): This isn't quite right... should probably stringize
-  // to something like "['3']".
-  EXPECT_EQ("3", bindings->dns_resolves[6]);
-
-  EXPECT_EQ("arg1", bindings->dns_resolves[7]);
+  // DnsResolve was called 8 times, however only 2 of those were string
+  // parameters. (so 6 of them failed immediately).
+  ASSERT_EQ(2U, bindings->dns_resolves.size());
+  EXPECT_EQ("", bindings->dns_resolves[0]);
+  EXPECT_EQ("arg1", bindings->dns_resolves[1]);
 
   // MyIpAddress was called two times.
   EXPECT_EQ(2, bindings->my_ip_address_count);