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>
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());
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(