From a72e8e2c4392df725c7e35ed1ae3f891e79e49ec Mon Sep 17 00:00:00 2001 From: Tom Sepez Date: Wed, 7 Oct 2015 10:17:53 -0700 Subject: Merge to XFA: Pass v8::Isolate to PDFium at init time. New code changes in JS_Runtime.cpp. (cherry picked from commit 3dedace9623fef6161a8666e53a4ab2b9be61e4c) Original Review URL: https://codereview.chromium.org/1367033002 . Also merge to XFA: Run FXJS_V8 embedder tests against a shared isolate, since the two go together. (cherry picked from commit 4ea721cb7954898a9722c389dae86c62957352d0) Original Review URL: https://codereview.chromium.org/1377293004 . R=thestig@chromium.org Review URL: https://codereview.chromium.org/1381633008 . --- fpdfsdk/include/jsapi/fxjs_v8.h | 7 +++++- fpdfsdk/src/fpdfview.cpp | 6 ++++- fpdfsdk/src/javascript/JS_Runtime.cpp | 27 +++++++++----------- fpdfsdk/src/javascript/JS_Runtime.h | 2 -- fpdfsdk/src/jsapi/fxjs_v8.cpp | 40 ++++++++++++++++++++++-------- fpdfsdk/src/jsapi/fxjs_v8_embeddertest.cpp | 24 ++++++++++-------- public/fpdf_formfill.h | 15 ++++------- public/fpdfview.h | 13 +++++++++- samples/pdfium_test.cc | 18 ++++++++------ testing/embedder_test.cpp | 15 +++++++---- testing/embedder_test.h | 9 +++++-- 11 files changed, 110 insertions(+), 66 deletions(-) diff --git a/fpdfsdk/include/jsapi/fxjs_v8.h b/fpdfsdk/include/jsapi/fxjs_v8.h index f51340acd3..6e8d2fa478 100644 --- a/fpdfsdk/include/jsapi/fxjs_v8.h +++ b/fpdfsdk/include/jsapi/fxjs_v8.h @@ -68,9 +68,14 @@ using FXJS_CONSTRUCTOR = void (*)(IFXJS_Context* cc, using FXJS_DESTRUCTOR = void (*)(v8::Local obj); // Call before making FXJS_PrepareIsolate call. -void FXJS_Initialize(unsigned int embedderDataSlot); +void FXJS_Initialize(unsigned int embedderDataSlot, v8::Isolate* pIsolate); void FXJS_Release(); +// Gets the global isolate set by FXJS_Initialize(), or makes a new one each +// time if there is no such isolate. Returns true if a new isolate had to be +// created. +bool FXJS_GetIsolate(v8::Isolate** pResultIsolate); + // Call before making FXJS_Define* calls. Resources allocated here are cleared // as part of FXJS_ReleaseRuntime(). void FXJS_PrepareIsolate(v8::Isolate* pIsolate); diff --git a/fpdfsdk/src/fpdfview.cpp b/fpdfsdk/src/fpdfview.cpp index 47da1f49de..cf05d79f5c 100644 --- a/fpdfsdk/src/fpdfview.cpp +++ b/fpdfsdk/src/fpdfview.cpp @@ -20,6 +20,7 @@ #include "../include/fpdfxfa/fpdfxfa_app.h" #include "../include/fpdfxfa/fpdfxfa_page.h" #include "../include/fpdfxfa/fpdfxfa_util.h" +#include "../include/jsapi/fxjs_v8.h" CFPDF_FileStream::CFPDF_FileStream(FPDF_FILEHANDLER* pFS) { m_pFS = pFS; @@ -194,8 +195,11 @@ FPDF_InitLibraryWithConfig(const FPDF_LIBRARY_CONFIG* cfg) { CPDF_ModuleMgr::Get()->SetCodecModule(g_pCodecModule); CPDF_ModuleMgr::Get()->InitPageModule(); CPDF_ModuleMgr::Get()->InitRenderModule(); - CPDFXFA_App::GetInstance()->Initialize(); + if (cfg && cfg->version >= 2) { + FXJS_Initialize(cfg->m_v8EmbedderSlot, + reinterpret_cast(cfg->m_pIsolate)); + } } DLLEXPORT void STDCALL FPDF_DestroyLibrary() { diff --git a/fpdfsdk/src/javascript/JS_Runtime.cpp b/fpdfsdk/src/javascript/JS_Runtime.cpp index f206154029..bc5443daf6 100644 --- a/fpdfsdk/src/javascript/JS_Runtime.cpp +++ b/fpdfsdk/src/javascript/JS_Runtime.cpp @@ -45,17 +45,18 @@ CJS_Runtime::CJS_Runtime(CPDFDoc_Environment* pApp) if (CPDFXFA_App::GetInstance()->GetJSERuntime()) { // TODO(tsepez): CPDFXFA_App should also use the embedder provided isolate. m_isolate = (v8::Isolate*)CPDFXFA_App::GetInstance()->GetJSERuntime(); - } else if (m_pApp->GetFormFillInfo()->m_pJsPlatform->version >= 2) { - m_isolate = reinterpret_cast( - m_pApp->GetFormFillInfo()->m_pJsPlatform->m_isolate); - } - if (!m_isolate) { - m_pArrayBufferAllocator.reset(new FXJS_ArrayBufferAllocator()); - - v8::Isolate::CreateParams params; - params.array_buffer_allocator = m_pArrayBufferAllocator.get(); - m_isolate = v8::Isolate::New(params); - m_isolateManaged = true; + } else { + IPDF_JSPLATFORM* pPlatform = m_pApp->GetFormFillInfo()->m_pJsPlatform; + if (pPlatform->version <= 2) { + unsigned int embedderDataSlot = 0; + v8::Isolate* pExternalIsolate = nullptr; + if (pPlatform->version == 2) { + pExternalIsolate = reinterpret_cast(pPlatform->m_isolate); + embedderDataSlot = pPlatform->m_v8EmbedderSlot; + } + FXJS_Initialize(embedderDataSlot, pExternalIsolate); + } + m_isolateManaged = FXJS_GetIsolate(&m_isolate); } v8::Isolate* isolate = m_isolate; @@ -69,10 +70,6 @@ CJS_Runtime::CJS_Runtime(CPDFDoc_Environment* pApp) return; } - unsigned int embedderDataSlot = 0; - if (m_pApp->GetFormFillInfo()->m_pJsPlatform->version >= 2) - embedderDataSlot = pApp->GetFormFillInfo()->m_pJsPlatform->m_v8EmbedderSlot; - FXJS_Initialize(embedderDataSlot); DefineJSObjects(); CPDFXFA_App::GetInstance()->SetJavaScriptInitialized(TRUE); diff --git a/fpdfsdk/src/javascript/JS_Runtime.h b/fpdfsdk/src/javascript/JS_Runtime.h index 0576980233..1034ae8c8b 100644 --- a/fpdfsdk/src/javascript/JS_Runtime.h +++ b/fpdfsdk/src/javascript/JS_Runtime.h @@ -10,7 +10,6 @@ #include #include -#include "../../../third_party/base/nonstd_unique_ptr.h" #include "../../../core/include/fxcrt/fx_basic.h" #include "../../include/javascript/IJavaScript.h" #include "../../include/jsapi/fxjs_v8.h" @@ -71,7 +70,6 @@ class CJS_Runtime : public IFXJS_Runtime { std::set m_FieldEventSet; v8::Isolate* m_isolate; bool m_isolateManaged; - nonstd::unique_ptr m_pArrayBufferAllocator; v8::Global m_context; std::set m_observers; }; diff --git a/fpdfsdk/src/jsapi/fxjs_v8.cpp b/fpdfsdk/src/jsapi/fxjs_v8.cpp index 765ec2700d..0be7cc01e2 100644 --- a/fpdfsdk/src/jsapi/fxjs_v8.cpp +++ b/fpdfsdk/src/jsapi/fxjs_v8.cpp @@ -16,14 +16,14 @@ const wchar_t kFXJSValueNameFxobj[] = L"fxobj"; const wchar_t kFXJSValueNameNull[] = L"null"; const wchar_t kFXJSValueNameUndefined[] = L"undefined"; -static unsigned int g_embedderDataSlot = 1u; - // Keep this consistent with the values defined in gin/public/context_holder.h // (without actually requiring a dependency on gin itself for the standalone // embedders of PDFIum). The value we want to use is: // kPerContextDataStartIndex + kEmbedderPDFium, which is 3. static const unsigned int kPerContextDataIndex = 3u; - +static unsigned int g_embedderDataSlot = 1u; +static v8::Isolate* g_isolate = nullptr; +static FXJS_ArrayBufferAllocator* g_arrayBufferAllocator = nullptr; static v8::Global* g_DefaultGlobalObjectTemplate = nullptr; class CFXJS_PrivateData { @@ -123,6 +123,33 @@ void FXJS_ArrayBufferAllocator::Free(void* data, size_t length) { free(data); } +void FXJS_Initialize(unsigned int embedderDataSlot, v8::Isolate* pIsolate) { + g_embedderDataSlot = embedderDataSlot; + g_isolate = pIsolate; +} + +void FXJS_Release() { + g_DefaultGlobalObjectTemplate = nullptr; + g_isolate = nullptr; + + delete g_arrayBufferAllocator; + g_arrayBufferAllocator = nullptr; +} + +bool FXJS_GetIsolate(v8::Isolate** pResultIsolate) { + if (g_isolate) { + *pResultIsolate = g_isolate; + return false; + } + // Provide backwards compatibility when no external isolate. + if (!g_arrayBufferAllocator) + g_arrayBufferAllocator = new FXJS_ArrayBufferAllocator(); + v8::Isolate::CreateParams params; + params.array_buffer_allocator = g_arrayBufferAllocator; + *pResultIsolate = v8::Isolate::New(params); + return true; +} + // static void FXJS_PerIsolateData::SetUp(v8::Isolate* pIsolate) { if (!pIsolate->GetData(g_embedderDataSlot)) @@ -135,13 +162,6 @@ FXJS_PerIsolateData* FXJS_PerIsolateData::Get(v8::Isolate* pIsolate) { pIsolate->GetData(g_embedderDataSlot)); } -void FXJS_Initialize(unsigned int embedderDataSlot) { - g_embedderDataSlot = embedderDataSlot; -} - -void FXJS_Release() { -} - int FXJS_DefineObj(v8::Isolate* pIsolate, const wchar_t* sObjName, FXJSOBJTYPE eObjType, diff --git a/fpdfsdk/src/jsapi/fxjs_v8_embeddertest.cpp b/fpdfsdk/src/jsapi/fxjs_v8_embeddertest.cpp index 55eb9c8408..fc2324781a 100644 --- a/fpdfsdk/src/jsapi/fxjs_v8_embeddertest.cpp +++ b/fpdfsdk/src/jsapi/fxjs_v8_embeddertest.cpp @@ -16,40 +16,42 @@ const wchar_t kScript[] = L"fred = 7"; class FXJSV8Embeddertest : public EmbedderTest { public: - FXJSV8Embeddertest() : m_pIsolate(nullptr) {} - ~FXJSV8Embeddertest() override {} + FXJSV8Embeddertest() + : m_pArrayBufferAllocator(new FXJS_ArrayBufferAllocator) { + v8::Isolate::CreateParams params; + params.array_buffer_allocator = m_pArrayBufferAllocator.get(); + m_pIsolate = v8::Isolate::New(params); + } + + ~FXJSV8Embeddertest() override { m_pIsolate->Dispose(); } void SetUp() override { + EmbedderTest::SetExternalIsolate(m_pIsolate); EmbedderTest::SetUp(); - m_pAllocator.reset(new FXJS_ArrayBufferAllocator()); - - v8::Isolate::CreateParams params; - params.array_buffer_allocator = m_pAllocator.get(); - m_pIsolate = v8::Isolate::New(params); v8::Isolate::Scope isolate_scope(m_pIsolate); v8::Locker locker(m_pIsolate); v8::HandleScope handle_scope(m_pIsolate); - FXJS_Initialize(0); FXJS_PerIsolateData::SetUp(m_pIsolate); FXJS_InitializeRuntime(m_pIsolate, nullptr, nullptr, m_pPersistentContext); } void TearDown() override { FXJS_ReleaseRuntime(m_pIsolate, m_pPersistentContext); + m_pPersistentContext.Reset(); FXJS_Release(); EmbedderTest::TearDown(); } - v8::Isolate* isolate() const { return m_pIsolate; } + v8::Isolate* isolate() { return m_pIsolate; } v8::Local GetV8Context() { - return v8::Local::New(m_pIsolate, m_pPersistentContext); + return m_pPersistentContext.Get(m_pIsolate); } private: + nonstd::unique_ptr m_pArrayBufferAllocator; v8::Isolate* m_pIsolate; v8::Global m_pPersistentContext; - nonstd::unique_ptr m_pAllocator; }; TEST_F(FXJSV8Embeddertest, Getters) { diff --git a/public/fpdf_formfill.h b/public/fpdf_formfill.h index 7be678cd47..d04911723d 100644 --- a/public/fpdf_formfill.h +++ b/public/fpdf_formfill.h @@ -299,17 +299,12 @@ typedef struct _IPDF_JsPlatform { /* Version 2. */ - /** - * pointer to the v8::Isolate to use, or NULL to force PDFium to create one. - **/ - void* m_isolate; + void* m_isolate; /* Unused in v3, retain for compatibility. */ + unsigned int m_v8EmbedderSlot; /* Unused in v3, retain for compatibility. */ + + /* Version 3. */ + /* Version 3 moves m_Isolate and m_v8EmbedderSlot to FPDF_LIBRARY_CONFIG. */ - /** - * The embedder data slot to use in the v8::Isolate to store PDFium's - * per-isolate data. The value needs to be between 0 and - * v8::Internals::kNumIsolateDataLots (exclusive). - */ - unsigned int m_v8EmbedderSlot; } IPDF_JSPLATFORM; // Flags for Cursor type diff --git a/public/fpdfview.h b/public/fpdfview.h index bfa13c43c1..f899709873 100644 --- a/public/fpdfview.h +++ b/public/fpdfview.h @@ -158,7 +158,7 @@ DLLEXPORT void STDCALL FPDF_InitLibrary(); // Process-wide options for initializing library. typedef struct FPDF_LIBRARY_CONFIG_ { - // Version number of the interface. Currently must be 1. + // Version number of the interface. Currently must be 2. int version; // Array of paths to scan in place of the defaults when using built-in @@ -166,6 +166,17 @@ typedef struct FPDF_LIBRARY_CONFIG_ { // The Array may be NULL itself to use the default paths. May be ignored // entirely depending upon the platform. const char** m_pUserFontPaths; + + // Version 2. + + // pointer to the v8::Isolate to use, or NULL to force PDFium to create one. + void* m_pIsolate; + + // The embedder data slot to use in the v8::Isolate to store PDFium's + // per-isolate data. The value needs to be between 0 and + // v8::Internals::kNumIsolateDataLots (exclusive). Note that 0 is fine + // for most embedders. + unsigned int m_v8EmbedderSlot; } FPDF_LIBRARY_CONFIG; // Function: FPDF_InitLibraryWithConfig diff --git a/samples/pdfium_test.cc b/samples/pdfium_test.cc index 056ba0b6db..a17a85390d 100644 --- a/samples/pdfium_test.cc +++ b/samples/pdfium_test.cc @@ -437,7 +437,7 @@ void RenderPdf(const std::string& name, const char* pBuf, size_t len, IPDF_JSPLATFORM platform_callbacks; memset(&platform_callbacks, '\0', sizeof(platform_callbacks)); - platform_callbacks.version = 2; + platform_callbacks.version = 3; platform_callbacks.app_alert = ExampleAppAlert; platform_callbacks.Doc_gotoPage = ExampleDocGotoPage; @@ -621,17 +621,19 @@ int main(int argc, const char* argv[]) { v8::V8::SetSnapshotDataBlob(&snapshot); #endif // V8_USE_EXTERNAL_STARTUP_DATA - if (options.font_directory.empty()) { - FPDF_InitLibrary(); - } else { - const char* path_array[2]; + FPDF_LIBRARY_CONFIG config; + config.version = 2; + config.m_pUserFontPaths = nullptr; + config.m_pIsolate = nullptr; + config.m_v8EmbedderSlot = 0; + + const char* path_array[2]; + if (!options.font_directory.empty()) { path_array[0] = options.font_directory.c_str(); path_array[1] = nullptr; - FPDF_LIBRARY_CONFIG config; - config.version = 1; config.m_pUserFontPaths = path_array; - FPDF_InitLibraryWithConfig(&config); } + FPDF_InitLibraryWithConfig(&config); UNSUPPORT_INFO unsuppored_info; memset(&unsuppored_info, '\0', sizeof(unsuppored_info)); diff --git a/testing/embedder_test.cpp b/testing/embedder_test.cpp index 5dc5a66eb6..1a3c4c6e53 100644 --- a/testing/embedder_test.cpp +++ b/testing/embedder_test.cpp @@ -123,21 +123,21 @@ FPDF_BOOL Is_Data_Avail(FX_FILEAVAIL* pThis, size_t offset, size_t size) { void Add_Segment(FX_DOWNLOADHINTS* pThis, size_t offset, size_t size) {} EmbedderTest::EmbedderTest() - : document_(nullptr), + : default_delegate_(new EmbedderTest::Delegate()), + document_(nullptr), form_handle_(nullptr), avail_(nullptr), + external_isolate_(nullptr), loader_(nullptr), file_length_(0), file_contents_(nullptr) { memset(&hints_, 0, sizeof(hints_)); memset(&file_access_, 0, sizeof(file_access_)); memset(&file_avail_, 0, sizeof(file_avail_)); - default_delegate_ = new EmbedderTest::Delegate(); - delegate_ = default_delegate_; + delegate_ = default_delegate_.get(); } EmbedderTest::~EmbedderTest() { - delete default_delegate_; } void EmbedderTest::SetUp() { @@ -159,7 +159,12 @@ void EmbedderTest::SetUp() { v8::V8::SetSnapshotDataBlob(&snapshot_); #endif // V8_USE_EXTERNAL_STARTUP_DATA - FPDF_InitLibrary(); + FPDF_LIBRARY_CONFIG config; + config.version = 2; + config.m_pUserFontPaths = nullptr; + config.m_pIsolate = external_isolate_; + config.m_v8EmbedderSlot = 0; + FPDF_InitLibraryWithConfig(&config); UNSUPPORT_INFO* info = static_cast(this); memset(info, 0, sizeof(UNSUPPORT_INFO)); diff --git a/testing/embedder_test.h b/testing/embedder_test.h index f490f82536..f6842d23c3 100644 --- a/testing/embedder_test.h +++ b/testing/embedder_test.h @@ -12,6 +12,7 @@ #include "../public/fpdf_ext.h" #include "../public/fpdf_formfill.h" #include "../public/fpdfview.h" +#include "../third_party/base/nonstd_unique_ptr.h" #include "testing/gtest/include/gtest/gtest.h" #include "v8/include/v8.h" @@ -60,8 +61,11 @@ class EmbedderTest : public ::testing::Test, void SetUp() override; void TearDown() override; + // Call before SetUp to pass shared isolate, otherwise PDFium creates one. + void SetExternalIsolate(v8::Isolate* isolate) { external_isolate_ = isolate; } + void SetDelegate(Delegate* delegate) { - delegate_ = delegate ? delegate : default_delegate_; + delegate_ = delegate ? delegate : default_delegate_.get(); } FPDF_DOCUMENT document() { return document_; } @@ -94,7 +98,7 @@ class EmbedderTest : public ::testing::Test, protected: Delegate* delegate_; - Delegate* default_delegate_; + nonstd::unique_ptr default_delegate_; FPDF_DOCUMENT document_; FPDF_FORMHANDLE form_handle_; FPDF_AVAIL avail_; @@ -104,6 +108,7 @@ class EmbedderTest : public ::testing::Test, v8::Platform* platform_; v8::StartupData natives_; v8::StartupData snapshot_; + v8::Isolate* external_isolate_; TestLoader* loader_; size_t file_length_; char* file_contents_; -- cgit v1.2.3