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(
