Track shared isolates better in FXJSE.

Fix the asymmetry between creating only some isolates and
releasing all of them, even the shared ones, by being more
careful not to trash those we didn't create.

Review-Url: https://codereview.chromium.org/2025193002

5 files changed, 51 insertions, 72 deletions

diff --git a/fpdfsdk/fpdfxfa/fpdfxfa_app.cpp b/fpdfsdk/fpdfxfa/fpdfxfa_app.cpp
index f7a6a3ba00..56dfd47b29 100644
--- a/fpdfsdk/fpdfxfa/fpdfxfa_app.cpp
+++ b/fpdfsdk/fpdfxfa/fpdfxfa_app.cpp
@@ -32,8 +32,7 @@ CPDFXFA_App::CPDFXFA_App()
     : m_bJavaScriptInitialized(FALSE),
       m_pXFAApp(NULL),
       m_pIsolate(nullptr),
-      m_csAppType(JS_STR_VIEWERTYPE_STANDARD),
-      m_bOwnedRuntime(false) {
+      m_csAppType(JS_STR_VIEWERTYPE_STANDARD) {
   m_pEnvList.RemoveAll();
 }
 
@@ -41,29 +40,25 @@ CPDFXFA_App::~CPDFXFA_App() {
   delete m_pXFAApp;
   m_pXFAApp = NULL;
 
-#ifdef PDF_ENABLE_XFA
-  FXJSE_Runtime_Release(m_pIsolate, m_bOwnedRuntime);
+  FXJSE_Runtime_Release(m_pIsolate);
   m_pIsolate = nullptr;
 
   FXJSE_Finalize();
   BC_Library_Destory();
-#endif
 }
 
 FX_BOOL CPDFXFA_App::Initialize(v8::Isolate* pIsolate) {
-#ifdef PDF_ENABLE_XFA
   BC_Library_Init();
   FXJSE_Initialize();
 
-  m_bOwnedRuntime = !pIsolate;
-  m_pIsolate = pIsolate ? pIsolate : FXJSE_Runtime_Create();
+  m_pIsolate = pIsolate ? pIsolate : FXJSE_Runtime_Create_Own();
   if (!m_pIsolate)
     return FALSE;
 
   m_pXFAApp = new CXFA_FFApp(this);
   m_pXFAApp->SetDefaultFontMgr(
       std::unique_ptr<CXFA_DefFontMgr>(new CXFA_DefFontMgr));
-#endif
+
   return TRUE;
 }
 
diff --git a/fpdfsdk/fpdfxfa/include/fpdfxfa_app.h b/fpdfsdk/fpdfxfa/include/fpdfxfa_app.h
index b561245dcd..f24c700434 100644
--- a/fpdfsdk/fpdfxfa/include/fpdfxfa_app.h
+++ b/fpdfsdk/fpdfxfa/include/fpdfxfa_app.h
@@ -87,7 +87,6 @@ class CPDFXFA_App : public IXFA_AppProvider {
   v8::Isolate* m_pIsolate;
   IFXJS_Runtime* m_pJSRuntime;
   CFX_WideString m_csAppType;
-  bool m_bOwnedRuntime;
 };
 
 #endif  // FPDFSDK_FPDFXFA_INCLUDE_FPDFXFA_APP_H_
diff --git a/xfa/fxjse/include/fxjse.h b/xfa/fxjse/include/fxjse.h
index 4a85251c03..ab95a6d675 100644
--- a/xfa/fxjse/include/fxjse.h
+++ b/xfa/fxjse/include/fxjse.h
@@ -69,8 +69,8 @@ struct FXJSE_CLASS_DESCRIPTOR {
 void FXJSE_Initialize();
 void FXJSE_Finalize();
 
-v8::Isolate* FXJSE_Runtime_Create();
-void FXJSE_Runtime_Release(v8::Isolate* pIsolate, bool bOwnedRuntime);
+v8::Isolate* FXJSE_Runtime_Create_Own();
+void FXJSE_Runtime_Release(v8::Isolate* pIsolate);
 
 CFXJSE_Context* FXJSE_Context_Create(
     v8::Isolate* pIsolate,
diff --git a/xfa/fxjse/runtime.cpp b/xfa/fxjse/runtime.cpp
index 70d90c44fa..c3d12c3745 100644
--- a/xfa/fxjse/runtime.cpp
+++ b/xfa/fxjse/runtime.cpp
@@ -24,56 +24,49 @@ static void FXJSE_KillV8() {
 }
 
 void FXJSE_Initialize() {
-  if (!CFXJSE_RuntimeData::g_RuntimeList) {
-    CFXJSE_RuntimeData::g_RuntimeList = new CFXJSE_RuntimeList;
-  }
+  if (!CFXJSE_IsolateTracker::g_pInstance)
+    CFXJSE_IsolateTracker::g_pInstance = new CFXJSE_IsolateTracker;
+
   static FX_BOOL bV8Initialized = FALSE;
-  if (bV8Initialized) {
+  if (bV8Initialized)
     return;
-  }
+
   bV8Initialized = TRUE;
   atexit(FXJSE_KillV8);
 }
 
-static void FXJSE_Runtime_DisposeCallback(v8::Isolate* pIsolate) {
+static void FXJSE_Runtime_DisposeCallback(v8::Isolate* pIsolate, bool bOwned) {
   if (FXJS_PerIsolateData* pData = FXJS_PerIsolateData::Get(pIsolate)) {
     delete pData->m_pFXJSERuntimeData;
     pData->m_pFXJSERuntimeData = nullptr;
   }
-  pIsolate->Dispose();
+  if (bOwned)
+    pIsolate->Dispose();
 }
 
 void FXJSE_Finalize() {
-  if (CFXJSE_RuntimeData::g_RuntimeList) {
-    CFXJSE_RuntimeData::g_RuntimeList->RemoveAllRuntimes(
-        FXJSE_Runtime_DisposeCallback);
-    delete CFXJSE_RuntimeData::g_RuntimeList;
-    CFXJSE_RuntimeData::g_RuntimeList = NULL;
-  }
+  if (!CFXJSE_IsolateTracker::g_pInstance)
+    return;
+
+  CFXJSE_IsolateTracker::g_pInstance->RemoveAll(FXJSE_Runtime_DisposeCallback);
+  delete CFXJSE_IsolateTracker::g_pInstance;
+  CFXJSE_IsolateTracker::g_pInstance = nullptr;
 }
 
-v8::Isolate* FXJSE_Runtime_Create() {
+v8::Isolate* FXJSE_Runtime_Create_Own() {
   v8::Isolate::CreateParams params;
   params.array_buffer_allocator = new FXJSE_ArrayBufferAllocator();
   v8::Isolate* pIsolate = v8::Isolate::New(params);
-  ASSERT(pIsolate && CFXJSE_RuntimeData::g_RuntimeList);
-  CFXJSE_RuntimeData::g_RuntimeList->AppendRuntime(pIsolate);
+  ASSERT(pIsolate && CFXJSE_IsolateTracker::g_pInstance);
+  CFXJSE_IsolateTracker::g_pInstance->Append(pIsolate);
   return pIsolate;
 }
 
-void FXJSE_Runtime_Release(v8::Isolate* pIsolate, bool bOwnedRuntime) {
+void FXJSE_Runtime_Release(v8::Isolate* pIsolate) {
   if (!pIsolate)
     return;
-  if (bOwnedRuntime) {
-    ASSERT(CFXJSE_RuntimeData::g_RuntimeList);
-    CFXJSE_RuntimeData::g_RuntimeList->RemoveRuntime(
-        pIsolate, FXJSE_Runtime_DisposeCallback);
-  } else {
-    if (FXJS_PerIsolateData* pData = FXJS_PerIsolateData::Get(pIsolate)) {
-      delete pData->m_pFXJSERuntimeData;
-      pData->m_pFXJSERuntimeData = nullptr;
-    }
-  }
+  CFXJSE_IsolateTracker::g_pInstance->Remove(pIsolate,
+                                             FXJSE_Runtime_DisposeCallback);
 }
 
 CFXJSE_RuntimeData* CFXJSE_RuntimeData::Create(v8::Isolate* pIsolate) {
@@ -97,27 +90,26 @@ CFXJSE_RuntimeData* CFXJSE_RuntimeData::Get(v8::Isolate* pIsolate) {
   return pData->m_pFXJSERuntimeData;
 }
 
-CFXJSE_RuntimeList* CFXJSE_RuntimeData::g_RuntimeList = nullptr;
+CFXJSE_IsolateTracker* CFXJSE_IsolateTracker::g_pInstance = nullptr;
 
-void CFXJSE_RuntimeList::AppendRuntime(v8::Isolate* pIsolate) {
-  m_RuntimeList.push_back(pIsolate);
+void CFXJSE_IsolateTracker::Append(v8::Isolate* pIsolate) {
+  m_OwnedIsolates.push_back(pIsolate);
 }
 
-void CFXJSE_RuntimeList::RemoveRuntime(
+void CFXJSE_IsolateTracker::Remove(
     v8::Isolate* pIsolate,
-    CFXJSE_RuntimeList::RuntimeDisposeCallback lpfnDisposeCallback) {
-  auto it = std::find(m_RuntimeList.begin(), m_RuntimeList.end(), pIsolate);
-  if (it != m_RuntimeList.end())
-    m_RuntimeList.erase(it);
-  if (lpfnDisposeCallback)
-    lpfnDisposeCallback(pIsolate);
+    CFXJSE_IsolateTracker::DisposeCallback lpfnDisposeCallback) {
+  auto it = std::find(m_OwnedIsolates.begin(), m_OwnedIsolates.end(), pIsolate);
+  bool bFound = it != m_OwnedIsolates.end();
+  if (bFound)
+    m_OwnedIsolates.erase(it);
+  lpfnDisposeCallback(pIsolate, bFound);
 }
 
-void CFXJSE_RuntimeList::RemoveAllRuntimes(
-    CFXJSE_RuntimeList::RuntimeDisposeCallback lpfnDisposeCallback) {
-  if (lpfnDisposeCallback) {
-    for (v8::Isolate* pIsolate : m_RuntimeList)
-      lpfnDisposeCallback(pIsolate);
-  }
-  m_RuntimeList.clear();
+void CFXJSE_IsolateTracker::RemoveAll(
+    CFXJSE_IsolateTracker::DisposeCallback lpfnDisposeCallback) {
+  for (v8::Isolate* pIsolate : m_OwnedIsolates)
+    lpfnDisposeCallback(pIsolate, true);
+
+  m_OwnedIsolates.clear();
 }
diff --git a/xfa/fxjse/runtime.h b/xfa/fxjse/runtime.h
index 1240089be4..b57efe1df7 100644
--- a/xfa/fxjse/runtime.h
+++ b/xfa/fxjse/runtime.h
@@ -15,39 +15,32 @@
 class CFXJSE_RuntimeList;
 
 class CFXJSE_RuntimeData {
- protected:
-  CFXJSE_RuntimeData(v8::Isolate* pIsolate) : m_pIsolate(pIsolate) {}
-
  public:
-  static CFXJSE_RuntimeData* Create(v8::Isolate* pIsolate);
   static CFXJSE_RuntimeData* Get(v8::Isolate* pIsolate);
 
- public:
   v8::Isolate* m_pIsolate;
   v8::Global<v8::FunctionTemplate> m_hRootContextGlobalTemplate;
   v8::Global<v8::Context> m_hRootContext;
 
- public:
-  static CFXJSE_RuntimeList* g_RuntimeList;
-
  protected:
+  static CFXJSE_RuntimeData* Create(v8::Isolate* pIsolate);
+  CFXJSE_RuntimeData(v8::Isolate* pIsolate) : m_pIsolate(pIsolate) {}
   CFXJSE_RuntimeData();
   CFXJSE_RuntimeData(const CFXJSE_RuntimeData&);
   CFXJSE_RuntimeData& operator=(const CFXJSE_RuntimeData&);
 };
 
-class CFXJSE_RuntimeList {
+class CFXJSE_IsolateTracker {
  public:
-  typedef void (*RuntimeDisposeCallback)(v8::Isolate*);
+  typedef void (*DisposeCallback)(v8::Isolate*, bool bOwnedIsolate);
+  static CFXJSE_IsolateTracker* g_pInstance;
 
- public:
-  void AppendRuntime(v8::Isolate* pIsolate);
-  void RemoveRuntime(v8::Isolate* pIsolate,
-                     RuntimeDisposeCallback lpfnDisposeCallback);
-  void RemoveAllRuntimes(RuntimeDisposeCallback lpfnDisposeCallback);
+  void Append(v8::Isolate* pIsolate);
+  void Remove(v8::Isolate* pIsolate, DisposeCallback lpfnDisposeCallback);
+  void RemoveAll(DisposeCallback lpfnDisposeCallback);
 
  protected:
-  std::vector<v8::Isolate*> m_RuntimeList;
+  std::vector<v8::Isolate*> m_OwnedIsolates;
 };
 
 #endif  // XFA_FXJSE_RUNTIME_H_