Make ownership of CXFA_ThisProxy explicit (and sane). Keep a reference to it in the CXFJSE_Context for later clean-up rather than trying to retrieve it though bound V8 objects. -- Make constructor private while we're at it. Change-Id: Id80b7cb53b33ec30ed349b4fa0d01afe5f5456b9 Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/71653 Commit-Queue: Tom Sepez <tsepez@chromium.org> Reviewed-by: Lei Zhang <thestig@chromium.org>
diff --git a/fxjs/xfa/cfxjse_context.cpp b/fxjs/xfa/cfxjse_context.cpp index 953448d..0606037 100644 --- a/fxjs/xfa/cfxjse_context.cpp +++ b/fxjs/xfa/cfxjse_context.cpp
@@ -15,6 +15,7 @@ #include "fxjs/xfa/cfxjse_runtimedata.h" #include "fxjs/xfa/cfxjse_value.h" #include "fxjs/xfa/cjx_object.h" +#include "third_party/base/ptr_util.h" #include "xfa/fxfa/parser/cxfa_thisproxy.h" namespace { @@ -130,15 +131,6 @@ 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, @@ -184,9 +176,13 @@ std::unique_ptr<CFXJSE_Context> CFXJSE_Context::Create( v8::Isolate* pIsolate, const FXJSE_CLASS_DESCRIPTOR* pGlobalClass, - CFXJSE_HostObject* pGlobalObject) { + CFXJSE_HostObject* pGlobalObject, + std::unique_ptr<CXFA_ThisProxy> pProxy) { CFXJSE_ScopeUtil_IsolateHandle scope(pIsolate); - auto pContext = std::make_unique<CFXJSE_Context>(pIsolate); + + // Private constructor. + auto pContext = + pdfium::WrapUnique(new CFXJSE_Context(pIsolate, std::move(pProxy))); v8::Local<v8::ObjectTemplate> hObjectTemplate; if (pGlobalClass) { CFXJSE_Class* pGlobalClassObj = @@ -218,14 +214,11 @@ return pContext; } -CFXJSE_Context::CFXJSE_Context(v8::Isolate* pIsolate) : m_pIsolate(pIsolate) {} +CFXJSE_Context::CFXJSE_Context(v8::Isolate* pIsolate, + std::unique_ptr<CXFA_ThisProxy> pProxy) + : m_pIsolate(pIsolate), m_pProxy(std::move(pProxy)) {} -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()); -} +CFXJSE_Context::~CFXJSE_Context() = default; std::unique_ptr<CFXJSE_Value> CFXJSE_Context::GetGlobalObject() { auto pValue = std::make_unique<CFXJSE_Value>(GetIsolate());
diff --git a/fxjs/xfa/cfxjse_context.h b/fxjs/xfa/cfxjse_context.h index 417cf23..ed302f0 100644 --- a/fxjs/xfa/cfxjse_context.h +++ b/fxjs/xfa/cfxjse_context.h
@@ -17,6 +17,7 @@ class CFXJSE_Class; class CFXJSE_HostObject; class CFXJSE_Value; +class CXFA_ThisProxy; struct FXJSE_CLASS_DESCRIPTOR; class CFXJSE_Context { @@ -24,9 +25,9 @@ static std::unique_ptr<CFXJSE_Context> Create( v8::Isolate* pIsolate, const FXJSE_CLASS_DESCRIPTOR* pGlobalClass, - CFXJSE_HostObject* pGlobalObject); + CFXJSE_HostObject* pGlobalObject, + std::unique_ptr<CXFA_ThisProxy> pProxy); - explicit CFXJSE_Context(v8::Isolate* pIsolate); ~CFXJSE_Context(); v8::Isolate* GetIsolate() const { return m_pIsolate.Get(); } @@ -40,12 +41,14 @@ CFXJSE_Value* lpNewThisObject); private: + CFXJSE_Context(v8::Isolate* pIsolate, std::unique_ptr<CXFA_ThisProxy> pProxy); CFXJSE_Context(const CFXJSE_Context&) = delete; CFXJSE_Context& operator=(const CFXJSE_Context&) = delete; v8::Global<v8::Context> m_hContext; UnownedPtr<v8::Isolate> m_pIsolate; std::vector<std::unique_ptr<CFXJSE_Class>> m_rgClasses; + std::unique_ptr<CXFA_ThisProxy> m_pProxy; }; void FXJSE_UpdateObjectBinding(v8::Local<v8::Object> hObject,
diff --git a/fxjs/xfa/cfxjse_engine.cpp b/fxjs/xfa/cfxjse_engine.cpp index fbfd008..44836c9 100644 --- a/fxjs/xfa/cfxjse_engine.cpp +++ b/fxjs/xfa/cfxjse_engine.cpp
@@ -108,7 +108,8 @@ m_pDocument(pDocument), m_JsContext(CFXJSE_Context::Create(fxjs_runtime->GetIsolate(), &GlobalClassDescriptor, - pDocument->GetRoot()->JSObject())), + pDocument->GetRoot()->JSObject(), + nullptr)), m_ResolveProcessor(std::make_unique<CFXJSE_ResolveProcessor>()) { RemoveBuiltInObjs(m_JsContext.get()); m_JsContext->EnableCompatibleMode(); @@ -499,12 +500,10 @@ if (!pScriptNode || !pSubform) return nullptr; - // Ownership of |proxy| is maintained through v8 bindings, and is - // manually freed in ~CFXJE_Context() after re-obtaining the binding - // from v8. - auto* proxy = new CXFA_ThisProxy(pSubform, pScriptNode); + auto proxy = std::make_unique<CXFA_ThisProxy>(pSubform, pScriptNode); + CJX_Object* js_object = proxy->JSObject(); auto pNewContext = CFXJSE_Context::Create( - GetIsolate(), &VariablesClassDescriptor, proxy->JSObject()); + GetIsolate(), &VariablesClassDescriptor, js_object, std::move(proxy)); RemoveBuiltInObjs(pNewContext.get()); pNewContext->EnableCompatibleMode(); CFXJSE_Context* pResult = pNewContext.get();
diff --git a/fxjs/xfa/cfxjse_formcalc_context.cpp b/fxjs/xfa/cfxjse_formcalc_context.cpp index d93f3b7..f25c263 100644 --- a/fxjs/xfa/cfxjse_formcalc_context.cpp +++ b/fxjs/xfa/cfxjse_formcalc_context.cpp
@@ -28,6 +28,7 @@ #include "xfa/fxfa/parser/cxfa_document.h" #include "xfa/fxfa/parser/cxfa_localevalue.h" #include "xfa/fxfa/parser/cxfa_node.h" +#include "xfa/fxfa/parser/cxfa_thisproxy.h" #include "xfa/fxfa/parser/cxfa_timezoneprovider.h" #include "xfa/fxfa/parser/xfa_utils.h" @@ -3208,7 +3209,7 @@ } std::unique_ptr<CFXJSE_Context> pNewContext( - CFXJSE_Context::Create(pIsolate, nullptr, nullptr)); + CFXJSE_Context::Create(pIsolate, nullptr, nullptr, nullptr)); auto returnValue = std::make_unique<CFXJSE_Value>(pIsolate); pNewContext->ExecuteScript(