From a00726d223bd6985f0199d26e5ec7196182c114e Mon Sep 17 00:00:00 2001 From: Jon Coppeard Date: Mon, 12 Aug 2019 10:43:32 +0000 Subject: [PATCH] Bug 1572782 - Remove js::FreeOp and make JSFreeOp opaque in public API r=tcampbell? Merge js::FreeOp and JSFreeOp, but alias the former to the latter while we fix uses. Differential Revision: https://phabricator.services.mozilla.com/D41410 --HG-- extra : moz-landing-system : lando --- dom/bindings/Codegen.py | 2 +- js/public/Class.h | 2 - js/public/GCAPI.h | 2 - js/public/MemoryFunctions.h | 14 +------ js/public/TypeDecls.h | 3 +- js/src/builtin/ModuleObject.cpp | 3 +- js/src/builtin/intl/Collator.h | 1 - js/src/builtin/intl/DateTimeFormat.h | 1 - js/src/builtin/intl/NumberFormat.h | 1 - js/src/builtin/intl/PluralRules.h | 2 - js/src/builtin/intl/RelativeTimeFormat.h | 2 - js/src/ctypes/CTypes.cpp | 17 ++++----- js/src/gc/ArenaList.h | 1 - js/src/gc/FreeOp-inl.h | 28 ++++++-------- js/src/gc/FreeOp.h | 47 ++++++++++++++++-------- js/src/gc/GC.h | 1 - js/src/gc/Heap.h | 1 - js/src/jsapi.cpp | 2 +- js/src/jsapi.h | 1 - js/src/vm/Runtime.cpp | 6 +-- 20 files changed, 59 insertions(+), 78 deletions(-) diff --git a/dom/bindings/Codegen.py b/dom/bindings/Codegen.py index 97e2de20853e..0e92c0538cfd 100644 --- a/dom/bindings/Codegen.py +++ b/dom/bindings/Codegen.py @@ -1764,7 +1764,7 @@ def finalizeHook(descriptor, hookName, freeOp, obj): if descriptor.wrapperCache: finalize += "ClearWrapper(self, self, %s);\n" % obj if descriptor.isGlobal(): - finalize += "mozilla::dom::FinalizeGlobal(CastToJSFreeOp(%s), %s);\n" % (freeOp, obj) + finalize += "mozilla::dom::FinalizeGlobal(js::CastToJSFreeOp(%s), %s);\n" % (freeOp, obj) finalize += fill( """ if (size_t mallocBytes = BindingJSObjectMallocBytes(self)) { diff --git a/js/public/Class.h b/js/public/Class.h index 564bcb6a2fc7..fdfd73dc4ce4 100644 --- a/js/public/Class.h +++ b/js/public/Class.h @@ -25,13 +25,11 @@ */ struct JSAtomState; -struct JSFreeOp; struct JSFunctionSpec; namespace js { struct Class; -class FreeOp; class Shape; // This is equal to JSFunction::class_. Use it in places where you don't want diff --git a/js/public/GCAPI.h b/js/public/GCAPI.h index 31eaba4a7b72..7307e030c5e3 100644 --- a/js/public/GCAPI.h +++ b/js/public/GCAPI.h @@ -19,8 +19,6 @@ #include "js/UniquePtr.h" #include "js/Utility.h" -struct JSFreeOp; - #ifdef JS_BROKEN_GCC_ATTRIBUTE_WARNING # pragma GCC diagnostic push # pragma GCC diagnostic ignored "-Wattributes" diff --git a/js/public/MemoryFunctions.h b/js/public/MemoryFunctions.h index 8712e34c93a8..b4e19c698488 100644 --- a/js/public/MemoryFunctions.h +++ b/js/public/MemoryFunctions.h @@ -16,22 +16,10 @@ #include "jstypes.h" // JS_PUBLIC_API struct JSContext; +class JSFreeOp; class JSObject; struct JSRuntime; -struct JSFreeOp { - protected: - JSRuntime* runtime_; - - explicit JSFreeOp(JSRuntime* rt) : runtime_(rt) {} - - public: - JSRuntime* runtime() const { - MOZ_ASSERT(runtime_); - return runtime_; - } -}; - extern JS_PUBLIC_API void* JS_malloc(JSContext* cx, size_t nbytes); extern JS_PUBLIC_API void* JS_realloc(JSContext* cx, void* p, size_t oldBytes, diff --git a/js/public/TypeDecls.h b/js/public/TypeDecls.h index 1aa1b84fa870..8fba20ceec2e 100644 --- a/js/public/TypeDecls.h +++ b/js/public/TypeDecls.h @@ -27,13 +27,14 @@ typedef uint8_t jsbytecode; class JSAtom; struct JSContext; class JSFunction; +class JSFreeOp; class JSObject; struct JSRuntime; class JSScript; class JSString; -struct JSFreeOp; namespace js { +using FreeOp = JSFreeOp; class TempAllocPolicy; }; // namespace js diff --git a/js/src/builtin/ModuleObject.cpp b/js/src/builtin/ModuleObject.cpp index c67838aac593..46c30dfae516 100644 --- a/js/src/builtin/ModuleObject.cpp +++ b/js/src/builtin/ModuleObject.cpp @@ -679,9 +679,8 @@ void ModuleNamespaceObject::ProxyHandler::trace(JSTracer* trc, } } -void ModuleNamespaceObject::ProxyHandler::finalize(JSFreeOp* fopArg, +void ModuleNamespaceObject::ProxyHandler::finalize(JSFreeOp* fop, JSObject* proxy) const { - FreeOp* fop = FreeOp::get(fopArg); auto& self = proxy->as(); if (self.hasBindings()) { diff --git a/js/src/builtin/intl/Collator.h b/js/src/builtin/intl/Collator.h index 4bb2dba22f3e..43c4084f14e7 100644 --- a/js/src/builtin/intl/Collator.h +++ b/js/src/builtin/intl/Collator.h @@ -17,7 +17,6 @@ namespace js { -class FreeOp; class GlobalObject; /******************** Collator ********************/ diff --git a/js/src/builtin/intl/DateTimeFormat.h b/js/src/builtin/intl/DateTimeFormat.h index 8c1cca0df8d9..ce8f0b6e5b8a 100644 --- a/js/src/builtin/intl/DateTimeFormat.h +++ b/js/src/builtin/intl/DateTimeFormat.h @@ -17,7 +17,6 @@ namespace js { -class FreeOp; class GlobalObject; class DateTimeFormatObject : public NativeObject { diff --git a/js/src/builtin/intl/NumberFormat.h b/js/src/builtin/intl/NumberFormat.h index 3c8cf7326e28..2e4fb33bf48f 100644 --- a/js/src/builtin/intl/NumberFormat.h +++ b/js/src/builtin/intl/NumberFormat.h @@ -24,7 +24,6 @@ struct UNumberFormatter; namespace js { class ArrayObject; -class FreeOp; class NumberFormatObject : public NativeObject { public: diff --git a/js/src/builtin/intl/PluralRules.h b/js/src/builtin/intl/PluralRules.h index 12e5224e0e20..7a920099bb08 100644 --- a/js/src/builtin/intl/PluralRules.h +++ b/js/src/builtin/intl/PluralRules.h @@ -20,8 +20,6 @@ struct UPluralRules; namespace js { -class FreeOp; - class PluralRulesObject : public NativeObject { public: static const Class class_; diff --git a/js/src/builtin/intl/RelativeTimeFormat.h b/js/src/builtin/intl/RelativeTimeFormat.h index b3c71404cb30..73864027a1dd 100644 --- a/js/src/builtin/intl/RelativeTimeFormat.h +++ b/js/src/builtin/intl/RelativeTimeFormat.h @@ -17,8 +17,6 @@ namespace js { -class FreeOp; - class RelativeTimeFormatObject : public NativeObject { public: static const Class class_; diff --git a/js/src/ctypes/CTypes.cpp b/js/src/ctypes/CTypes.cpp index 50f99db3cc40..711727579ff3 100644 --- a/js/src/ctypes/CTypes.cpp +++ b/js/src/ctypes/CTypes.cpp @@ -4365,9 +4365,8 @@ static void FinalizeFFIType(JSFreeOp* fop, JSObject* obj, const Value& slot, size_t elementCount) { ffi_type* ffiType = static_cast(slot.toPrivate()); size_t size = elementCount * sizeof(ffi_type*); - FreeOp::get(fop)->free_(obj, ffiType->elements, size, - MemoryUse::CTypeFFITypeElements); - FreeOp::get(fop)->delete_(obj, ffiType, MemoryUse::CTypeFFIType); + fop->free_(obj, ffiType->elements, size, MemoryUse::CTypeFFITypeElements); + fop->delete_(obj, ffiType, MemoryUse::CTypeFFIType); } void CType::Finalize(JSFreeOp* fop, JSObject* obj) { @@ -4384,7 +4383,7 @@ void CType::Finalize(JSFreeOp* fop, JSObject* obj) { slot = JS_GetReservedSlot(obj, SLOT_FNINFO); if (!slot.isUndefined()) { auto fninfo = static_cast(slot.toPrivate()); - FreeOp::get(fop)->delete_(obj, fninfo, MemoryUse::CTypeFunctionInfo); + fop->delete_(obj, fninfo, MemoryUse::CTypeFunctionInfo); } break; } @@ -4397,7 +4396,7 @@ void CType::Finalize(JSFreeOp* fop, JSObject* obj) { if (!slot.isUndefined()) { auto info = static_cast(slot.toPrivate()); fieldCount = info->count(); - FreeOp::get(fop)->delete_(obj, info, MemoryUse::CTypeFieldInfo); + fop->delete_(obj, info, MemoryUse::CTypeFieldInfo); } // Free the ffi_type info. @@ -7268,7 +7267,7 @@ void CClosure::Finalize(JSFreeOp* fop, JSObject* obj) { } ClosureInfo* cinfo = static_cast(slot.toPrivate()); - FreeOp::get(fop)->delete_(obj, cinfo, MemoryUse::CClosureInfo); + fop->delete_(obj, cinfo, MemoryUse::CClosureInfo); } void CClosure::ClosureStub(ffi_cif* cif, void* result, void** args, @@ -7532,9 +7531,9 @@ void CData::Finalize(JSFreeOp* fop, JSObject* obj) { if (owns) { JSObject* typeObj = &JS_GetReservedSlot(obj, SLOT_CTYPE).toObject(); size_t size = CType::GetSize(typeObj); - FreeOp::get(fop)->free_(obj, *buffer, size, MemoryUse::CDataBuffer); + fop->free_(obj, *buffer, size, MemoryUse::CDataBuffer); } - FreeOp::get(fop)->delete_(obj, buffer, MemoryUse::CDataBufferPtr); + fop->delete_(obj, buffer, MemoryUse::CDataBufferPtr); } JSObject* CData::GetCType(JSObject* dataObj) { @@ -8463,7 +8462,7 @@ void Int64Base::Finalize(JSFreeOp* fop, JSObject* obj) { } uint64_t* buffer = static_cast(slot.toPrivate()); - FreeOp::get(fop)->delete_(obj, buffer, MemoryUse::CTypesInt64); + fop->delete_(obj, buffer, MemoryUse::CTypesInt64); } uint64_t Int64Base::GetInt(JSObject* obj) { diff --git a/js/src/gc/ArenaList.h b/js/src/gc/ArenaList.h index 6433c8ac283c..7c8961979fa6 100644 --- a/js/src/gc/ArenaList.h +++ b/js/src/gc/ArenaList.h @@ -18,7 +18,6 @@ namespace js { -class FreeOp; class Nursery; class TenuringTracer; diff --git a/js/src/gc/FreeOp-inl.h b/js/src/gc/FreeOp-inl.h index c2f6c358fc2f..24b066254f0b 100644 --- a/js/src/gc/FreeOp-inl.h +++ b/js/src/gc/FreeOp-inl.h @@ -12,51 +12,45 @@ #include "gc/ZoneAllocator.h" #include "js/RefCounted.h" -namespace js { - -inline void FreeOp::free_(gc::Cell* cell, void* p, size_t nbytes, - MemoryUse use) { +inline void JSFreeOp::free_(Cell* cell, void* p, size_t nbytes, MemoryUse use) { if (p) { removeCellMemory(cell, nbytes, use); js_free(p); } } -inline void FreeOp::freeLater(gc::Cell* cell, void* p, size_t nbytes, - MemoryUse use) { +inline void JSFreeOp::freeLater(Cell* cell, void* p, size_t nbytes, + MemoryUse use) { if (p) { removeCellMemory(cell, nbytes, use); queueForFreeLater(p); } } -inline void FreeOp::queueForFreeLater(void* p) { - // Default FreeOps are not constructed on the stack, and will hold onto the +inline void JSFreeOp::queueForFreeLater(void* p) { + // Default JSFreeOps are not constructed on the stack, and will hold onto the // pointers to free indefinitely. MOZ_ASSERT(!isDefaultFreeOp()); // It's not safe to free this allocation immediately, so we must crash if we // can't append to the list. - AutoEnterOOMUnsafeRegion oomUnsafe; + js::AutoEnterOOMUnsafeRegion oomUnsafe; if (!freeLaterList.append(p)) { - oomUnsafe.crash("FreeOp::freeLater"); + oomUnsafe.crash("JSFreeOp::freeLater"); } } template -inline void FreeOp::release(gc::Cell* cell, T* p, size_t nbytes, - MemoryUse use) { +inline void JSFreeOp::release(Cell* cell, T* p, size_t nbytes, MemoryUse use) { if (p) { removeCellMemory(cell, nbytes, use); p->Release(); } } -inline void FreeOp::removeCellMemory(gc::Cell* cell, size_t nbytes, - MemoryUse use) { +inline void JSFreeOp::removeCellMemory(Cell* cell, size_t nbytes, + MemoryUse use) { RemoveCellMemory(cell, nbytes, use, isCollecting()); } -} // namespace js - -#endif // gc_FreeOp_inl_h +#endif // gc_JSFreeOp_inl_h diff --git a/js/src/gc/FreeOp.h b/js/src/gc/FreeOp.h index d7fbdd35d338..3d6f849d929a 100644 --- a/js/src/gc/FreeOp.h +++ b/js/src/gc/FreeOp.h @@ -19,10 +19,10 @@ struct JSRuntime; namespace js { - namespace gc { class AutoSetThreadIsPerformingGC; } // namespace gc +} // namespace js /* * A FreeOp can do one thing: free memory. For convenience, it has delete_ @@ -31,19 +31,32 @@ class AutoSetThreadIsPerformingGC; * FreeOp is passed to finalizers and other sweep-phase hooks so that we do not * need to pass a JSContext to those hooks. */ -class FreeOp : public JSFreeOp { - Vector freeLaterList; - jit::JitPoisonRangeVector jitPoisonRanges; +class JSFreeOp { + using Cell = js::gc::Cell; + using MemoryUse = js::MemoryUse; + + JSRuntime* runtime_; + + // We may accumulate a set of deferred free operations to be performed when + // the JSFreeOp is destroyed. This only applies to non-default JSFreeOps that + // are stack allocated and used during GC sweeping. + js::Vector freeLaterList; + + js::jit::JitPoisonRangeVector jitPoisonRanges; + const bool isDefault; bool isCollecting_; - friend class gc::AutoSetThreadIsPerformingGC; + friend class js::gc::AutoSetThreadIsPerformingGC; public: - static FreeOp* get(JSFreeOp* fop) { return static_cast(fop); } + explicit JSFreeOp(JSRuntime* maybeRuntime, bool isDefault = false); + ~JSFreeOp(); - explicit FreeOp(JSRuntime* maybeRuntime, bool isDefault = false); - ~FreeOp(); + JSRuntime* runtime() const { + MOZ_ASSERT(runtime_); + return runtime_; + } bool onMainThread() const { return runtime_ != nullptr; } @@ -65,7 +78,7 @@ class FreeOp : public JSFreeOp { // The memory should have been associated with the GC thing using // js::InitReservedSlot or js::InitObjectPrivate, or possibly // js::AddCellMemory. - void free_(gc::Cell* cell, void* p, size_t nbytes, MemoryUse use); + void free_(Cell* cell, void* p, size_t nbytes, MemoryUse use); // Deprecated. Where possible, memory should be tracked against the owning GC // thing by calling js::AddCellMemory and the memory freed with freeLater() @@ -81,9 +94,9 @@ class FreeOp : public JSFreeOp { // // This is used to ensure that copy-on-write object elements are not freed // until all objects that refer to them have been finalized. - void freeLater(gc::Cell* cell, void* p, size_t nbytes, MemoryUse use); + void freeLater(Cell* cell, void* p, size_t nbytes, MemoryUse use); - bool appendJitPoisonRange(const jit::JitPoisonRange& range) { + bool appendJitPoisonRange(const js::jit::JitPoisonRange& range) { // FreeOps other than the defaultFreeOp() are constructed on the stack, // and won't hold onto the pointers to free indefinitely. MOZ_ASSERT(!isDefaultFreeOp()); @@ -109,7 +122,7 @@ class FreeOp : public JSFreeOp { // js::InitReservedSlot or js::InitObjectPrivate, or possibly // js::AddCellMemory. template - void delete_(gc::Cell* cell, T* p, MemoryUse use) { + void delete_(Cell* cell, T* p, MemoryUse use) { delete_(cell, p, sizeof(T), use); } @@ -120,7 +133,7 @@ class FreeOp : public JSFreeOp { // js::InitReservedSlot or js::InitObjectPrivate, or possibly // js::AddCellMemory. template - void delete_(gc::Cell* cell, T* p, size_t nbytes, MemoryUse use) { + void delete_(Cell* cell, T* p, size_t nbytes, MemoryUse use) { if (p) { p->~T(); free_(cell, p, nbytes, use); @@ -139,7 +152,7 @@ class FreeOp : public JSFreeOp { // each zone. If this is the case then some other form of accounting would be // more appropriate. template - void release(gc::Cell* cell, T* p, MemoryUse use) { + void release(Cell* cell, T* p, MemoryUse use) { release(cell, p, sizeof(T), use); } @@ -150,16 +163,18 @@ class FreeOp : public JSFreeOp { // js::InitReservedSlot or js::InitObjectPrivate, or possibly // js::AddCellMemory. template - void release(gc::Cell* cell, T* p, size_t nbytes, MemoryUse use); + void release(Cell* cell, T* p, size_t nbytes, MemoryUse use); // Update the memory accounting for a GC for memory freed by some other // method. - void removeCellMemory(gc::Cell* cell, size_t nbytes, MemoryUse use); + void removeCellMemory(Cell* cell, size_t nbytes, MemoryUse use); private: void queueForFreeLater(void* p); }; +namespace js { +using FreeOp = JSFreeOp; } // namespace js #endif // gc_FreeOp_h diff --git a/js/src/gc/GC.h b/js/src/gc/GC.h index 4773cb612a25..32910b3360b5 100644 --- a/js/src/gc/GC.h +++ b/js/src/gc/GC.h @@ -25,7 +25,6 @@ namespace js { class AccessorShape; class FatInlineAtom; -class FreeOp; class NormalAtom; class Nursery; diff --git a/js/src/gc/Heap.h b/js/src/gc/Heap.h index 911d34b55aff..4494e42e21f1 100644 --- a/js/src/gc/Heap.h +++ b/js/src/gc/Heap.h @@ -21,7 +21,6 @@ namespace js { class AutoLockGC; class AutoLockGCBgAlloc; -class FreeOp; class NurseryDecommitTask; namespace gc { diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp index 9899b7d777c1..219af5be56c2 100644 --- a/js/src/jsapi.cpp +++ b/js/src/jsapi.cpp @@ -1146,7 +1146,7 @@ JS_PUBLIC_API void* JS_string_realloc(JSContext* cx, void* p, size_t oldBytes, JS_PUBLIC_API void JS_string_free(JSContext* cx, void* p) { return js_free(p); } JS_PUBLIC_API void JS_freeop(JSFreeOp* fop, void* p) { - return FreeOp::get(fop)->freeUntracked(p); + return fop->freeUntracked(p); } JS_PUBLIC_API void JS::AddAssociatedMemory(JSObject* obj, size_t nbytes, diff --git a/js/src/jsapi.h b/js/src/jsapi.h index ddcd2669391a..21c214c9063c 100644 --- a/js/src/jsapi.h +++ b/js/src/jsapi.h @@ -52,7 +52,6 @@ /************************************************************************/ -struct JSFreeOp; struct JSFunctionSpec; struct JSPropertySpec; diff --git a/js/src/vm/Runtime.cpp b/js/src/vm/Runtime.cpp index 2d1f34eb6c64..7981479e9648 100644 --- a/js/src/vm/Runtime.cpp +++ b/js/src/vm/Runtime.cpp @@ -567,12 +567,12 @@ void JSRuntime::traceSharedIntlData(JSTracer* trc) { sharedIntlData.ref().trace(trc); } -FreeOp::FreeOp(JSRuntime* maybeRuntime, bool isDefault) - : JSFreeOp(maybeRuntime), isDefault(isDefault), isCollecting_(!isDefault) { +JSFreeOp::JSFreeOp(JSRuntime* maybeRuntime, bool isDefault) + : runtime_(maybeRuntime), isDefault(isDefault), isCollecting_(!isDefault) { MOZ_ASSERT_IF(maybeRuntime, CurrentThreadCanAccessRuntime(maybeRuntime)); } -FreeOp::~FreeOp() { +JSFreeOp::~JSFreeOp() { for (size_t i = 0; i < freeLaterList.length(); i++) { freeUntracked(freeLaterList[i]); }