commit 9af756aed04b3ec439645f753417859f1e3be496
author Tom Sepez <tsepez@chromium.org> Tue Jul 14 22:03:15 2020 +0000
committer Chromium commit bot <commit-bot@chromium.org> Tue Jul 14 22:03:15 2020 +0000
tree 9ca66e6c4c1ac0ccda95f75c58737e494989651f
parent 89ebf5c383918196e45cf78a0cd99d2f8891927a

Make CFXJSE_Context delete its own CXFA_ThisProxies.

Instead of having CFXJS_Engine, which is another "hop" away
clean it up on the context's behalf.

Change-Id: Ie7275a40d737128f3633c8d67526ccaca2a470f5
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/71532
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Tom Sepez <tsepez@chromium.org>

fxjs/xfa/cfxjse_context.cpp

diff --git a/fxjs/xfa/cfxjse_context.cpp b/fxjs/xfa/cfxjse_context.cpp
index 134154e..953448d 100644
--- a/fxjs/xfa/cfxjse_context.cpp
+++ b/fxjs/xfa/cfxjse_context.cpp
@@ -14,6 +14,8 @@
 #include "fxjs/xfa/cfxjse_isolatetracker.h"
 #include "fxjs/xfa/cfxjse_runtimedata.h"
 #include "fxjs/xfa/cfxjse_value.h"
+#include "fxjs/xfa/cjx_object.h"
+#include "xfa/fxfa/parser/cxfa_thisproxy.h"
 
 namespace {
 
@@ -128,6 +130,15 @@
   hObject->SetAlignedPointerInInternalField(1, nullptr);
 }
 
+CXFA_ThisProxy* ToThisProxy(CFXJSE_Value* pValue) {
+  CFXJSE_HostObject* pHostObject = pValue->ToHostObject();
+  if (!pHostObject)
+    return nullptr;
+
+  CJX_Object* pJSObject = pHostObject->AsCJXObject();
+  return pJSObject ? ToThisProxy(pJSObject->GetXFAObject()) : nullptr;
+}
+
 }  // namespace
 
 void FXJSE_UpdateObjectBinding(v8::Local<v8::Object> hObject,
@@ -209,7 +220,12 @@
 
 CFXJSE_Context::CFXJSE_Context(v8::Isolate* pIsolate) : m_pIsolate(pIsolate) {}
 
-CFXJSE_Context::~CFXJSE_Context() = default;
+CFXJSE_Context::~CFXJSE_Context() {
+  // This is what prevents leaking the CXFA_ThisProxies allocated in
+  // CXFJSE_Engine::CreateVariablesContext(). If the global object is
+  // not a thisproxy, then this degenerates to deleting nullptr, a no-op.
+  delete ToThisProxy(GetGlobalObject().get());
+}
 
 std::unique_ptr<CFXJSE_Value> CFXJSE_Context::GetGlobalObject() {
   auto pValue = std::make_unique<CFXJSE_Value>(GetIsolate());

fxjs/xfa/cfxjse_engine.cpp

diff --git a/fxjs/xfa/cfxjse_engine.cpp b/fxjs/xfa/cfxjse_engine.cpp
index 96395a9..5db71c0 100644
--- a/fxjs/xfa/cfxjse_engine.cpp
+++ b/fxjs/xfa/cfxjse_engine.cpp
@@ -72,15 +72,6 @@
 
 const char kFormCalcRuntime[] = "pfm_rt";
 
-CXFA_ThisProxy* ToThisProxy(CFXJSE_Value* pValue) {
-  CFXJSE_HostObject* pHostObject = pValue->ToHostObject();
-  if (!pHostObject)
-    return nullptr;
-
-  CJX_Object* pJSObject = pHostObject->AsCJXObject();
-  return pJSObject ? ToThisProxy(pJSObject->GetXFAObject()) : nullptr;
-}
-
 }  // namespace
 
 // static
@@ -130,11 +121,6 @@
 }
 
 CFXJSE_Engine::~CFXJSE_Engine() {
-  // This is what prevents leaking the CXFA_ThisProxies allocated in
-  // CreateVariablesContext().
-  for (const auto& pair : m_mapVariableToContext)
-    delete ToThisProxy(pair.second->GetGlobalObject().get());
-
   // This is what ensures that the v8 object bound to a CXFA_Node
   // no longer retains that binding since it will outlive that node.
   for (const auto& pair : m_mapObjectToValue)
@@ -515,7 +501,7 @@
     return nullptr;
 
   // Ownership of |proxy| is maintained through v8 bindings, and is
-  // manually freed in ~CFXJE_Engine() after re-obtaining the binding
+  // manually freed in ~CFXJE_Context() after re-obtaining the binding
   // from v8.
   auto* proxy = new CXFA_ThisProxy(pSubform, pScriptNode);
   auto pNewContext = CFXJSE_Context::Create(