Fix a crash when processing an invalid PAC script.

net/proxy/proxy_resolver_v8.cc

Index: net/proxy/proxy_resolver_v8.cc
diff --git a/net/proxy/proxy_resolver_v8.cc b/net/proxy/proxy_resolver_v8.cc
index 693fbb24837ff84b01f7ffd4bd5018c6e3b77944..559b1a648a7e66c7f413d929507d3673d9e9f0c8 100644
--- a/net/proxy/proxy_resolver_v8.cc
+++ b/net/proxy/proxy_resolver_v8.cc
@@ -379,11 +379,9 @@ class ProxyResolverV8::Context {
     v8::Context::Scope function_scope(context);
 
     v8::Local<v8::Value> function;
-    if (!GetFindProxyForURL(&function)) {
-      js_bindings()->OnError(
-          -1, base::ASCIIToUTF16("FindProxyForURL() is undefined."));
-      return ERR_PAC_SCRIPT_FAILED;
-    }
+    int rv = GetFindProxyForURL(&function);
+    if (rv != OK)
+      return rv;
 
     v8::Local<v8::Value> argv[] = {
       ASCIIStringToV8String(isolate_, query_url.spec()),
@@ -509,23 +507,38 @@ class ProxyResolverV8::Context {
     // At a minimum, the FindProxyForURL() function must be defined for this
     // to be a legitimiate PAC script.
     v8::Local<v8::Value> function;
-    if (!GetFindProxyForURL(&function)) {
-      js_bindings()->OnError(
-          -1, base::ASCIIToUTF16("FindProxyForURL() is undefined."));
-      return ERR_PAC_SCRIPT_FAILED;
-    }
-
-    return OK;
+    return GetFindProxyForURL(&function);
   }
 
  private:
-  bool GetFindProxyForURL(v8::Local<v8::Value>* function) {
+  int GetFindProxyForURL(v8::Local<v8::Value>* function) {
     v8::Local<v8::Context> context =
         v8::Local<v8::Context>::New(isolate_, v8_context_);
+
+    v8::TryCatch try_catch;
+
     *function =
         context->Global()->Get(
             ASCIILiteralToV8String(isolate_, "FindProxyForURL"));
-    return (*function)->IsFunction();
+
+    if (try_catch.HasCaught())
+      HandleError(try_catch.Message());
+
+    if (function->IsEmpty()) {
+      js_bindings()->OnError(
+          -1,
+          base::ASCIIToUTF16("Accessing FindProxyForURL threw an exception."));

[mmenke, 2015/02/25 17:50:43]

Is it possible for try_catch.HasCaught() != function->IsEmpty()?  It seems weird
to have code to catch a thrown errors in a variable...and then use a completely
different check to see if we actually caught something.

If it is possible, should have a test.  If not, should fix this code - either
just check one variable and maybe DCHECK on the other, or make the shared error
message more general.

[eroman, 2015/02/25 18:07:26]

I do not know if it is possible for function->IsEmpty() when
!try_catch.HasCaught(). Perhaps Jakob can answer that.

I think it is reasonable though to test them individually as an extra
precaution:
  - The TryCatch::HasCaught() to print the exception error messsage
  - The function->IsEmpty() used to avoid null dereference

[mmenke, 2015/02/25 18:44:51]

I'm fine with handling them both, but this code still seems weird.  Suggest
either:

if (HasCaught() || function->IsEmpty()) {
  if (HasCaught())
     ..
  ..
  return ERR_PAC_SCRIPT_FAILED;
}

or

if (HasCaught()) {
  ...
  return ERR_PAC_SCRIPT_FAILED;
}

if (IsEmpty()) {
  // Log error that does *not* claim an exception was thrown.
  // "Unable to get FindProxyForURL"?
  return ERR_PAC_SCRIPT_FAILED;
}

+      return ERR_PAC_SCRIPT_FAILED;
+    }
+
+    if (!(*function)->IsFunction()) {
+      js_bindings()->OnError(
+          -1, base::ASCIIToUTF16(
+                  "FindProxyForURL is undefined or not a function."));
+      return ERR_PAC_SCRIPT_FAILED;
+    }
+
+    return OK;
   }
 
   // Handle an exception thrown by V8.