summaryrefslogtreecommitdiff
path: root/fxjs
diff options
context:
space:
mode:
authorDan Sinclair <dsinclair@chromium.org>2018-05-17 13:53:52 +0000
committerChromium commit bot <commit-bot@chromium.org>2018-05-17 13:53:52 +0000
commitdc5d88bcebbeeb696b405464e901add55d1efaf7 (patch)
tree54bc913015ea4d08b9fe46997f25a3153cbc70d7 /fxjs
parentdb3c6cefceddf25c25f1205d7b633f09e873bf98 (diff)
downloadpdfium-dc5d88bcebbeeb696b405464e901add55d1efaf7.tar.xz
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>
Diffstat (limited to 'fxjs')
-rw-r--r--fxjs/cfxjs_engine.cpp18
-rw-r--r--fxjs/cfxjs_engine.h9
-rw-r--r--fxjs/cfxjs_engine_embeddertest.cpp51
-rw-r--r--fxjs/cfxjs_engine_unittest.cpp5
-rw-r--r--fxjs/cjs_app.cpp3
-rw-r--r--fxjs/cjs_event_context.cpp23
-rw-r--r--fxjs/cjs_event_context.h2
-rw-r--r--fxjs/cjs_event_context_stub.cpp6
-rw-r--r--fxjs/cjs_event_context_stub.h2
-rw-r--r--fxjs/cjs_runtime.cpp11
-rw-r--r--fxjs/cjs_runtime.h3
-rw-r--r--fxjs/cjs_runtimestub.cpp5
-rw-r--r--fxjs/cjs_runtimestub.h3
-rw-r--r--fxjs/ijs_event_context.h5
-rw-r--r--fxjs/ijs_runtime.cpp5
-rw-r--r--fxjs/ijs_runtime.h13
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_