From 8790cfc6106d9505803374dc898afae7583fb560 Mon Sep 17 00:00:00 2001
From: Joyee Cheung <joyeec9h3@gmail.com>
Date: Wed, 12 Jul 2023 17:18:43 +0200
Subject: [PATCH] src: make BaseObject iteration order deterministic
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Previously we just rely on the unordered_set order to iterate over
the BaseObjects, which is not deterministic.

The iteration is only used in printing, verification, and snapshot
generation. In the first two cases the performance overhead of
sorting does not matter because they are only used for debugging.
In the last case the determinism is more important than the trivial
overhead of sorting. So this patch makes the iteration deterministic
by sorting the set first, as what is already being done when we
drain the queue.

PR-URL: https://github.com/nodejs/node/pull/48702
Refs: https://github.com/nodejs/build/issues/3043
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
---
 src/cleanup_queue-inl.h | 4 +++-
 src/cleanup_queue.cc    | 9 ++++++++-
 src/cleanup_queue.h     | 2 ++
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/src/cleanup_queue-inl.h b/src/cleanup_queue-inl.h
index 5d9a56e6b07956..d1fbd8241d9919 100644
--- a/src/cleanup_queue-inl.h
+++ b/src/cleanup_queue-inl.h
@@ -39,7 +39,9 @@ void CleanupQueue::Remove(Callback cb, void* arg) {
 
 template <typename T>
 void CleanupQueue::ForEachBaseObject(T&& iterator) const {
-  for (const auto& hook : cleanup_hooks_) {
+  std::vector<CleanupHookCallback> callbacks = GetOrdered();
+
+  for (const auto& hook : callbacks) {
     BaseObject* obj = GetBaseObject(hook);
     if (obj != nullptr) iterator(obj);
   }
diff --git a/src/cleanup_queue.cc b/src/cleanup_queue.cc
index 6290b6796c5327..5923db6b89d4b4 100644
--- a/src/cleanup_queue.cc
+++ b/src/cleanup_queue.cc
@@ -5,7 +5,8 @@
 
 namespace node {
 
-void CleanupQueue::Drain() {
+std::vector<CleanupQueue::CleanupHookCallback> CleanupQueue::GetOrdered()
+    const {
   // Copy into a vector, since we can't sort an unordered_set in-place.
   std::vector<CleanupHookCallback> callbacks(cleanup_hooks_.begin(),
                                              cleanup_hooks_.end());
@@ -20,6 +21,12 @@ void CleanupQueue::Drain() {
               return a.insertion_order_counter_ > b.insertion_order_counter_;
             });
 
+  return callbacks;
+}
+
+void CleanupQueue::Drain() {
+  std::vector<CleanupHookCallback> callbacks = GetOrdered();
+
   for (const CleanupHookCallback& cb : callbacks) {
     if (cleanup_hooks_.count(cb) == 0) {
       // This hook was removed from the `cleanup_hooks_` set during another
diff --git a/src/cleanup_queue.h b/src/cleanup_queue.h
index 64e04e1856a197..2ca333aca855ff 100644
--- a/src/cleanup_queue.h
+++ b/src/cleanup_queue.h
@@ -6,6 +6,7 @@
 #include <cstddef>
 #include <cstdint>
 #include <unordered_set>
+#include <vector>
 
 #include "memory_tracker.h"
 
@@ -66,6 +67,7 @@ class CleanupQueue : public MemoryRetainer {
     uint64_t insertion_order_counter_;
   };
 
+  std::vector<CleanupHookCallback> GetOrdered() const;
   inline BaseObject* GetBaseObject(const CleanupHookCallback& callback) const;
 
   // Use an unordered_set, so that we have efficient insertion and removal.