From 3b4382a847b5a7439a3107512dbe54c317108579 Mon Sep 17 00:00:00 2001 From: Tom Sepez Date: Tue, 22 Sep 2015 15:54:26 -0700 Subject: Revert "Merge to master: contention over isolate data slots" Reason for revert: embeddertests failed. This reverts commit 70bc04b16646c92f221c5aa56831b01d6ec7c1ca. TBR=thestig@chromium.org Review URL: https://codereview.chromium.org/1358263004 . --- fpdfsdk/include/jsapi/fxjs_v8.h | 23 +---- fpdfsdk/src/jsapi/fxjs_v8.cpp | 158 +++++++++++++++-------------- fpdfsdk/src/jsapi/fxjs_v8_embeddertest.cpp | 2 +- 3 files changed, 86 insertions(+), 97 deletions(-) diff --git a/fpdfsdk/include/jsapi/fxjs_v8.h b/fpdfsdk/include/jsapi/fxjs_v8.h index eb810c0695..a1541593a1 100644 --- a/fpdfsdk/include/jsapi/fxjs_v8.h +++ b/fpdfsdk/include/jsapi/fxjs_v8.h @@ -11,12 +11,7 @@ #define FPDFSDK_INCLUDE_JSAPI_FXJS_V8_H_ #include -#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; +#include "../../../core/include/fxcrt/fx_string.h" // For CFX_WideString enum FXJSOBJTYPE { FXJS_DYNAMIC = 0, @@ -29,18 +24,6 @@ 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; - IFXJS_Runtime* m_pFXJSRuntime; - - protected: - FXJS_PerIsolateData() : m_pFXJSRuntime(nullptr) {} -}; - extern const wchar_t kFXJSValueNameString[]; extern const wchar_t kFXJSValueNameNumber[]; extern const wchar_t kFXJSValueNameBoolean[]; @@ -50,6 +33,10 @@ 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 c5697c7084..69ea2cb48b 100644 --- a/fpdfsdk/src/jsapi/fxjs_v8.cpp +++ b/fpdfsdk/src/jsapi/fxjs_v8.cpp @@ -17,32 +17,22 @@ const wchar_t kFXJSValueNameFxobj[] = L"fxobj"; const wchar_t kFXJSValueNameNull[] = L"null"; const wchar_t kFXJSValueNameUndefined[] = L"undefined"; -static unsigned int g_embedderDataSlot = 1u; +static unsigned int g_embedderDataSlot = 0u; class CFXJS_PrivateData { public: - CFXJS_PrivateData(int nObjDefID) : ObjDefID(nObjDefID), pPrivate(NULL) {} - + CFXJS_PrivateData() : ObjDefID(-1), pPrivate(NULL) {} int ObjDefID; void* pPrivate; }; -class CFXJS_ObjDefinition { +class CFXJS_ObjDefintion { public: - 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) + CFXJS_ObjDefintion(v8::Isolate* isolate, + const wchar_t* sObjName, + FXJSOBJTYPE eObjType, + FXJS_CONSTRUCTOR pConstructor, + FXJS_DESTRUCTOR pDestructor) : objName(sObjName), objType(eObjType), m_pConstructor(pConstructor), @@ -61,11 +51,12 @@ class CFXJS_ObjDefinition { m_bSetAsGlobalObject = TRUE; } } - ~CFXJS_ObjDefinition() { + ~CFXJS_ObjDefintion() { m_objTemplate.Reset(); m_StaticObj.Reset(); } + public: const wchar_t* objName; FXJSOBJTYPE objType; FXJS_CONSTRUCTOR m_pConstructor; @@ -88,16 +79,9 @@ void FXJS_ArrayBufferAllocator::Free(void* data, size_t length) { free(data); } -// static -void FXJS_PerIsolateData::SetUp(v8::Isolate* pIsolate) { +void FXJS_PrepareIsolate(v8::Isolate* pIsolate) { if (!pIsolate->GetData(g_embedderDataSlot)) - pIsolate->SetData(g_embedderDataSlot, new FXJS_PerIsolateData()); -} - -// static -FXJS_PerIsolateData* FXJS_PerIsolateData::Get(v8::Isolate* pIsolate) { - return static_cast( - pIsolate->GetData(g_embedderDataSlot)); + pIsolate->SetData(g_embedderDataSlot, new CFX_PtrArray()); } int FXJS_DefineObj(v8::Isolate* pIsolate, @@ -108,11 +92,12 @@ int FXJS_DefineObj(v8::Isolate* pIsolate, v8::Isolate::Scope isolate_scope(pIsolate); v8::HandleScope handle_scope(pIsolate); - 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; + 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; } void FXJS_DefineObjMethod(v8::Isolate* pIsolate, @@ -122,12 +107,14 @@ void FXJS_DefineObjMethod(v8::Isolate* pIsolate, v8::Isolate::Scope isolate_scope(pIsolate); v8::HandleScope handle_scope(pIsolate); - CFX_ByteString bsMethodName = CFX_WideString(sMethodName).UTF8Encode(); - CFXJS_ObjDefinition* pObjDef = - CFXJS_ObjDefinition::ForID(pIsolate, nObjDefnID); + 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); v8::Local objTemp = v8::Local::New(pIsolate, pObjDef->m_objTemplate); - objTemp->Set( v8::String::NewFromUtf8(pIsolate, bsMethodName.c_str(), v8::NewStringType::kNormal).ToLocalChecked(), @@ -143,9 +130,12 @@ void FXJS_DefineObjProperty(v8::Isolate* pIsolate, v8::Isolate::Scope isolate_scope(pIsolate); v8::HandleScope handle_scope(pIsolate); - CFX_ByteString bsPropertyName = CFX_WideString(sPropName).UTF8Encode(); - CFXJS_ObjDefinition* pObjDef = - CFXJS_ObjDefinition::ForID(pIsolate, nObjDefnID); + 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); v8::Local objTemp = v8::Local::New(pIsolate, pObjDef->m_objTemplate); objTemp->SetAccessor( @@ -163,9 +153,10 @@ 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); - CFXJS_ObjDefinition* pObjDef = - CFXJS_ObjDefinition::ForID(pIsolate, nObjDefnID); + // Note: GetAt() halts if out-of-range even in release builds. + CFXJS_ObjDefintion* pObjDef = (CFXJS_ObjDefintion*)pArray->GetAt(nObjDefnID); v8::Local objTemp = v8::Local::New(pIsolate, pObjDef->m_objTemplate); objTemp->SetNamedPropertyHandler(pPropGet, pPropPut, pPropQurey, pPropDel); @@ -179,9 +170,12 @@ void FXJS_DefineObjConst(v8::Isolate* pIsolate, v8::Isolate::Scope isolate_scope(pIsolate); v8::HandleScope handle_scope(pIsolate); - CFX_ByteString bsConstName = CFX_WideString(sConstName).UTF8Encode(); - CFXJS_ObjDefinition* pObjDef = - CFXJS_ObjDefinition::ForID(pIsolate, nObjDefnID); + 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); v8::Local objTemp = v8::Local::New(pIsolate, pObjDef->m_objTemplate); objTemp->Set(pIsolate, bsConstName.c_str(), pDefault); @@ -193,9 +187,10 @@ static v8::Global& _getGlobalObjectTemplate( v8::Isolate::Scope isolate_scope(pIsolate); v8::HandleScope handle_scope(pIsolate); - int maxID = CFXJS_ObjDefinition::MaxID(pIsolate); - for (int i = 0; i < maxID; ++i) { - CFXJS_ObjDefinition* pObjDef = CFXJS_ObjDefinition::ForID(pIsolate, i); + 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); if (pObjDef->m_bSetAsGlobalObject) return pObjDef->m_objTemplate; } @@ -209,7 +204,9 @@ void FXJS_DefineGlobalMethod(v8::Isolate* pIsolate, v8::Isolate::Scope isolate_scope(pIsolate); v8::HandleScope handle_scope(pIsolate); - CFX_ByteString bsMethodName = CFX_WideString(sMethodName).UTF8Encode(); + CFX_WideString ws = CFX_WideString(sMethodName); + CFX_ByteString bsMethodName = ws.UTF8Encode(); + v8::Local funTempl = v8::FunctionTemplate::New(pIsolate, pMethodCall); v8::Local objTemp; @@ -267,13 +264,15 @@ void FXJS_InitializeRuntime(v8::Isolate* pIsolate, v8::Local::New(pIsolate, globalObjTemp)); v8::Context::Scope context_scope(v8Context); - FXJS_PerIsolateData::SetUp(pIsolate); - FXJS_PerIsolateData* pData = FXJS_PerIsolateData::Get(pIsolate); - pData->m_pFXJSRuntime = pFXRuntime; + v8::Local ptr = v8::External::New(pIsolate, pFXRuntime); + v8Context->SetEmbedderData(1, ptr); - int maxID = CFXJS_ObjDefinition::MaxID(pIsolate); - for (int i = 0; i < maxID; ++i) { - CFXJS_ObjDefinition* pObjDef = CFXJS_ObjDefinition::ForID(pIsolate, i); + 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); CFX_WideString ws = CFX_WideString(pObjDef->objName); CFX_ByteString bs = ws.UTF8Encode(); v8::Local objName = @@ -284,11 +283,14 @@ 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, new CFXJS_PrivateData(i)); + ->SetAlignedPointerInInternalField(0, pPrivateData); if (pObjDef->m_pConstructor) pObjDef->m_pConstructor(context, v8Context->Global() @@ -317,13 +319,12 @@ void FXJS_ReleaseRuntime(v8::Isolate* pIsolate, v8::Local::New(pIsolate, v8PersistentContext); v8::Context::Scope context_scope(context); - FXJS_PerIsolateData* pData = FXJS_PerIsolateData::Get(pIsolate); - if (!pData) + CFX_PtrArray* pArray = (CFX_PtrArray*)pIsolate->GetData(g_embedderDataSlot); + if (!pArray) return; - int maxID = CFXJS_ObjDefinition::MaxID(pIsolate); - for (int i = 0; i < maxID; ++i) { - CFXJS_ObjDefinition* pObjDef = CFXJS_ObjDefinition::ForID(pIsolate, i); + for (int i = 0; i < pArray->GetSize(); i++) { + CFXJS_ObjDefintion* pObjDef = (CFXJS_ObjDefintion*)pArray->GetAt(i); if (!pObjDef->m_StaticObj.IsEmpty()) { v8::Local pObj = v8::Local::New(pIsolate, pObjDef->m_StaticObj); @@ -333,9 +334,8 @@ void FXJS_ReleaseRuntime(v8::Isolate* pIsolate, } delete pObjDef; } - - pIsolate->SetData(g_embedderDataSlot, nullptr); - delete pData; + delete pArray; + pIsolate->SetData(g_embedderDataSlot, NULL); } 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 (nObjDefnID == -1) { + if (-1 == nObjDefnID) { v8::Local objTempl = v8::ObjectTemplate::New(pIsolate); v8::Local obj; if (objTempl->NewInstance(context).ToLocal(&obj)) @@ -390,15 +390,13 @@ v8::Local FXJS_NewFxDynamicObj(v8::Isolate* pIsolate, return v8::Local(); } - FXJS_PerIsolateData* pData = FXJS_PerIsolateData::Get(pIsolate); - if (!pData) + CFX_PtrArray* pArray = (CFX_PtrArray*)pIsolate->GetData(g_embedderDataSlot); + if (!pArray) return v8::Local(); - if (nObjDefnID < 0 || nObjDefnID >= CFXJS_ObjDefinition::MaxID(pIsolate)) + if (nObjDefnID < 0 || nObjDefnID >= pArray->GetSize()) return v8::Local(); - - CFXJS_ObjDefinition* pObjDef = - CFXJS_ObjDefinition::ForID(pIsolate, nObjDefnID); + CFXJS_ObjDefintion* pObjDef = (CFXJS_ObjDefintion*)pArray->GetAt(nObjDefnID); v8::Local objTemp = v8::Local::New(pIsolate, pObjDef->m_objTemplate); @@ -406,7 +404,10 @@ v8::Local FXJS_NewFxDynamicObj(v8::Isolate* pIsolate, if (!objTemp->NewInstance(context).ToLocal(&obj)) return v8::Local(); - obj->SetAlignedPointerInInternalField(0, new CFXJS_PrivateData(nObjDefnID)); + CFXJS_PrivateData* pPrivateData = new CFXJS_PrivateData; + pPrivateData->ObjDefID = nObjDefnID; + + obj->SetAlignedPointerInInternalField(0, pPrivateData); if (pObjDef->m_pConstructor) pObjDef->m_pConstructor( pJSContext, obj, @@ -416,11 +417,12 @@ v8::Local FXJS_NewFxDynamicObj(v8::Isolate* pIsolate, } v8::Local FXJS_GetThisObj(v8::Isolate* pIsolate) { + // Return the global object. v8::Isolate::Scope isolate_scope(pIsolate); - if (!FXJS_PerIsolateData::Get(pIsolate)) + CFX_PtrArray* pArray = (CFX_PtrArray*)pIsolate->GetData(g_embedderDataSlot); + if (!pArray) return v8::Local(); - // Return the global object. v8::Local context = pIsolate->GetCurrentContext(); return context->Global()->GetPrototype()->ToObject(context).ToLocalChecked(); } @@ -446,12 +448,12 @@ v8::Isolate* FXJS_GetRuntime(v8::Local pObj) { int FXJS_GetObjDefnID(v8::Isolate* pIsolate, const wchar_t* pObjName) { v8::Isolate::Scope isolate_scope(pIsolate); - if (!FXJS_PerIsolateData::Get(pIsolate)) + CFX_PtrArray* pArray = (CFX_PtrArray*)pIsolate->GetData(g_embedderDataSlot); + if (!pArray) return -1; - int maxID = CFXJS_ObjDefinition::MaxID(pIsolate); - for (int i = 0; i < maxID; ++i) { - CFXJS_ObjDefinition* pObjDef = CFXJS_ObjDefinition::ForID(pIsolate, i); + for (int i = 0; i < pArray->GetSize(); i++) { + CFXJS_ObjDefintion* pObjDef = (CFXJS_ObjDefintion*)pArray->GetAt(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 7726c40c5e..2671a91fff 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_PerIsolateData::SetUp(m_pIsolate); + FXJS_PrepareIsolate(m_pIsolate); FXJS_InitializeRuntime(m_pIsolate, nullptr, nullptr, m_pPersistentContext); } -- cgit v1.2.3