Remove remaining NoDestructor usage
Replace remaining NoDestructor usage with allocations at PDFium library
initialization time, and corresponding deletions at library destruction
time.
Bug: pdfium:1876
Change-Id: I658cbe12ea37ceccace4cbef58a39dea37fa8110
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/116751
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Thomas Sepez <tsepez@google.com>
Commit-Queue: Lei Zhang <thestig@chromium.org>
diff --git a/core/fxcrt/fx_memory.h b/core/fxcrt/fx_memory.h
index 477823c..c9fa991 100644
--- a/core/fxcrt/fx_memory.h
+++ b/core/fxcrt/fx_memory.h
@@ -31,6 +31,7 @@
#endif
void FX_InitializeMemoryAllocators();
+void FX_DestroyMemoryAllocators();
NOINLINE void FX_OutOfMemoryTerminate(size_t size);
// General Partition Allocators.
diff --git a/core/fxcrt/fx_memory_malloc.cpp b/core/fxcrt/fx_memory_malloc.cpp
index e199d42..bc958dd 100644
--- a/core/fxcrt/fx_memory_malloc.cpp
+++ b/core/fxcrt/fx_memory_malloc.cpp
@@ -68,6 +68,8 @@
void FX_InitializeMemoryAllocators() {}
+void FX_DestroyMemoryAllocators() {}
+
void* FX_ArrayBufferAllocate(size_t length) {
void* result = calloc(length, 1);
if (!result)
diff --git a/core/fxcrt/fx_memory_pa.cpp b/core/fxcrt/fx_memory_pa.cpp
index a30fc1a..be6e2af 100644
--- a/core/fxcrt/fx_memory_pa.cpp
+++ b/core/fxcrt/fx_memory_pa.cpp
@@ -8,7 +8,6 @@
#include "core/fxcrt/fx_safe_types.h"
#include "partition_alloc/partition_alloc.h"
-#include "third_party/base/no_destructor.h"
#if !defined(PDF_USE_PARTITION_ALLOC)
#error "File compiled under wrong build option."
@@ -18,24 +17,29 @@
constexpr partition_alloc::PartitionOptions kOptions = {};
+struct Allocators {
+#ifndef V8_ENABLE_SANDBOX
+ partition_alloc::PartitionAllocator array_buffer_allocator{kOptions};
+#endif
+
+ partition_alloc::PartitionAllocator general_allocator{kOptions};
+ partition_alloc::PartitionAllocator string_allocator{kOptions};
+};
+
+Allocators* g_allocators = nullptr;
+
#ifndef V8_ENABLE_SANDBOX
partition_alloc::PartitionAllocator& GetArrayBufferPartitionAllocator() {
- static pdfium::base::NoDestructor<partition_alloc::PartitionAllocator>
- s_array_buffer_allocator(kOptions);
- return *s_array_buffer_allocator;
+ return g_allocators->array_buffer_allocator;
}
-#endif // V8_ENABLE_SANDBOX
+#endif
partition_alloc::PartitionAllocator& GetGeneralPartitionAllocator() {
- static pdfium::base::NoDestructor<partition_alloc::PartitionAllocator>
- s_general_allocator(kOptions);
- return *s_general_allocator;
+ return g_allocators->general_allocator;
}
partition_alloc::PartitionAllocator& GetStringPartitionAllocator() {
- static pdfium::base::NoDestructor<partition_alloc::PartitionAllocator>
- s_string_allocator(kOptions);
- return *s_string_allocator;
+ return g_allocators->string_allocator;
}
} // namespace
@@ -122,20 +126,16 @@
} // namespace pdfium::internal
void FX_InitializeMemoryAllocators() {
- static bool s_partition_allocators_initialized = false;
- if (!s_partition_allocators_initialized) {
- partition_alloc::PartitionAllocGlobalInit(FX_OutOfMemoryTerminate);
- // These calls force the allocators to be created and initialized (via magic
- // of static local variables).
-#ifndef V8_ENABLE_SANDBOX
- GetArrayBufferPartitionAllocator();
-#endif // V8_ENABLE_SANDBOX
- GetGeneralPartitionAllocator();
- GetStringPartitionAllocator();
- s_partition_allocators_initialized = true;
+ if (!g_allocators) {
+ g_allocators = new Allocators();
}
}
+void FX_DestroyMemoryAllocators() {
+ delete g_allocators;
+ g_allocators = nullptr;
+}
+
#ifndef V8_ENABLE_SANDBOX
void* FX_ArrayBufferAllocate(size_t length) {
return GetArrayBufferPartitionAllocator()
diff --git a/fpdfsdk/fpdf_view.cpp b/fpdfsdk/fpdf_view.cpp
index 1507230..0f0c6ac 100644
--- a/fpdfsdk/fpdf_view.cpp
+++ b/fpdfsdk/fpdf_view.cpp
@@ -57,7 +57,6 @@
#ifdef PDF_ENABLE_V8
#include "fxjs/cfx_v8_array_buffer_allocator.h"
-#include "third_party/base/no_destructor.h"
#endif
#ifdef PDF_ENABLE_XFA
@@ -274,6 +273,7 @@
CPDF_PageModule::Destroy();
CFX_GEModule::Destroy();
CFX_Timer::DestroyGlobals();
+ FX_DestroyMemoryAllocators();
g_bLibraryInitialized = false;
}
@@ -1151,8 +1151,11 @@
}
FPDF_EXPORT void* FPDF_CALLCONV FPDF_GetArrayBufferAllocatorSharedInstance() {
- static pdfium::base::NoDestructor<CFX_V8ArrayBufferAllocator> allocator;
- return allocator.get();
+ // Deliberately leaked. This allocator is used outside of the library
+ // initialization / destruction lifecycle, and the caller does not take
+ // ownership of the object. Thus there is no existing way to delete this.
+ static auto* s_allocator = new CFX_V8ArrayBufferAllocator();
+ return s_allocator;
}
#endif // PDF_ENABLE_V8
diff --git a/fpdfsdk/fpdf_view_embeddertest.cpp b/fpdfsdk/fpdf_view_embeddertest.cpp
index d708e8a..dc87b5f 100644
--- a/fpdfsdk/fpdf_view_embeddertest.cpp
+++ b/fpdfsdk/fpdf_view_embeddertest.cpp
@@ -2089,36 +2089,34 @@
FPDF_DestroyLibrary();
std::string agg_checksum;
+ const FPDF_LIBRARY_CONFIG kAggConfig = {
+ .version = 4,
+ .m_pUserFontPaths = nullptr,
+ .m_pIsolate = nullptr,
+ .m_v8EmbedderSlot = 0,
+ .m_pPlatform = nullptr,
+ .m_RendererType = FPDF_RENDERERTYPE_AGG,
+ };
+ FPDF_InitLibraryWithConfig(&kAggConfig);
{
- FPDF_LIBRARY_CONFIG config = {
- .version = 4,
- .m_pUserFontPaths = nullptr,
- .m_pIsolate = nullptr,
- .m_v8EmbedderSlot = 0,
- .m_pPlatform = nullptr,
- .m_RendererType = FPDF_RENDERERTYPE_AGG,
- };
- FPDF_InitLibraryWithConfig(&config);
-
ASSERT_TRUE(OpenDocument("rectangles.pdf"));
FPDF_PAGE page = LoadPage(0);
ScopedFPDFBitmap bitmap = RenderPage(page);
agg_checksum = HashBitmap(bitmap.get());
UnloadPage(page);
CloseDocument();
- FPDF_DestroyLibrary();
}
+ FPDF_DestroyLibrary();
std::string skia_checksum;
+ const FPDF_LIBRARY_CONFIG kSkiaConfig = {
+ .version = 2,
+ .m_pUserFontPaths = nullptr,
+ .m_pIsolate = nullptr,
+ .m_v8EmbedderSlot = 0,
+ };
+ FPDF_InitLibraryWithConfig(&kSkiaConfig);
{
- FPDF_LIBRARY_CONFIG config = {
- .version = 2,
- .m_pUserFontPaths = nullptr,
- .m_pIsolate = nullptr,
- .m_v8EmbedderSlot = 0,
- };
- FPDF_InitLibraryWithConfig(&config);
-
ASSERT_TRUE(OpenDocument("rectangles.pdf"));
FPDF_PAGE page = LoadPage(0);
ScopedFPDFBitmap bitmap = RenderPage(page);
diff --git a/public/fpdfview.h b/public/fpdfview.h
index 580c524..e1b3a32 100644
--- a/public/fpdfview.h
+++ b/public/fpdfview.h
@@ -1452,6 +1452,9 @@
// Use is optional, but allows external creation of isolates
// matching the ones PDFium will make when none is provided
// via |FPDF_LIBRARY_CONFIG::m_pIsolate|.
+//
+// Can only be called when the library is in an uninitialized or
+// destroyed state.
FPDF_EXPORT void* FPDF_CALLCONV FPDF_GetArrayBufferAllocatorSharedInstance();
#endif // PDF_ENABLE_V8
diff --git a/third_party/BUILD.gn b/third_party/BUILD.gn
index f3f56d7..32fc813 100644
--- a/third_party/BUILD.gn
+++ b/third_party/BUILD.gn
@@ -556,7 +556,6 @@
"base/check_op.h",
"base/component_export.h",
"base/immediate_crash.h",
- "base/no_destructor.h",
"base/notreached.h",
"base/template_util.h",
]
diff --git a/third_party/base/no_destructor.h b/third_party/base/no_destructor.h
deleted file mode 100644
index 5d125a9..0000000
--- a/third_party/base/no_destructor.h
+++ /dev/null
@@ -1,134 +0,0 @@
-// Copyright 2018 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef THIRD_PARTY_BASE_NO_DESTRUCTOR_H_
-#define THIRD_PARTY_BASE_NO_DESTRUCTOR_H_
-
-#include <new>
-#include <type_traits>
-#include <utility>
-
-namespace pdfium {
-namespace base {
-
-// Helper type to create a function-local static variable of type `T` when `T`
-// has a non-trivial destructor. Storing a `T` in a `base::NoDestructor<T>` will
-// prevent `~T()` from running, even when the variable goes out of scope.
-//
-// Useful when a variable has static storage duration but its type has a
-// non-trivial destructor. Chromium bans global constructors and destructors:
-// using a function-local static variable prevents the former, while using
-// `base::NoDestructor<T>` prevents the latter.
-//
-// ## Caveats
-//
-// - Must only be used as a function-local static variable. Declaring a global
-// variable of type `base::NoDestructor<T>` will still generate a global
-// constructor; declaring a local or member variable will lead to memory leaks
-// or other surprising and undesirable behaviour.
-//
-// - If the data is rarely used, consider creating it on demand rather than
-// caching it for the lifetime of the program. Though `base::NoDestructor<T>`
-// does not heap allocate, the compiler still reserves space in bss for
-// storing `T`, which costs memory at runtime.
-//
-// - If `T` is trivially destructible, do not use `base::NoDestructor<T>`:
-//
-// const uint64_t GetUnstableSessionSeed() {
-// // No need to use `base::NoDestructor<T>` as `uint64_t` is trivially
-// // destructible and does not require a global destructor.
-// static const uint64_t kSessionSeed = base::RandUint64();
-// return kSessionSeed;
-// }
-//
-// ## Example Usage
-//
-// const std::string& GetDefaultText() {
-// // Required since `static const std::string` requires a global destructor.
-// static const base::NoDestructor<std::string> s("Hello world!");
-// return *s;
-// }
-//
-// More complex initialization using a lambda:
-//
-// const std::string& GetRandomNonce() {
-// // `nonce` is initialized with random data the first time this function is
-// // called, but its value is fixed thereafter.
-// static const base::NoDestructor<std::string> nonce([] {
-// std::string s(16);
-// crypto::RandString(s.data(), s.size());
-// return s;
-// }());
-// return *nonce;
-// }
-//
-// ## Thread safety
-//
-// Initialisation of function-local static variables is thread-safe since C++11.
-// The standard guarantees that:
-//
-// - function-local static variables will be initialised the first time
-// execution passes through the declaration.
-//
-// - if another thread's execution concurrently passes through the declaration
-// in the middle of initialisation, that thread will wait for the in-progress
-// initialisation to complete.
-template <typename T>
-class NoDestructor {
- public:
- static_assert(
- !std::is_trivially_destructible_v<T>,
- "T is trivially destructible; please use a function-local static "
- "of type T directly instead");
-
- // Not constexpr; just write static constexpr T x = ...; if the value should
- // be a constexpr.
- template <typename... Args>
- explicit NoDestructor(Args&&... args) {
- new (storage_) T(std::forward<Args>(args)...);
- }
-
- // Allows copy and move construction of the contained type, to allow
- // construction from an initializer list, e.g. for std::vector.
- explicit NoDestructor(const T& x) { new (storage_) T(x); }
- explicit NoDestructor(T&& x) { new (storage_) T(std::move(x)); }
-
- NoDestructor(const NoDestructor&) = delete;
- NoDestructor& operator=(const NoDestructor&) = delete;
-
- ~NoDestructor() = default;
-
- const T& operator*() const { return *get(); }
- T& operator*() { return *get(); }
-
- const T* operator->() const { return get(); }
- T* operator->() { return get(); }
-
- const T* get() const { return reinterpret_cast<const T*>(storage_); }
- T* get() { return reinterpret_cast<T*>(storage_); }
-
- private:
- alignas(T) char storage_[sizeof(T)];
-
-#if defined(LEAK_SANITIZER)
- // TODO(https://crbug.com/812277): This is a hack to work around the fact
- // that LSan doesn't seem to treat NoDestructor as a root for reachability
- // analysis. This means that code like this:
- // static base::NoDestructor<std::vector<int>> v({1, 2, 3});
- // is considered a leak. Using the standard leak sanitizer annotations to
- // suppress leaks doesn't work: std::vector is implicitly constructed before
- // calling the base::NoDestructor constructor.
- //
- // Unfortunately, I haven't been able to demonstrate this issue in simpler
- // reproductions: until that's resolved, hold an explicit pointer to the
- // placement-new'd object in leak sanitizer mode to help LSan realize that
- // objects allocated by the contained type are still reachable.
- T* storage_ptr_ = reinterpret_cast<T*>(storage_);
-#endif // defined(LEAK_SANITIZER)
-};
-
-} // namespace base
-} // namespace pdfium
-
-#endif // THIRD_PARTY_BASE_NO_DESTRUCTOR_H_
