Merge to master: contention over isolate data slots

Work on this was first performed on the XFA branch, since
it has additional requirements (FXJSE layer) that needed
to be accomodated by the solution.

(cherry picked from commit ed7b2b50aa1744e0bc5a60bef12c61fa91d863b7)
Original Review URL: https://codereview.chromium.org/1351173002 .

R=thestig@chromium.org

Review URL: https://codereview.chromium.org/1354593004 .

3 files changed, 97 insertions, 86 deletions

diff --git a/fpdfsdk/include/jsapi/fxjs_v8.h b/fpdfsdk/include/jsapi/fxjs_v8.h
index a1541593a1..eb810c0695 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 <v8.h>
-#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,18 @@ 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[];
@@ -33,10 +50,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..c5697c7084 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<int>(
+        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<CFXJS_ObjDefinition*>(
+        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<FXJS_PerIsolateData*>(
+      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<v8::ObjectTemplate> objTemp =
       v8::Local<v8::ObjectTemplate>::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<v8::ObjectTemplate> objTemp =
       v8::Local<v8::ObjectTemplate>::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<v8::ObjectTemplate> objTemp =
       v8::Local<v8::ObjectTemplate>::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<v8::ObjectTemplate> objTemp =
       v8::Local<v8::ObjectTemplate>::New(pIsolate, pObjDef->m_objTemplate);
   objTemp->Set(pIsolate, bsConstName.c_str(), pDefault);
@@ -187,10 +193,9 @@ static v8::Global<v8::ObjectTemplate>& _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<v8::FunctionTemplate> funTempl =
       v8::FunctionTemplate::New(pIsolate, pMethodCall);
   v8::Local<v8::ObjectTemplate> objTemp;
@@ -264,15 +267,13 @@ void FXJS_InitializeRuntime(v8::Isolate* pIsolate,
       v8::Local<v8::ObjectTemplate>::New(pIsolate, globalObjTemp));
   v8::Context::Scope context_scope(v8Context);
 
-  v8::Local<v8::External> ptr = v8::External::New(pIsolate, pFXRuntime);
-  v8Context->SetEmbedderData(1, ptr);
+  FXJS_PerIsolateData::SetUp(pIsolate);
+  FXJS_PerIsolateData* pData = FXJS_PerIsolateData::Get(pIsolate);
+  pData->m_pFXJSRuntime = pFXRuntime;
 
-  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<v8::String> 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<v8::Context>::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<v8::Object> pObj =
           v8::Local<v8::Object>::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<v8::Object> FXJS_NewFxDynamicObj(v8::Isolate* pIsolate,
                                            int nObjDefnID) {
   v8::Isolate::Scope isolate_scope(pIsolate);
   v8::Local<v8::Context> context = pIsolate->GetCurrentContext();
-  if (-1 == nObjDefnID) {
+  if (nObjDefnID == -1) {
     v8::Local<v8::ObjectTemplate> objTempl = v8::ObjectTemplate::New(pIsolate);
     v8::Local<v8::Object> obj;
     if (objTempl->NewInstance(context).ToLocal(&obj))
@@ -390,13 +390,15 @@ v8::Local<v8::Object> FXJS_NewFxDynamicObj(v8::Isolate* pIsolate,
     return v8::Local<v8::Object>();
   }
 
-  CFX_PtrArray* pArray = (CFX_PtrArray*)pIsolate->GetData(g_embedderDataSlot);
-  if (!pArray)
+  FXJS_PerIsolateData* pData = FXJS_PerIsolateData::Get(pIsolate);
+  if (!pData)
     return v8::Local<v8::Object>();
 
-  if (nObjDefnID < 0 || nObjDefnID >= pArray->GetSize())
+  if (nObjDefnID < 0 || nObjDefnID >= CFXJS_ObjDefinition::MaxID(pIsolate))
     return v8::Local<v8::Object>();
-  CFXJS_ObjDefintion* pObjDef = (CFXJS_ObjDefintion*)pArray->GetAt(nObjDefnID);
+
+  CFXJS_ObjDefinition* pObjDef =
+      CFXJS_ObjDefinition::ForID(pIsolate, nObjDefnID);
 
   v8::Local<v8::ObjectTemplate> objTemp =
       v8::Local<v8::ObjectTemplate>::New(pIsolate, pObjDef->m_objTemplate);
@@ -404,10 +406,7 @@ v8::Local<v8::Object> FXJS_NewFxDynamicObj(v8::Isolate* pIsolate,
   if (!objTemp->NewInstance(context).ToLocal(&obj))
     return v8::Local<v8::Object>();
 
-  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<v8::Object> FXJS_NewFxDynamicObj(v8::Isolate* pIsolate,
 }
 
 v8::Local<v8::Object> 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<v8::Object>();
 
+  // Return the global object.
   v8::Local<v8::Context> context = pIsolate->GetCurrentContext();
   return context->Global()->GetPrototype()->ToObject(context).ToLocalChecked();
 }
@@ -448,12 +446,12 @@ v8::Isolate* FXJS_GetRuntime(v8::Local<v8::Object> 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);
   }