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>
diff --git a/fpdfsdk/cpdfsdk_actionhandler.cpp b/fpdfsdk/cpdfsdk_actionhandler.cpp
index 98c0124..660b09c 100644
--- a/fpdfsdk/cpdfsdk_actionhandler.cpp
+++ b/fpdfsdk/cpdfsdk_actionhandler.cpp
@@ -544,8 +544,7 @@
cb(pContext);
- WideString csInfo;
- pContext->RunScript(script, &csInfo);
+ pContext->RunScript(script);
pRuntime->ReleaseEventContext(pContext);
- // TODO(dsinclair): Return error if RunScript returns false.
+ // TODO(dsinclair): Return error if RunScript returns a IJS_Runtime::JS_Error.
}
diff --git a/fpdfsdk/cpdfsdk_interform.cpp b/fpdfsdk/cpdfsdk_interform.cpp
index 4f11bab..b37562c 100644
--- a/fpdfsdk/cpdfsdk_interform.cpp
+++ b/fpdfsdk/cpdfsdk_interform.cpp
@@ -291,10 +291,9 @@
bool bRC = true;
pContext->OnField_Calculate(pFormField, pField, sValue, bRC);
- WideString sInfo;
- bool bRet = pContext->RunScript(csJS, &sInfo);
+ Optional<IJS_Runtime::JS_Error> err = pContext->RunScript(csJS);
pRuntime->ReleaseEventContext(pContext);
- if (bRet && bRC && sValue.Compare(sOldValue) != 0)
+ if (!err && bRC && sValue.Compare(sOldValue) != 0)
pField->SetValue(sValue, true);
}
m_bBusy = false;
@@ -328,10 +327,10 @@
IJS_EventContext* pContext = pRuntime->NewEventContext();
pContext->OnField_Format(pFormField, Value, true);
- WideString sInfo;
- bool bRet = pContext->RunScript(script, &sInfo);
+
+ Optional<IJS_Runtime::JS_Error> err = pContext->RunScript(script);
pRuntime->ReleaseEventContext(pContext);
- if (bRet) {
+ if (!err) {
sValue = Value;
bFormatted = true;
}
diff --git a/fxjs/cfxjs_engine.cpp b/fxjs/cfxjs_engine.cpp
index 5def57e..107ed3a 100644
--- a/fxjs/cfxjs_engine.cpp
+++ b/fxjs/cfxjs_engine.cpp
@@ -520,7 +520,8 @@
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 @@
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 4f903bd..d1fb70c 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 @@
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 @@
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 bcd4183..75f0798 100644
--- a/fxjs/cfxjs_engine_embeddertest.cpp
+++ b/fxjs/cfxjs_engine_embeddertest.cpp
@@ -21,13 +21,13 @@
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 @@
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 @@
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 @@
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 d5c473c..5b93072 100644
--- a/fxjs/cfxjs_engine_unittest.cpp
+++ b/fxjs/cfxjs_engine_unittest.cpp
@@ -79,8 +79,9 @@
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 020bc56..4e21ed9 100644
--- a/fxjs/cjs_app.cpp
+++ b/fxjs/cjs_app.cpp
@@ -415,8 +415,7 @@
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 195fb1b..0d9ba34 100644
--- a/fxjs/cjs_event_context.cpp
+++ b/fxjs/cjs_event_context.cpp
@@ -25,15 +25,16 @@
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 @@
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 f957290..3cfc6da 100644
--- a/fxjs/cjs_event_context.h
+++ b/fxjs/cjs_event_context.h
@@ -24,7 +24,7 @@
~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 0517ab2..82530e4 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 bc85369..c8c5e33 100644
--- a/fxjs/cjs_event_context_stub.h
+++ b/fxjs/cjs_event_context_stub.h
@@ -15,7 +15,7 @@
~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 f6e7b2d..b47c595 100644
--- a/fxjs/cjs_runtime.cpp
+++ b/fxjs/cjs_runtime.cpp
@@ -190,14 +190,9 @@
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 c5e69fb..0c32562 100644
--- a/fxjs/cjs_runtime.h
+++ b/fxjs/cjs_runtime.h
@@ -37,7 +37,8 @@
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 c1e680e..44bdbca 100644
--- a/fxjs/cjs_runtimestub.cpp
+++ b/fxjs/cjs_runtimestub.cpp
@@ -42,6 +42,7 @@
}
#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 a9e85fd..cc32744 100644
--- a/fxjs/cjs_runtimestub.h
+++ b/fxjs/cjs_runtimestub.h
@@ -33,7 +33,8 @@
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 6050d65..8317bc2 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 @@
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 3f4bcd2..afbdaef 100644
--- a/fxjs/ijs_runtime.cpp
+++ b/fxjs/ijs_runtime.cpp
@@ -38,3 +38,8 @@
}
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 cde31c6..a21aae8 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 @@
// 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 @@
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 @@
#endif // PDF_ENABLE_XFA
protected:
- IJS_Runtime() {}
+ IJS_Runtime() = default;
};
#endif // FXJS_IJS_RUNTIME_H_