From dc5d88bcebbeeb696b405464e901add55d1efaf7 Mon Sep 17 00:00:00 2001 From: Dan Sinclair Date: Thu, 17 May 2018 13:53:52 +0000 Subject: Convert JS execute methods to return Optional This CL changes several of the JS execution methods to to return an Optional 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 Reviewed-by: Tom Sepez --- fpdfsdk/cpdfsdk_actionhandler.cpp | 5 ++-- fpdfsdk/cpdfsdk_interform.cpp | 11 ++++---- fxjs/cfxjs_engine.cpp | 18 +++++++++----- fxjs/cfxjs_engine.h | 9 ++----- fxjs/cfxjs_engine_embeddertest.cpp | 51 ++++++++++++++++++++++++++++++++------ fxjs/cfxjs_engine_unittest.cpp | 5 ++-- fxjs/cjs_app.cpp | 3 +-- fxjs/cjs_event_context.cpp | 23 +++++++---------- fxjs/cjs_event_context.h | 2 +- fxjs/cjs_event_context_stub.cpp | 6 ++--- fxjs/cjs_event_context_stub.h | 2 +- fxjs/cjs_runtime.cpp | 11 +++----- fxjs/cjs_runtime.h | 3 ++- fxjs/cjs_runtimestub.cpp | 5 ++-- fxjs/cjs_runtimestub.h | 3 ++- fxjs/ijs_event_context.h | 5 +++- fxjs/ijs_runtime.cpp | 5 ++++ fxjs/ijs_runtime.h | 13 ++++++++-- 18 files changed, 112 insertions(+), 68 deletions(-) diff --git a/fpdfsdk/cpdfsdk_actionhandler.cpp b/fpdfsdk/cpdfsdk_actionhandler.cpp index 98c01240b2..660b09c515 100644 --- a/fpdfsdk/cpdfsdk_actionhandler.cpp +++ b/fpdfsdk/cpdfsdk_actionhandler.cpp @@ -544,8 +544,7 @@ void CPDFSDK_ActionHandler::RunScript(CPDFSDK_FormFillEnvironment* pFormFillEnv, 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 4f11babbbe..b37562ccf2 100644 --- a/fpdfsdk/cpdfsdk_interform.cpp +++ b/fpdfsdk/cpdfsdk_interform.cpp @@ -291,10 +291,9 @@ void CPDFSDK_InterForm::OnCalculate(CPDF_FormField* pFormField) { bool bRC = true; pContext->OnField_Calculate(pFormField, pField, sValue, bRC); - WideString sInfo; - bool bRet = pContext->RunScript(csJS, &sInfo); + Optional 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 @@ WideString CPDFSDK_InterForm::OnFormat(CPDF_FormField* pFormField, IJS_EventContext* pContext = pRuntime->NewEventContext(); pContext->OnField_Format(pFormField, Value, true); - WideString sInfo; - bool bRet = pContext->RunScript(script, &sInfo); + + Optional 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 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 CFXJS_Engine::Execute( + const WideString& script) { v8::Isolate::Scope isolate_scope(GetIsolate()); v8::TryCatch try_catch(GetIsolate()); v8::Local 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 msg = try_catch.Message(); + v8::Maybe line = msg->GetLineNumber(context); + + return IJS_Runtime::JS_Error(line.FromMaybe(-1), msg->GetStartColumn(), + WideString::FromUTF8(*error)); } v8::Local 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 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 Execute(const WideString& script); v8::Local GetThisObj(); v8::Local 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 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 err = + ExecuteInCurrentContext(WideString(kScript1)); + EXPECT_FALSE(err); CheckAssignmentInCurrentContext(kExpected1); } @@ -62,17 +64,23 @@ TEST_F(CFXJSEngineEmbedderTest, MultipleEngines) { v8::Local context2 = engine2.GetV8Context(); v8::Context::Scope context_scope(GetV8Context()); - ExecuteInCurrentContext(WideString(kScript0)); + Optional err = + ExecuteInCurrentContext(WideString(kScript0)); + EXPECT_FALSE(err); CheckAssignmentInCurrentContext(kExpected0); { v8::Context::Scope context_scope1(context1); - ExecuteInCurrentContext(WideString(kScript1)); + Optional err = + ExecuteInCurrentContext(WideString(kScript1)); + EXPECT_FALSE(err); CheckAssignmentInCurrentContext(kExpected1); } { v8::Context::Scope context_scope2(context2); - ExecuteInCurrentContext(WideString(kScript2)); + Optional 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 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 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 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 CJS_EventContext::RunScript( + const WideString& script) { v8::Isolate::Scope isolate_scope(m_pRuntime->GetIsolate()); v8::HandleScope handle_scope(m_pRuntime->GetIsolate()); v8::Local 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 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 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 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 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 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 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 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 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 ExecuteScript( + const WideString& script) override; protected: UnownedPtr 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 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::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 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 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_ -- cgit v1.2.3