Convert JS execute methods to return Optional<IJS_Runtime::JS_Error>

This CL changes several of the JS execution methods to to return an
Optional<IJS_Runtime::JS_Error> instead of a bool with a WideString
out param.

The IJS_Runtime::JS_Error will contain the line, column and exception message
if an error occurs during execution.

Change-Id: I37785ae6cd133a4c94ad8d25289473600b8a5d19
Reviewed-on: https://pdfium-review.googlesource.com/32614
Commit-Queue: dsinclair <dsinclair@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>

16 files changed, 105 insertions, 59 deletions

diff --git a/fxjs/cfxjs_engine.cpp b/fxjs/cfxjs_engine.cpp
index 5def57e992..107ed3abae 100644
--- a/fxjs/cfxjs_engine.cpp
+++ b/fxjs/cfxjs_engine.cpp
@@ -520,7 +520,8 @@ void CFXJS_Engine::ReleaseEngine() {
   GetIsolate()->SetData(g_embedderDataSlot, nullptr);
 }
 
-int CFXJS_Engine::Execute(const WideString& script, FXJSErr* pError) {
+Optional<IJS_Runtime::JS_Error> CFXJS_Engine::Execute(
+    const WideString& script) {
   v8::Isolate::Scope isolate_scope(GetIsolate());
   v8::TryCatch try_catch(GetIsolate());
   v8::Local<v8::Context> context = GetIsolate()->GetCurrentContext();
@@ -528,17 +529,22 @@ int CFXJS_Engine::Execute(const WideString& script, FXJSErr* pError) {
   if (!v8::Script::Compile(context, NewString(script.AsStringView()))
            .ToLocal(&compiled_script)) {
     v8::String::Utf8Value error(GetIsolate(), try_catch.Exception());
-    // TODO(tsepez): return error via pError->message.
-    return -1;
+    v8::Local<v8::Message> msg = try_catch.Message();
+    v8::Maybe<int> line = msg->GetLineNumber(context);
+
+    return IJS_Runtime::JS_Error(line.FromMaybe(-1), msg->GetStartColumn(),
+                                 WideString::FromUTF8(*error));
   }
 
   v8::Local<v8::Value> result;
   if (!compiled_script->Run(context).ToLocal(&result)) {
     v8::String::Utf8Value error(GetIsolate(), try_catch.Exception());
-    // TODO(tsepez): return error via pError->message.
-    return -1;
+    auto msg = try_catch.Message();
+    auto line = msg->GetLineNumber(context);
+    return IJS_Runtime::JS_Error(line.FromMaybe(-1), msg->GetStartColumn(),
+                                 WideString::FromUTF8(*error));
   }
-  return 0;
+  return pdfium::nullopt;
 }
 
 v8::Local<v8::Object> CFXJS_Engine::NewFXJSBoundObject(int nObjDefnID,
diff --git a/fxjs/cfxjs_engine.h b/fxjs/cfxjs_engine.h
index 4f903bd96e..d1fb70cc28 100644
--- a/fxjs/cfxjs_engine.h
+++ b/fxjs/cfxjs_engine.h
@@ -21,6 +21,7 @@
 
 #include "core/fxcrt/fx_string.h"
 #include "fxjs/cfx_v8.h"
+#include "fxjs/ijs_runtime.h"
 #include "v8/include/v8-util.h"
 #include "v8/include/v8.h"
 
@@ -43,12 +44,6 @@ enum FXJSOBJTYPE {
   FXJSOBJTYPE_GLOBAL,       // The global object itself (may only appear once).
 };
 
-struct FXJSErr {
-  const wchar_t* message;
-  const wchar_t* srcline;
-  unsigned linnum;
-};
-
 class FXJS_PerIsolateData {
  public:
   ~FXJS_PerIsolateData();
@@ -128,7 +123,7 @@ class CFXJS_Engine : public CFX_V8 {
   void ReleaseEngine();
 
   // Called after FXJS_InitializeEngine call made.
-  int Execute(const WideString& script, FXJSErr* perror);
+  Optional<IJS_Runtime::JS_Error> Execute(const WideString& script);
 
   v8::Local<v8::Object> GetThisObj();
   v8::Local<v8::Object> NewFXJSBoundObject(int nObjDefnID,
diff --git a/fxjs/cfxjs_engine_embeddertest.cpp b/fxjs/cfxjs_engine_embeddertest.cpp
index bcd4183de0..75f07982aa 100644
--- a/fxjs/cfxjs_engine_embeddertest.cpp
+++ b/fxjs/cfxjs_engine_embeddertest.cpp
@@ -21,13 +21,13 @@ const wchar_t kScript2[] = L"fred = 8";
 
 class CFXJSEngineEmbedderTest : public JSEmbedderTest {
  public:
-  void ExecuteInCurrentContext(const WideString& script) {
+  Optional<IJS_Runtime::JS_Error> ExecuteInCurrentContext(
+      const WideString& script) {
     auto* current_engine =
         CFXJS_Engine::EngineFromIsolateCurrentContext(isolate());
-    FXJSErr error;
-    int sts = current_engine->Execute(script, &error);
-    EXPECT_EQ(0, sts);
+    return current_engine->Execute(script);
   }
+
   void CheckAssignmentInCurrentContext(double expected) {
     auto* current_engine =
         CFXJS_Engine::EngineFromIsolateCurrentContext(isolate());
@@ -44,7 +44,9 @@ TEST_F(CFXJSEngineEmbedderTest, Getters) {
   v8::HandleScope handle_scope(isolate());
   v8::Context::Scope context_scope(GetV8Context());
 
-  ExecuteInCurrentContext(WideString(kScript1));
+  Optional<IJS_Runtime::JS_Error> err =
+      ExecuteInCurrentContext(WideString(kScript1));
+  EXPECT_FALSE(err);
   CheckAssignmentInCurrentContext(kExpected1);
 }
 
@@ -62,17 +64,23 @@ TEST_F(CFXJSEngineEmbedderTest, MultipleEngines) {
   v8::Local<v8::Context> context2 = engine2.GetV8Context();
 
   v8::Context::Scope context_scope(GetV8Context());
-  ExecuteInCurrentContext(WideString(kScript0));
+  Optional<IJS_Runtime::JS_Error> err =
+      ExecuteInCurrentContext(WideString(kScript0));
+  EXPECT_FALSE(err);
   CheckAssignmentInCurrentContext(kExpected0);
 
   {
     v8::Context::Scope context_scope1(context1);
-    ExecuteInCurrentContext(WideString(kScript1));
+    Optional<IJS_Runtime::JS_Error> err =
+        ExecuteInCurrentContext(WideString(kScript1));
+    EXPECT_FALSE(err);
     CheckAssignmentInCurrentContext(kExpected1);
   }
   {
     v8::Context::Scope context_scope2(context2);
-    ExecuteInCurrentContext(WideString(kScript2));
+    Optional<IJS_Runtime::JS_Error> err =
+        ExecuteInCurrentContext(WideString(kScript2));
+    EXPECT_FALSE(err);
     CheckAssignmentInCurrentContext(kExpected2);
   }
 
@@ -102,3 +110,30 @@ TEST_F(CFXJSEngineEmbedderTest, MultipleEngines) {
   engine1.ReleaseEngine();
   engine2.ReleaseEngine();
 }
+
+TEST_F(CFXJSEngineEmbedderTest, JSCompileError) {
+  v8::Isolate::Scope isolate_scope(isolate());
+  v8::HandleScope handle_scope(isolate());
+  v8::Context::Scope context_scope(GetV8Context());
+
+  Optional<IJS_Runtime::JS_Error> err =
+      ExecuteInCurrentContext(L"functoon(x) { return x+1; }");
+  EXPECT_TRUE(err);
+  EXPECT_EQ(L"SyntaxError: Unexpected token {", err->exception);
+  EXPECT_EQ(1, err->line);
+  EXPECT_EQ(12, err->column);
+}
+
+TEST_F(CFXJSEngineEmbedderTest, JSRuntimeError) {
+  v8::Isolate::Scope isolate_scope(isolate());
+  v8::HandleScope handle_scope(isolate());
+  v8::Context::Scope context_scope(GetV8Context());
+
+  Optional<IJS_Runtime::JS_Error> err =
+      ExecuteInCurrentContext(L"let a = 3;\nundefined.colour");
+  EXPECT_TRUE(err);
+  EXPECT_EQ(L"TypeError: Cannot read property 'colour' of undefined",
+            err->exception);
+  EXPECT_EQ(2, err->line);
+  EXPECT_EQ(10, err->column);
+}
diff --git a/fxjs/cfxjs_engine_unittest.cpp b/fxjs/cfxjs_engine_unittest.cpp
index d5c473c178..5b93072935 100644
--- a/fxjs/cfxjs_engine_unittest.cpp
+++ b/fxjs/cfxjs_engine_unittest.cpp
@@ -79,8 +79,9 @@ TEST_F(FXJSEngineUnitTest, GC) {
     EXPECT_FALSE(temp_destroyed);
   }
 
-  FXJSErr error;
-  engine()->Execute(L"gc();", &error);
+  Optional<IJS_Runtime::JS_Error> err = engine()->Execute(L"gc();");
+  EXPECT_FALSE(err);
+
   EXPECT_TRUE(perm_created);
   EXPECT_FALSE(perm_destroyed);
   EXPECT_TRUE(temp_created);
diff --git a/fxjs/cjs_app.cpp b/fxjs/cjs_app.cpp
index 020bc5662f..4e21ed994b 100644
--- a/fxjs/cjs_app.cpp
+++ b/fxjs/cjs_app.cpp
@@ -415,8 +415,7 @@ void CJS_App::RunJsScript(CJS_Runtime* pRuntime, const WideString& wsScript) {
   if (!pRuntime->IsBlocking()) {
     IJS_EventContext* pContext = pRuntime->NewEventContext();
     pContext->OnExternal_Exec();
-    WideString wtInfo;
-    pContext->RunScript(wsScript, &wtInfo);
+    pContext->RunScript(wsScript);
     pRuntime->ReleaseEventContext(pContext);
   }
 }
diff --git a/fxjs/cjs_event_context.cpp b/fxjs/cjs_event_context.cpp
index 195fb1b888..0d9ba34db1 100644
--- a/fxjs/cjs_event_context.cpp
+++ b/fxjs/cjs_event_context.cpp
@@ -25,15 +25,16 @@ CPDFSDK_FormFillEnvironment* CJS_EventContext::GetFormFillEnv() {
   return m_pRuntime->GetFormFillEnv();
 }
 
-bool CJS_EventContext::RunScript(const WideString& script, WideString* info) {
+Optional<IJS_Runtime::JS_Error> CJS_EventContext::RunScript(
+    const WideString& script) {
   v8::Isolate::Scope isolate_scope(m_pRuntime->GetIsolate());
   v8::HandleScope handle_scope(m_pRuntime->GetIsolate());
   v8::Local<v8::Context> context = m_pRuntime->GetV8Context();
   v8::Context::Scope context_scope(context);
 
   if (m_bBusy) {
-    *info = JSGetStringFromID(JSMessage::kBusyError);
-    return false;
+    return IJS_Runtime::JS_Error(1, 1,
+                                 JSGetStringFromID(JSMessage::kBusyError));
   }
 
   AutoRestorer<bool> restorer(&m_bBusy);
@@ -43,23 +44,17 @@ bool CJS_EventContext::RunScript(const WideString& script, WideString* info) {
   CJS_Runtime::FieldEvent event(m_pEventHandler->TargetName(),
                                 m_pEventHandler->EventType());
   if (!m_pRuntime->AddEventToSet(event)) {
-    *info = JSGetStringFromID(JSMessage::kDuplicateEventError);
-    return false;
+    return IJS_Runtime::JS_Error(
+        1, 1, JSGetStringFromID(JSMessage::kDuplicateEventError));
   }
 
-  WideString sErrorMessage;
-  int nRet = 0;
+  Optional<IJS_Runtime::JS_Error> err;
   if (script.GetLength() > 0)
-    nRet = m_pRuntime->ExecuteScript(script.c_str(), &sErrorMessage);
-
-  if (nRet < 0)
-    *info += sErrorMessage;
-  else
-    *info = JSGetStringFromID(JSMessage::kRunSuccess);
+    err = m_pRuntime->ExecuteScript(script.c_str());
 
   m_pRuntime->RemoveEventFromSet(event);
   m_pEventHandler->Destroy();
-  return nRet >= 0;
+  return err;
 }
 
 void CJS_EventContext::OnApp_Init() {
diff --git a/fxjs/cjs_event_context.h b/fxjs/cjs_event_context.h
index f9572909e8..3cfc6da775 100644
--- a/fxjs/cjs_event_context.h
+++ b/fxjs/cjs_event_context.h
@@ -24,7 +24,7 @@ class CJS_EventContext : public IJS_EventContext {
   ~CJS_EventContext() override;
 
   // IJS_EventContext
-  bool RunScript(const WideString& script, WideString* info) override;
+  Optional<IJS_Runtime::JS_Error> RunScript(const WideString& script) override;
   void OnApp_Init() override;
   void OnDoc_Open(CPDFSDK_FormFillEnvironment* pFormFillEnv,
                   const WideString& strTargetName) override;
diff --git a/fxjs/cjs_event_context_stub.cpp b/fxjs/cjs_event_context_stub.cpp
index 0517ab2c49..82530e4c7b 100644
--- a/fxjs/cjs_event_context_stub.cpp
+++ b/fxjs/cjs_event_context_stub.cpp
@@ -6,7 +6,7 @@
 
 #include "fxjs/cjs_event_context_stub.h"
 
-bool CJS_EventContextStub::RunScript(const WideString& script,
-                                     WideString* info) {
-  return false;
+Optional<IJS_Runtime::JS_Error> CJS_EventContextStub::RunScript(
+    const WideString& script) {
+  return IJS_Runtime::JS_Error(1, 1, L"JavaScript support not present");
 }
diff --git a/fxjs/cjs_event_context_stub.h b/fxjs/cjs_event_context_stub.h
index bc853694df..c8c5e33ac3 100644
--- a/fxjs/cjs_event_context_stub.h
+++ b/fxjs/cjs_event_context_stub.h
@@ -15,7 +15,7 @@ class CJS_EventContextStub final : public IJS_EventContext {
   ~CJS_EventContextStub() override {}
 
   // IJS_EventContext:
-  bool RunScript(const WideString& script, WideString* info) override;
+  Optional<IJS_Runtime::JS_Error> RunScript(const WideString& script) override;
 
   void OnApp_Init() override {}
   void OnDoc_Open(CPDFSDK_FormFillEnvironment* pFormFillEnv,
diff --git a/fxjs/cjs_runtime.cpp b/fxjs/cjs_runtime.cpp
index f6e7b2d18a..b47c595350 100644
--- a/fxjs/cjs_runtime.cpp
+++ b/fxjs/cjs_runtime.cpp
@@ -190,14 +190,9 @@ CPDFSDK_FormFillEnvironment* CJS_Runtime::GetFormFillEnv() const {
   return m_pFormFillEnv.Get();
 }
 
-int CJS_Runtime::ExecuteScript(const WideString& script, WideString* info) {
-  FXJSErr error = {};
-  int nRet = Execute(script, &error);
-  if (nRet < 0) {
-    *info = WideString::Format(L"[ Line: %05d { %ls } ] : %s", error.linnum - 1,
-                               error.srcline, error.message);
-  }
-  return nRet;
+Optional<IJS_Runtime::JS_Error> CJS_Runtime::ExecuteScript(
+    const WideString& script) {
+  return Execute(script);
 }
 
 bool CJS_Runtime::AddEventToSet(const FieldEvent& event) {
diff --git a/fxjs/cjs_runtime.h b/fxjs/cjs_runtime.h
index c5e69fbdc8..0c32562304 100644
--- a/fxjs/cjs_runtime.h
+++ b/fxjs/cjs_runtime.h
@@ -37,7 +37,8 @@ class CJS_Runtime : public IJS_Runtime,
   IJS_EventContext* NewEventContext() override;
   void ReleaseEventContext(IJS_EventContext* pContext) override;
   CPDFSDK_FormFillEnvironment* GetFormFillEnv() const override;
-  int ExecuteScript(const WideString& script, WideString* info) override;
+  Optional<IJS_Runtime::JS_Error> ExecuteScript(
+      const WideString& script) override;
 
   CJS_EventContext* GetCurrentEventContext() const;
 
diff --git a/fxjs/cjs_runtimestub.cpp b/fxjs/cjs_runtimestub.cpp
index c1e680e5d5..44bdbca884 100644
--- a/fxjs/cjs_runtimestub.cpp
+++ b/fxjs/cjs_runtimestub.cpp
@@ -42,6 +42,7 @@ bool CJS_RuntimeStub::SetValueByNameInGlobalObject(const ByteStringView&,
 }
 #endif  // PDF_ENABLE_XFA
 
-int CJS_RuntimeStub::ExecuteScript(const WideString& script, WideString* info) {
-  return 0;
+Optional<IJS_Runtime::JS_Error> CJS_RuntimeStub::ExecuteScript(
+    const WideString& script) {
+  return pdfium::nullopt;
 }
diff --git a/fxjs/cjs_runtimestub.h b/fxjs/cjs_runtimestub.h
index a9e85fdf03..cc32744736 100644
--- a/fxjs/cjs_runtimestub.h
+++ b/fxjs/cjs_runtimestub.h
@@ -33,7 +33,8 @@ class CJS_RuntimeStub final : public IJS_Runtime {
                                     CFXJSE_Value*) override;
 #endif  // PDF_ENABLE_XFA
 
-  int ExecuteScript(const WideString& script, WideString* info) override;
+  Optional<IJS_Runtime::JS_Error> ExecuteScript(
+      const WideString& script) override;
 
  protected:
   UnownedPtr<CPDFSDK_FormFillEnvironment> const m_pFormFillEnv;
diff --git a/fxjs/ijs_event_context.h b/fxjs/ijs_event_context.h
index 6050d65eca..8317bc29be 100644
--- a/fxjs/ijs_event_context.h
+++ b/fxjs/ijs_event_context.h
@@ -9,6 +9,8 @@
 
 #include "core/fxcrt/fx_string.h"
 #include "core/fxcrt/fx_system.h"
+#include "fxjs/ijs_runtime.h"
+#include "third_party/base/optional.h"
 
 class CPDF_Bookmark;
 class CPDF_FormField;
@@ -22,7 +24,8 @@ class IJS_EventContext {
  public:
   virtual ~IJS_EventContext() {}
 
-  virtual bool RunScript(const WideString& script, WideString* info) = 0;
+  virtual Optional<IJS_Runtime::JS_Error> RunScript(
+      const WideString& script) = 0;
 
   virtual void OnApp_Init() = 0;
 
diff --git a/fxjs/ijs_runtime.cpp b/fxjs/ijs_runtime.cpp
index 3f4bcd2497..afbdaefca4 100644
--- a/fxjs/ijs_runtime.cpp
+++ b/fxjs/ijs_runtime.cpp
@@ -38,3 +38,8 @@ std::unique_ptr<IJS_Runtime> IJS_Runtime::Create(
 }
 
 IJS_Runtime::~IJS_Runtime() = default;
+
+IJS_Runtime::JS_Error::JS_Error(int line,
+                                int column,
+                                const WideString& exception)
+    : line(line), column(column), exception(exception) {}
diff --git a/fxjs/ijs_runtime.h b/fxjs/ijs_runtime.h
index cde31c6e52..a21aae86de 100644
--- a/fxjs/ijs_runtime.h
+++ b/fxjs/ijs_runtime.h
@@ -11,6 +11,7 @@
 
 #include "core/fxcrt/fx_string.h"
 #include "core/fxcrt/fx_system.h"
+#include "third_party/base/optional.h"
 
 #ifdef PDF_ENABLE_XFA
 #include "fxjs/fxjse.h"
@@ -25,6 +26,14 @@ class IJS_EventContext;
 // when JS is not present.
 class IJS_Runtime {
  public:
+  struct JS_Error {
+    int line;
+    int column;
+    WideString exception;
+
+    JS_Error(int line, int column, const WideString& exception);
+  };
+
   static void Initialize(unsigned int slot, void* isolate);
   static void Destroy();
   static std::unique_ptr<IJS_Runtime> Create(
@@ -35,7 +44,7 @@ class IJS_Runtime {
   virtual IJS_EventContext* NewEventContext() = 0;
   virtual void ReleaseEventContext(IJS_EventContext* pContext) = 0;
   virtual CPDFSDK_FormFillEnvironment* GetFormFillEnv() const = 0;
-  virtual int ExecuteScript(const WideString& script, WideString* info) = 0;
+  virtual Optional<JS_Error> ExecuteScript(const WideString& script) = 0;
 
 #ifdef PDF_ENABLE_XFA
   virtual bool GetValueByNameFromGlobalObject(const ByteStringView& utf8Name,
@@ -45,7 +54,7 @@ class IJS_Runtime {
 #endif  // PDF_ENABLE_XFA
 
  protected:
-  IJS_Runtime() {}
+  IJS_Runtime() = default;
 };
 
 #endif  // FXJS_IJS_RUNTIME_H_