fix: Actually fix JavaScriptTimerAction invocation crash

Previous fix was a red herring. Whilst technically there was an
issue there, it turned out not the be the cause of the crash that
I've repeatedly ran into.

The real issue was that SArray isn't really a safe data structure,
by design. Smaller revision ranges exacerbate the data structure's
limitations. For some use cases a small revision range will be
fine, but timers are likely a prime example of an API where a small
revision range is NOT fine.

This is because the way timers are used in JavaScript is often as
timeouts, which is why the API is called setTimeout(). Often the
user schedules a timeout and it's never expected to fire. Instead
clearTimeout will be called when some other operation returns
before the timeout is reached. This means timers are frequently
being inserted and deleted. SArray by design will reuse indexes,
when this occurs the revision is incremented. However, the Index32
data structure only permits 64 revisions. That number is likely to
(and in my case definitely does) wrap around frequently.

That on its lonesome isn't an issue. What's problematic is that
the timer manager uses an efficient wheel data structure built
atop SArray. Scheduled timers are added to one of many wheels and
into the SArray index of all scheduled timers. When a timer is
cleared, the wheels are not touched at all, only the used timer
SArray is modified. The idea being that later on when it's time
to execute the relevant timers stored in (part of) a wheel, we
just check to see if index in the wheel resolves to a timer in
our used/scheduled timer SArray. We a small revision range, we
frequently end up reusing both and index AND matching revision.

This leads to both early firing of timers AND firing of timers
that have since been destructed i.e. cause the crash.

This commit swap from using 32-bit index+revisions to 52-bit
index+revisions. 20 bits of which are reserved for the revision.
So now we can have over 1 million revisions before wrapping
around.

It's now substantially unlikely for index re-use to happen.
HOWEVER, in theory, it COULD still occur. Suppose that in this
extremely unlikely circumstance our timer were to fire too
early. That's not ideal, but is probably a risk we can live with.
What we can't accept is the risk of an outright crash. So
we've also implemented protection in JavaScriptTimerAction
itself. Basically its destructor now effectively marks itself as
being destroyed. The execution logic detects this and bails out,
evading the crash.

Author: Benjamin-Dobell

bridge/jsb_essentials.cpp

@@ -167,7 +167,8 @@ namespace jsb
             Environment::wrap(isolate)->get_timer_manager().set_timer(handle,
                 JavaScriptTimerAction(v8::Global<v8::Function>(isolate, func), 0), rate > 0 ? rate : 0, loop);
         }
-        info.GetReturnValue().Set((int32_t) handle);
+        // TODO: V8 update. Once we update V8 past 12.6.221 we can skip the cast to double
+        info.GetReturnValue().Set((double) (int64_t) handle);
     }
 
     void _clear_timer(const v8::FunctionCallbackInfo<v8::Value>& info)

bridge/jsb_timer_action.cpp

@@ -6,7 +6,11 @@ namespace jsb
 {
     void JavaScriptTimerAction::operator()(v8::Isolate* isolate)
     {
-        jsb_checkf(function_, "Attempt made to call a moved/unassigned JavaScriptTimerAction");
+        if (!function_)
+        {
+            JSB_LOG(Warning, "Ignored attempt to execute a unassigned/moved/destroyed JavaScriptTimerAction");
+            return;
+        }
 
         const v8::Local<v8::Function> func = function_->Get(isolate);
         const v8::Local<v8::Context> context = func->GetCreationContextChecked();

bridge/jsb_timer_action.h

@@ -5,6 +5,9 @@
 
 namespace jsb
 {
+    /**
+     * This struct is *not* POD, but aims to be compatible with SArray's memory relocation logic.
+     */
     struct JavaScriptTimerAction
     {
         jsb_force_inline JavaScriptTimerAction(): function_(nullptr), argc_(0), argv_(nullptr)
@@ -29,6 +32,8 @@ namespace jsb
         {
             delete function_;
             delete[] argv_;
+            function_ = nullptr;
+            argv_ = nullptr;
         }
 
         JavaScriptTimerAction(JavaScriptTimerAction& p_other) = delete;

internal/jsb_format.h

@@ -6,10 +6,11 @@
 
 namespace jsb::internal
 {
-    template<typename T> struct TFormat            { static Variant from(const T& p_item) { return p_item; } };
-    template<>           struct TFormat<Index64>   { static Variant from(const Index64& p_item) { return *p_item; } };
-    template<>           struct TFormat<Index32>   { static Variant from(const Index32& p_item) { return *p_item; } };
-    template<>           struct TFormat<uintptr_t> { static Variant from(const uintptr_t& p_item) { return Variant((uint64_t) p_item); } };
+    template<typename T> struct TFormat                { static Variant from(const T& p_item) { return p_item; } };
+    template<>           struct TFormat<IndexSafe64>   { static Variant from(const IndexSafe64& p_item) { return *p_item; } };
+    template<>           struct TFormat<Index64>       { static Variant from(const Index64& p_item) { return *p_item; } };
+    template<>           struct TFormat<Index32>       { static Variant from(const Index32& p_item) { return *p_item; } };
+    template<>           struct TFormat<uintptr_t>     { static Variant from(const uintptr_t& p_item) { return Variant((uint64_t) p_item); } };
     template<typename T> static Variant convert(const T& p_item) { return TFormat<T>::from(p_item); }
 
     template <typename... VarArgs>

internal/jsb_sindex.h

@@ -75,8 +75,12 @@ namespace jsb::internal
         UnderlyingType packed_;
     };
 
+    // Allocates 64 bits, but only the lower 52 are used. This is the range that can be safely stored in a JS number.
+    // index(0, 4_294_967_295) revision(1, 4_294_967_295)
+    typedef TIndex<uint64_t, 20, 0xfffff> IndexSafe64;
+
     // do not really support the index bigger than int32_t.MaxValue
-    // /*if unsigned*/ index(0, 4_294_967_295) revision(1, 4_294_967_295)
+    // /*if unsigned*/ index(0, 4_294_967_295) revision(1, 1_048_575)
     typedef TIndex<uint64_t, 32, 0xffffffff> Index64;
 
     // index(0, 67_108_863) revision(1, 63)

internal/jsb_timer_manager.h

@@ -10,23 +10,23 @@ namespace jsb::internal
         jsb_force_inline TimerHandle() = default;
         jsb_force_inline ~TimerHandle() = default;
 
-        jsb_force_inline TimerHandle(const Index32& p_index): id(p_index) {}
-        jsb_force_inline TimerHandle& operator=(const Index32& p_index) { id = p_index; return *this; }
+        jsb_force_inline TimerHandle(const IndexSafe64& p_index): id(p_index) {}
+        jsb_force_inline TimerHandle& operator=(const IndexSafe64& p_index) { id = p_index; return *this; }
 
         jsb_force_inline TimerHandle(const TimerHandle& p_other): id(p_other.id) {}
         jsb_force_inline TimerHandle& operator=(const TimerHandle& p_other) { id = p_other.id; return *this; }
 
         jsb_force_inline TimerHandle(TimerHandle&& p_other) noexcept: id(p_other.id) {}
         jsb_force_inline TimerHandle& operator=(TimerHandle&& p_other) noexcept { id = p_other.id; p_other.id = {}; return *this; }
 
-        jsb_force_inline explicit operator int32_t() const { return (int32_t) *id; }
-        jsb_force_inline explicit operator Index32() const { return id; }
-        jsb_force_inline explicit operator bool() const { return id != Index32::none(); }
+        jsb_force_inline explicit operator int64_t() const { return (int64_t) *id; }
+        jsb_force_inline explicit operator IndexSafe64() const { return id; }
+        jsb_force_inline explicit operator bool() const { return id != IndexSafe64::none(); }
 
-        jsb_force_inline explicit TimerHandle(const int32_t p_index): id((uint32_t) p_index) {}
+        jsb_force_inline explicit TimerHandle(const int64_t p_index): id((int64_t) p_index) {}
 
     private:
-        Index32 id;
+        IndexSafe64 id;
 
         template<typename, uint8_t, uint8_t, uint64_t>
         friend class TTimerManager;
@@ -59,11 +59,11 @@ namespace jsb::internal
 
         struct WheelSlot
         {
-            std::vector<Index32> timer_indices;
+            std::vector<IndexSafe64> timer_indices;
 
-            jsb_force_inline void append(const Index32& index) { timer_indices.push_back(index); }
+            jsb_force_inline void append(const IndexSafe64& index) { timer_indices.push_back(index); }
 
-            jsb_force_inline void move_to(std::vector<Index32>& cache)
+            jsb_force_inline void move_to(std::vector<IndexSafe64>& cache)
             {
                 cache.reserve(cache.size() + timer_indices.size());
                 cache.insert(cache.end(), timer_indices.begin(), timer_indices.end());
@@ -97,15 +97,15 @@ namespace jsb::internal
                 index = 0;
             }
 
-            uint32_t add(uint64_t p_delay, const Index32& p_timer_index)
+            uint64_t add(uint64_t p_delay, const IndexSafe64& p_timer_index)
             {
                 const uint64_t offset = p_delay >= interval ? (p_delay / interval) - 1 : p_delay / interval;
-                const uint32_t slot_index = (uint32_t)((index + offset) % (uint64_t)kWheelSlotNum);
+                const uint64_t slot_index = (uint64_t)((index + offset) % (uint64_t)kWheelSlotNum);
                 slots[slot_index].append(p_timer_index);
                 return slot_index;
             }
 
-            void next(std::vector<Index32>& p_active_indices)
+            void next(std::vector<IndexSafe64>& p_active_indices)
             {
                 slots[index++].move_to(p_active_indices);
             }
@@ -132,10 +132,10 @@ namespace jsb::internal
 
         uint64_t _elapsed;
         uint32_t _time_slice;
-        SArray<TimerData, Index32> _used_timers;
+        SArray<TimerData, IndexSafe64> _used_timers;
         WheelState _wheels[kWheelNum];
-        std::vector<Index32> _activated_timers;
-        std::vector<Index32> _moving_timers;
+        std::vector<IndexSafe64> _activated_timers;
+        std::vector<IndexSafe64> _moving_timers;
 
         static void check_internal_state()
         {
@@ -184,7 +184,7 @@ namespace jsb::internal
             _used_timers.remove_at(inout_handle.id);
 
             const uint64_t delay = p_first_delay > 0 ? p_first_delay : p_rate;
-            const Index32 index = _used_timers.add(TimerData());
+            const IndexSafe64 index = _used_timers.add(TimerData());
             TimerData& timer = _used_timers.get_value(index);
             timer.rate = p_rate;
             timer.expires = delay + _elapsed;
@@ -206,7 +206,7 @@ namespace jsb::internal
         {
             if (_clear_timer(p_handle.id))
             {
-                p_handle.id = Index32::none();
+                p_handle.id = IndexSafe64::none();
                 return true;
             }
             return false;
@@ -258,7 +258,7 @@ namespace jsb::internal
                     }
 
                     _wheels[wheel_index + 1].next(_moving_timers);
-                    for (const Index32& index : _moving_timers)
+                    for (const IndexSafe64& index : _moving_timers)
                     {
                         TimerData* timer;
                         if (_used_timers.try_get_value_pointer(index, timer))
@@ -284,7 +284,7 @@ namespace jsb::internal
         bool invoke_timers(TContext* ctx)
         {
             if (_activated_timers.empty()) return false;
-            for (const Index32& index : _activated_timers)
+            for (const IndexSafe64& index : _activated_timers)
             {
                 TimerData* timer;
                 if (!_used_timers.try_get_value_pointer(index, timer))
@@ -314,13 +314,13 @@ namespace jsb::internal
         }
 
     private:
-        bool _clear_timer(const Index32& p_index)
+        bool _clear_timer(const IndexSafe64& p_index)
         {
             check_internal_state();
             return _used_timers.remove_at(p_index);
         }
 
-        void rearrange_timer(const Index32& p_timer_id, uint64_t p_delay)
+        void rearrange_timer(const IndexSafe64& p_timer_id, uint64_t p_delay)
         {
             jsb_check(_used_timers.is_valid_index(p_timer_id));
             for (WheelState& wheel : _wheels)