From 163f59b9a0b019539e9a463ec183c964e7317d5b Mon Sep 17 00:00:00 2001 From: Tom Sepez Date: Fri, 25 Sep 2015 09:29:47 -0700 Subject: Revert "Revert "Merge to master: contention over isolate data slots"" This reverts commit 3b4382a847b5a7439a3107512dbe54c317108579. The difference between this CL and the one that failed is fxjs_v8.cpp:271. In master, we pass the runtime information as: v8::isolate -> v8::Context -> FXJS Runtime, but in XFA: V8::Isolate -> PerIsolate struct -> FXJS Runtime. The master way is more correct, in that FXJS_Runtime is 1:1 with v8 contexts and many:1 (in theory) with isolates. It looks like the XFA branch missed a patch along the way. I'll do that next. Having made this change, the only data in the per-isolate struct will be the ptr array (on master); it will also include the XFA context (on XFA). I've kept the struct on master for the sake of similarity. R=thestig@chromium.org Review URL: https://codereview.chromium.org/1365733003 . --- fpdfsdk/include/jsapi/fxjs_v8.h | 22 ++++- fpdfsdk/src/jsapi/fxjs_v8.cpp | 154 ++++++++++++++--------------- fpdfsdk/src/jsapi/fxjs_v8_embeddertest.cpp | 2 +- 3 files changed, 94 insertions(+), 84 deletions(-) diff --git a/fpdfsdk/include/jsapi/fxjs_v8.h b/fpdfsdk/include/jsapi/fxjs_v8.h index a1541593a1..15800e24e9 100644 --- a/fpdfsdk/include/jsapi/fxjs_v8.h +++ b/fpdfsdk/include/jsapi/fxjs_v8.h @@ -11,7 +11,12 @@ #define FPDFSDK_INCLUDE_JSAPI_FXJS_V8_H_ #include -#include "../../../core/include/fxcrt/fx_string.h" // For CFX_WideString +#include "../../../core/include/fxcrt/fx_basic.h" + +// FXJS_V8 places no interpretation on these two classes; it merely +// passes them on to the caller-provided FXJS_CONSTRUCTORs. +class IFXJS_Context; +class IFXJS_Runtime; enum FXJSOBJTYPE { FXJS_DYNAMIC = 0, @@ -24,6 +29,17 @@ struct FXJSErr { unsigned linnum; }; +class FXJS_PerIsolateData { + public: + static void SetUp(v8::Isolate* pIsolate); + static FXJS_PerIsolateData* Get(v8::Isolate* pIsolate); + + CFX_PtrArray m_ObjectDefnArray; + + protected: + FXJS_PerIsolateData() {} +}; + extern const wchar_t kFXJSValueNameString[]; extern const wchar_t kFXJSValueNameNumber[]; extern const wchar_t kFXJSValueNameBoolean[]; @@ -33,10 +49,6 @@ extern const wchar_t kFXJSValueNameFxobj[]; extern const wchar_t kFXJSValueNameNull[]; extern const wchar_t kFXJSValueNameUndefined[]; -// FXJS_V8 places no interpretation on these two classes; it merely -// passes them on to the caller-provided FXJS_CONSTRUCTORs. -class IFXJS_Context; -class IFXJS_Runtime; class FXJS_ArrayBufferAllocator : public v8::ArrayBuffer::Allocator { void* Allocate(size_t length) override; diff --git a/fpdfsdk/src/jsapi/fxjs_v8.cpp b/fpdfsdk/src/jsapi/fxjs_v8.cpp index 69ea2cb48b..d5673e6e87 100644 --- a/fpdfsdk/src/jsapi/fxjs_v8.cpp +++ b/fpdfsdk/src/jsapi/fxjs_v8.cpp @@ -17,22 +17,32 @@ const wchar_t kFXJSValueNameFxobj[] = L"fxobj"; const wchar_t kFXJSValueNameNull[] = L"null"; const wchar_t kFXJSValueNameUndefined[] = L"undefined"; -static unsigned int g_embedderDataSlot = 0u; +static unsigned int g_embedderDataSlot = 1u; class CFXJS_PrivateData { public: - CFXJS_PrivateData() : ObjDefID(-1), pPrivate(NULL) {} + CFXJS_PrivateData(int nObjDefID) : ObjDefID(nObjDefID), pPrivate(NULL) {} + int ObjDefID; void* pPrivate; }; -class CFXJS_ObjDefintion { +class CFXJS_ObjDefinition { public: - CFXJS_ObjDefintion(v8::Isolate* isolate, - const wchar_t* sObjName, - FXJSOBJTYPE eObjType, - FXJS_CONSTRUCTOR pConstructor, - FXJS_DESTRUCTOR pDestructor) + static int MaxID(v8::Isolate* pIsolate) { + return static_cast( + FXJS_PerIsolateData::Get(pIsolate)->m_ObjectDefnArray.GetSize()); + } + static CFXJS_ObjDefinition* ForID(v8::Isolate* pIsolate, int id) { + // Note: GetAt() halts if out-of-range even in release builds. + return static_cast( + FXJS_PerIsolateData::Get(pIsolate)->m_ObjectDefnArray.GetAt(id)); + } + CFXJS_ObjDefinition(v8::Isolate* isolate, + const wchar_t* sObjName, + FXJSOBJTYPE eObjType, + FXJS_CONSTRUCTOR pConstructor, + FXJS_DESTRUCTOR pDestructor) : objName(sObjName), objType(eObjType), m_pConstructor(pConstructor), @@ -51,12 +61,11 @@ class CFXJS_ObjDefintion { m_bSetAsGlobalObject = TRUE; } } - ~CFXJS_ObjDefintion() { + ~CFXJS_ObjDefinition() { m_objTemplate.Reset(); m_StaticObj.Reset(); } - public: const wchar_t* objName; FXJSOBJTYPE objType; FXJS_CONSTRUCTOR m_pConstructor; @@ -79,9 +88,16 @@ void FXJS_ArrayBufferAllocator::Free(void* data, size_t length) { free(data); } -void FXJS_PrepareIsolate(v8::Isolate* pIsolate) { +// static +void FXJS_PerIsolateData::SetUp(v8::Isolate* pIsolate) { if (!pIsolate->GetData(g_embedderDataSlot)) - pIsolate->SetData(g_embedderDataSlot, new CFX_PtrArray()); + pIsolate->SetData(g_embedderDataSlot, new FXJS_PerIsolateData()); +} + +// static +FXJS_PerIsolateData* FXJS_PerIsolateData::Get(v8::Isolate* pIsolate) { + return static_cast( + pIsolate->GetData(g_embedderDataSlot)); } int FXJS_DefineObj(v8::Isolate* pIsolate, @@ -92,12 +108,11 @@ int FXJS_DefineObj(v8::Isolate* pIsolate, v8::Isolate::Scope isolate_scope(pIsolate); v8::HandleScope handle_scope(pIsolate); - FXJS_PrepareIsolate(pIsolate); - CFX_PtrArray* pArray = (CFX_PtrArray*)pIsolate->GetData(g_embedderDataSlot); - CFXJS_ObjDefintion* pObjDef = new CFXJS_ObjDefintion( - pIsolate, sObjName, eObjType, pConstructor, pDestructor); - pArray->Add(pObjDef); - return pArray->GetSize() - 1; + FXJS_PerIsolateData::SetUp(pIsolate); + FXJS_PerIsolateData* pData = FXJS_PerIsolateData::Get(pIsolate); + pData->m_ObjectDefnArray.Add(new CFXJS_ObjDefinition( + pIsolate, sObjName, eObjType, pConstructor, pDestructor)); + return pData->m_ObjectDefnArray.GetSize() - 1; } void FXJS_DefineObjMethod(v8::Isolate* pIsolate, @@ -107,14 +122,12 @@ void FXJS_DefineObjMethod(v8::Isolate* pIsolate, v8::Isolate::Scope isolate_scope(pIsolate); v8::HandleScope handle_scope(pIsolate); - CFX_WideString ws = CFX_WideString(sMethodName); - CFX_ByteString bsMethodName = ws.UTF8Encode(); - CFX_PtrArray* pArray = (CFX_PtrArray*)pIsolate->GetData(g_embedderDataSlot); - - // Note: GetAt() halts if out-of-range even in release builds. - CFXJS_ObjDefintion* pObjDef = (CFXJS_ObjDefintion*)pArray->GetAt(nObjDefnID); + CFX_ByteString bsMethodName = CFX_WideString(sMethodName).UTF8Encode(); + CFXJS_ObjDefinition* pObjDef = + CFXJS_ObjDefinition::ForID(pIsolate, nObjDefnID); v8::Local objTemp = v8::Local::New(pIsolate, pObjDef->m_objTemplate); + objTemp->Set( v8::String::NewFromUtf8(pIsolate, bsMethodName.c_str(), v8::NewStringType::kNormal).ToLocalChecked(), @@ -130,12 +143,9 @@ void FXJS_DefineObjProperty(v8::Isolate* pIsolate, v8::Isolate::Scope isolate_scope(pIsolate); v8::HandleScope handle_scope(pIsolate); - CFX_WideString ws = CFX_WideString(sPropName); - CFX_ByteString bsPropertyName = ws.UTF8Encode(); - CFX_PtrArray* pArray = (CFX_PtrArray*)pIsolate->GetData(g_embedderDataSlot); - - // Note: GetAt() halts if out-of-range even in release builds. - CFXJS_ObjDefintion* pObjDef = (CFXJS_ObjDefintion*)pArray->GetAt(nObjDefnID); + CFX_ByteString bsPropertyName = CFX_WideString(sPropName).UTF8Encode(); + CFXJS_ObjDefinition* pObjDef = + CFXJS_ObjDefinition::ForID(pIsolate, nObjDefnID); v8::Local objTemp = v8::Local::New(pIsolate, pObjDef->m_objTemplate); objTemp->SetAccessor( @@ -153,10 +163,9 @@ void FXJS_DefineObjAllProperties(v8::Isolate* pIsolate, v8::NamedPropertyDeleterCallback pPropDel) { v8::Isolate::Scope isolate_scope(pIsolate); v8::HandleScope handle_scope(pIsolate); - CFX_PtrArray* pArray = (CFX_PtrArray*)pIsolate->GetData(g_embedderDataSlot); - // Note: GetAt() halts if out-of-range even in release builds. - CFXJS_ObjDefintion* pObjDef = (CFXJS_ObjDefintion*)pArray->GetAt(nObjDefnID); + CFXJS_ObjDefinition* pObjDef = + CFXJS_ObjDefinition::ForID(pIsolate, nObjDefnID); v8::Local objTemp = v8::Local::New(pIsolate, pObjDef->m_objTemplate); objTemp->SetNamedPropertyHandler(pPropGet, pPropPut, pPropQurey, pPropDel); @@ -170,12 +179,9 @@ void FXJS_DefineObjConst(v8::Isolate* pIsolate, v8::Isolate::Scope isolate_scope(pIsolate); v8::HandleScope handle_scope(pIsolate); - CFX_WideString ws = CFX_WideString(sConstName); - CFX_ByteString bsConstName = ws.UTF8Encode(); - CFX_PtrArray* pArray = (CFX_PtrArray*)pIsolate->GetData(g_embedderDataSlot); - - // Note: GetAt() halts if out-of-range even in release builds. - CFXJS_ObjDefintion* pObjDef = (CFXJS_ObjDefintion*)pArray->GetAt(nObjDefnID); + CFX_ByteString bsConstName = CFX_WideString(sConstName).UTF8Encode(); + CFXJS_ObjDefinition* pObjDef = + CFXJS_ObjDefinition::ForID(pIsolate, nObjDefnID); v8::Local objTemp = v8::Local::New(pIsolate, pObjDef->m_objTemplate); objTemp->Set(pIsolate, bsConstName.c_str(), pDefault); @@ -187,10 +193,9 @@ static v8::Global& _getGlobalObjectTemplate( v8::Isolate::Scope isolate_scope(pIsolate); v8::HandleScope handle_scope(pIsolate); - CFX_PtrArray* pArray = (CFX_PtrArray*)pIsolate->GetData(g_embedderDataSlot); - ASSERT(pArray != NULL); - for (int i = 0; i < pArray->GetSize(); i++) { - CFXJS_ObjDefintion* pObjDef = (CFXJS_ObjDefintion*)pArray->GetAt(i); + int maxID = CFXJS_ObjDefinition::MaxID(pIsolate); + for (int i = 0; i < maxID; ++i) { + CFXJS_ObjDefinition* pObjDef = CFXJS_ObjDefinition::ForID(pIsolate, i); if (pObjDef->m_bSetAsGlobalObject) return pObjDef->m_objTemplate; } @@ -204,9 +209,7 @@ void FXJS_DefineGlobalMethod(v8::Isolate* pIsolate, v8::Isolate::Scope isolate_scope(pIsolate); v8::HandleScope handle_scope(pIsolate); - CFX_WideString ws = CFX_WideString(sMethodName); - CFX_ByteString bsMethodName = ws.UTF8Encode(); - + CFX_ByteString bsMethodName = CFX_WideString(sMethodName).UTF8Encode(); v8::Local funTempl = v8::FunctionTemplate::New(pIsolate, pMethodCall); v8::Local objTemp; @@ -264,15 +267,13 @@ void FXJS_InitializeRuntime(v8::Isolate* pIsolate, v8::Local::New(pIsolate, globalObjTemp)); v8::Context::Scope context_scope(v8Context); + FXJS_PerIsolateData::SetUp(pIsolate); v8::Local ptr = v8::External::New(pIsolate, pFXRuntime); v8Context->SetEmbedderData(1, ptr); - CFX_PtrArray* pArray = (CFX_PtrArray*)pIsolate->GetData(g_embedderDataSlot); - if (!pArray) - return; - - for (int i = 0; i < pArray->GetSize(); i++) { - CFXJS_ObjDefintion* pObjDef = (CFXJS_ObjDefintion*)pArray->GetAt(i); + int maxID = CFXJS_ObjDefinition::MaxID(pIsolate); + for (int i = 0; i < maxID; ++i) { + CFXJS_ObjDefinition* pObjDef = CFXJS_ObjDefinition::ForID(pIsolate, i); CFX_WideString ws = CFX_WideString(pObjDef->objName); CFX_ByteString bs = ws.UTF8Encode(); v8::Local objName = @@ -283,14 +284,11 @@ void FXJS_InitializeRuntime(v8::Isolate* pIsolate, if (pObjDef->objType == FXJS_DYNAMIC) { // Document is set as global object, need to construct it first. if (ws.Equal(L"Document")) { - CFXJS_PrivateData* pPrivateData = new CFXJS_PrivateData; - pPrivateData->ObjDefID = i; - v8Context->Global() ->GetPrototype() ->ToObject(v8Context) .ToLocalChecked() - ->SetAlignedPointerInInternalField(0, pPrivateData); + ->SetAlignedPointerInInternalField(0, new CFXJS_PrivateData(i)); if (pObjDef->m_pConstructor) pObjDef->m_pConstructor(context, v8Context->Global() @@ -319,12 +317,13 @@ void FXJS_ReleaseRuntime(v8::Isolate* pIsolate, v8::Local::New(pIsolate, v8PersistentContext); v8::Context::Scope context_scope(context); - CFX_PtrArray* pArray = (CFX_PtrArray*)pIsolate->GetData(g_embedderDataSlot); - if (!pArray) + FXJS_PerIsolateData* pData = FXJS_PerIsolateData::Get(pIsolate); + if (!pData) return; - for (int i = 0; i < pArray->GetSize(); i++) { - CFXJS_ObjDefintion* pObjDef = (CFXJS_ObjDefintion*)pArray->GetAt(i); + int maxID = CFXJS_ObjDefinition::MaxID(pIsolate); + for (int i = 0; i < maxID; ++i) { + CFXJS_ObjDefinition* pObjDef = CFXJS_ObjDefinition::ForID(pIsolate, i); if (!pObjDef->m_StaticObj.IsEmpty()) { v8::Local pObj = v8::Local::New(pIsolate, pObjDef->m_StaticObj); @@ -334,8 +333,9 @@ void FXJS_ReleaseRuntime(v8::Isolate* pIsolate, } delete pObjDef; } - delete pArray; - pIsolate->SetData(g_embedderDataSlot, NULL); + + pIsolate->SetData(g_embedderDataSlot, nullptr); + delete pData; } void FXJS_Initialize(unsigned int embedderDataSlot) { @@ -382,7 +382,7 @@ v8::Local FXJS_NewFxDynamicObj(v8::Isolate* pIsolate, int nObjDefnID) { v8::Isolate::Scope isolate_scope(pIsolate); v8::Local context = pIsolate->GetCurrentContext(); - if (-1 == nObjDefnID) { + if (nObjDefnID == -1) { v8::Local objTempl = v8::ObjectTemplate::New(pIsolate); v8::Local obj; if (objTempl->NewInstance(context).ToLocal(&obj)) @@ -390,13 +390,15 @@ v8::Local FXJS_NewFxDynamicObj(v8::Isolate* pIsolate, return v8::Local(); } - CFX_PtrArray* pArray = (CFX_PtrArray*)pIsolate->GetData(g_embedderDataSlot); - if (!pArray) + FXJS_PerIsolateData* pData = FXJS_PerIsolateData::Get(pIsolate); + if (!pData) return v8::Local(); - if (nObjDefnID < 0 || nObjDefnID >= pArray->GetSize()) + if (nObjDefnID < 0 || nObjDefnID >= CFXJS_ObjDefinition::MaxID(pIsolate)) return v8::Local(); - CFXJS_ObjDefintion* pObjDef = (CFXJS_ObjDefintion*)pArray->GetAt(nObjDefnID); + + CFXJS_ObjDefinition* pObjDef = + CFXJS_ObjDefinition::ForID(pIsolate, nObjDefnID); v8::Local objTemp = v8::Local::New(pIsolate, pObjDef->m_objTemplate); @@ -404,10 +406,7 @@ v8::Local FXJS_NewFxDynamicObj(v8::Isolate* pIsolate, if (!objTemp->NewInstance(context).ToLocal(&obj)) return v8::Local(); - CFXJS_PrivateData* pPrivateData = new CFXJS_PrivateData; - pPrivateData->ObjDefID = nObjDefnID; - - obj->SetAlignedPointerInInternalField(0, pPrivateData); + obj->SetAlignedPointerInInternalField(0, new CFXJS_PrivateData(nObjDefnID)); if (pObjDef->m_pConstructor) pObjDef->m_pConstructor( pJSContext, obj, @@ -417,12 +416,11 @@ v8::Local FXJS_NewFxDynamicObj(v8::Isolate* pIsolate, } v8::Local FXJS_GetThisObj(v8::Isolate* pIsolate) { - // Return the global object. v8::Isolate::Scope isolate_scope(pIsolate); - CFX_PtrArray* pArray = (CFX_PtrArray*)pIsolate->GetData(g_embedderDataSlot); - if (!pArray) + if (!FXJS_PerIsolateData::Get(pIsolate)) return v8::Local(); + // Return the global object. v8::Local context = pIsolate->GetCurrentContext(); return context->Global()->GetPrototype()->ToObject(context).ToLocalChecked(); } @@ -448,12 +446,12 @@ v8::Isolate* FXJS_GetRuntime(v8::Local pObj) { int FXJS_GetObjDefnID(v8::Isolate* pIsolate, const wchar_t* pObjName) { v8::Isolate::Scope isolate_scope(pIsolate); - CFX_PtrArray* pArray = (CFX_PtrArray*)pIsolate->GetData(g_embedderDataSlot); - if (!pArray) + if (!FXJS_PerIsolateData::Get(pIsolate)) return -1; - for (int i = 0; i < pArray->GetSize(); i++) { - CFXJS_ObjDefintion* pObjDef = (CFXJS_ObjDefintion*)pArray->GetAt(i); + int maxID = CFXJS_ObjDefinition::MaxID(pIsolate); + for (int i = 0; i < maxID; ++i) { + CFXJS_ObjDefinition* pObjDef = CFXJS_ObjDefinition::ForID(pIsolate, i); if (FXSYS_wcscmp(pObjDef->objName, pObjName) == 0) return i; } diff --git a/fpdfsdk/src/jsapi/fxjs_v8_embeddertest.cpp b/fpdfsdk/src/jsapi/fxjs_v8_embeddertest.cpp index 2671a91fff..7726c40c5e 100644 --- a/fpdfsdk/src/jsapi/fxjs_v8_embeddertest.cpp +++ b/fpdfsdk/src/jsapi/fxjs_v8_embeddertest.cpp @@ -31,7 +31,7 @@ class FXJSV8Embeddertest : public EmbedderTest { v8::Isolate::Scope isolate_scope(m_pIsolate); v8::HandleScope handle_scope(m_pIsolate); FXJS_Initialize(0); - FXJS_PrepareIsolate(m_pIsolate); + FXJS_PerIsolateData::SetUp(m_pIsolate); FXJS_InitializeRuntime(m_pIsolate, nullptr, nullptr, m_pPersistentContext); } -- cgit v1.2.3