Fix a bug where if a PAC script ended with a comment and no newline, it would fail to parse.

The library is now compiled and executed in a separate pass, rather than trying to append the source segments.

BUG=http://crbug.com/22864
TEST=ProxyResolverV8Test.TrailingComment

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

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

2 files changed, 54 insertions, 16 deletions

diff --git a/net/proxy/proxy_resolver_v8.cc b/net/proxy/proxy_resolver_v8.cc
index 214837d..dee799b 100644
--- a/net/proxy/proxy_resolver_v8.cc
+++ b/net/proxy/proxy_resolver_v8.cc
@@ -20,6 +20,8 @@ namespace {
 
 // Pseudo-name for the PAC script.
 const char kPacResourceName[] = "proxy-pac-script.js";
+// Pseudo-name for the PAC utility script.
+const char kPacUtilityResourceName[] = "proxy-pac-utility-script.js";
 
 // Convert a V8 String to a std::string.
 std::string V8StringToStdString(v8::Handle<v8::String> s) {
@@ -130,24 +132,19 @@ class ProxyResolverV8::Context {
 
     v8::Context::Scope ctx(v8_context_);
 
-    v8::TryCatch try_catch;
-
-    // Compile the script, including the PAC library functions.
-    std::string text_raw_utf8 = pac_data_utf8 + PROXY_RESOLVER_SCRIPT;
-    v8::Local<v8::String> text = StdStringToV8String(text_raw_utf8);
-    v8::ScriptOrigin origin = v8::ScriptOrigin(
-        v8::String::New(kPacResourceName));
-    v8::Local<v8::Script> code = v8::Script::Compile(text, &origin);
-
-    // Execute.
-    if (!code.IsEmpty())
-      code->Run();
-
-    if (try_catch.HasCaught()) {
-      HandleError(try_catch.Message());
-      return ERR_PAC_SCRIPT_FAILED;
+    // Add the PAC utility functions to the environment.
+    // (This script should never fail, as it is a string literal!)
+    int rv = RunScript(PROXY_RESOLVER_SCRIPT, kPacUtilityResourceName);
+    if (rv != OK) {
+      NOTREACHED();
+      return rv;
     }
 
+    // Add the user's PAC code to the environment.
+    rv = RunScript(pac_data_utf8, kPacResourceName);
+    if (rv != OK)
+      return rv;
+
     // At a minimum, the FindProxyForURL() function must be defined for this
     // to be a legitimiate PAC script.
     v8::Local<v8::Value> function;
@@ -179,6 +176,29 @@ class ProxyResolverV8::Context {
     js_bindings_->OnError(line_number, error_message);
   }
 
+  // Compiles and runs |script_utf8| in the current V8 context.
+  // Returns OK on success, otherwise an error code.
+  int RunScript(const std::string& script_utf8, const char* script_name) {
+    v8::TryCatch try_catch;
+
+    // Compile the script.
+    v8::Local<v8::String> text = StdStringToV8String(script_utf8);
+    v8::ScriptOrigin origin = v8::ScriptOrigin(v8::String::New(script_name));
+    v8::Local<v8::Script> code = v8::Script::Compile(text, &origin);
+
+    // Execute.
+    if (!code.IsEmpty())
+      code->Run();
+
+    // Check for errors.
+    if (try_catch.HasCaught()) {
+      HandleError(try_catch.Message());
+      return ERR_PAC_SCRIPT_FAILED;
+    }
+
+    return OK;
+  }
+
   // V8 callback for when "alert()" is invoked by the PAC script.
   static v8::Handle<v8::Value> AlertCallback(const v8::Arguments& args) {
     Context* context =
diff --git a/net/proxy/proxy_resolver_v8_unittest.cc b/net/proxy/proxy_resolver_v8_unittest.cc
index 45e0459..de6ea5c 100644
--- a/net/proxy/proxy_resolver_v8_unittest.cc
+++ b/net/proxy/proxy_resolver_v8_unittest.cc
@@ -420,5 +420,23 @@ TEST(ProxyResolverV8Test, LoadLog) {
                     LoadLog::PHASE_END);
 }
 
+// Try loading a PAC script which ends with a trailing comment (no terminal
+// newline). This should not cause problems with the PAC utility functions
+// that we add to the script.
+// http://crbug.com/22864
+TEST(ProxyResolverV8Test, TrailingComment) {
+  ProxyResolverV8WithMockBindings resolver;
+  int result = resolver.SetPacScriptFromDisk("ends_with_comment.js");
+  EXPECT_EQ(OK, result);
+
+  ProxyInfo proxy_info;
+  scoped_refptr<LoadLog> log(new LoadLog);
+  result = resolver.GetProxyForURL(kQueryUrl, &proxy_info, NULL, NULL, log);
+
+  EXPECT_EQ(OK, result);
+  EXPECT_FALSE(proxy_info.is_direct());
+  EXPECT_EQ("success:80", proxy_info.proxy_server().ToURI());
+}
+
 }  // namespace
 }  // namespace net