Follow up #220/#230: drop unused timeout id and emit U+FFFD for malformed UTF-16

Polyfills/Scheduling/Source/TimeoutDispatcher.cpp

@@ -2,6 +2,7 @@
 
 #include <cassert>
 #include <optional>
+#include <stdexcept>
 
 namespace Babylon::Polyfills::Internal
 {
@@ -63,11 +64,6 @@ namespace Babylon::Polyfills::Internal
     }
 
     TimeoutDispatcher::TimeoutId TimeoutDispatcher::Dispatch(std::shared_ptr<Napi::FunctionReference> function, std::chrono::milliseconds delay, bool repeat)
-    {
-        return DispatchImpl(function, delay, repeat, 0);
-    }
-
-    TimeoutDispatcher::TimeoutId TimeoutDispatcher::DispatchImpl(std::shared_ptr<Napi::FunctionReference> function, std::chrono::milliseconds delay, bool repeat, TimeoutId id)
     {
         if (delay.count() < 0)
         {
@@ -76,14 +72,23 @@ namespace Babylon::Polyfills::Internal
 
         std::unique_lock<std::recursive_mutex> lk{m_mutex};
 
-        if (id == 0)
-        {
-            id = NextTimeoutId();
-        }
+        // Always a fresh id: re-arming a repeating timeout no longer goes through
+        // Dispatch, so there is no caller that reuses an existing id. Passing one
+        // into unordered_map::insert would keep the old Timeout and then add a
+        // second m_timeMap entry pointing at it; Clear() erases only one.
+        const auto id = NextTimeoutId();
         const auto earliestTime = m_timeMap.empty() ? TimePoint::max() : m_timeMap.cbegin()->second->time;
         const auto time = Now() + delay;
-        const auto result = m_idMap.insert({id, std::make_unique<Timeout>(id, ++m_lastSequence, std::move(function), time, repeat ? std::make_optional<std::chrono::milliseconds>(delay) : std::nullopt)});
-        m_timeMap.insert({time, result.first->second.get()});
+        auto timeout = std::make_unique<Timeout>(id, ++m_lastSequence, std::move(function), time, repeat ? std::make_optional<std::chrono::milliseconds>(delay) : std::nullopt);
+        // try_emplace does not consume timeout if the id is already present, unlike
+        // emplace, so a failed insert cannot destroy the Timeout and leave a
+        // dangling m_timeMap pointer. NextTimeoutId should already skip live ids.
+        const auto [it, inserted] = m_idMap.try_emplace(id, std::move(timeout));
+        if (!inserted)
+        {
+            throw std::logic_error{"TimeoutDispatcher: NextTimeoutId returned a duplicate id"};
+        }
+        m_timeMap.insert({time, it->second.get()});
 
         if (time <= earliestTime)
         {

Polyfills/Scheduling/Source/TimeoutDispatcher.h

@@ -28,8 +28,6 @@ namespace Babylon::Polyfills::Internal
     private:
         using TimePoint = std::chrono::time_point<std::chrono::steady_clock, std::chrono::microseconds>;
 
-        TimeoutId DispatchImpl(std::shared_ptr<Napi::FunctionReference> function, std::chrono::milliseconds delay, bool repeat, TimeoutId id);
-
         TimeoutId NextTimeoutId();
         void ThreadFunction();
         void CallFunction(TimeoutId id, uint64_t sequence);

Polyfills/TextDecoder/README.md

@@ -11,14 +11,15 @@ A C++ implementation of the [WHATWG Encoding API](https://encoding.spec.whatwg.o
 - Constructing `TextDecoder` with no argument (defaults to `utf-8`).
 - Constructing `TextDecoder` with any WHATWG label for UTF-8 (`"utf-8"`, `"utf8"`, `"unicode-1-1-utf-8"`, `"unicode11utf8"`, `"unicode20utf8"`, `"x-unicode20utf8"`), UTF-16LE (`"utf-16"`, `"utf-16le"`, `"ucs-2"`, `"unicode"`, `"unicodeFEFF"`, `"csunicode"`, `"iso-10646-ucs-2"`) or UTF-16BE (`"utf-16be"`, `"unicodeFFFE"`). Labels are matched case-insensitively and ignore surrounding whitespace.
 - Stripping a leading byte order mark when decoding UTF-16.
+- Replacing malformed UTF-16 (a trailing odd byte, or an unpaired surrogate) with U+FFFD, for both endiannesses.
 - Calling `decode()` with no argument or `undefined` returns an empty string (matches the Web API).
 
 ### Not Supported
 
 - Encodings other than UTF-8 and UTF-16 — passing any other label (e.g. `"iso-8859-1"`) throws a JavaScript `Error`.
 - `DataView` is not accepted by `decode()` — due to missing `Napi::DataView` support in the underlying JSI layer.
 - Passing a non-BufferSource value (e.g. a string or number) to `decode()` throws a `TypeError`.
-- The `fatal` option: decoding errors are not detected and do not throw a `TypeError`. A trailing odd byte in a UTF-16 sequence is dropped rather than decoded as U+FFFD.
+- The `fatal` option: decoding errors do not throw a `TypeError`. Malformed UTF-16 (a trailing odd byte, or an unpaired surrogate) is replaced with U+FFFD instead.
 - The `ignoreBOM` option: a leading UTF-16 byte order mark is always stripped and cannot be retained. A UTF-8 byte order mark is never stripped.
 - Streaming decode (passing `{ stream: true }` to `decode()`) — each call is stateless.
 - The `encoding` property on the `TextDecoder` instance is not exposed.

Polyfills/TextDecoder/Source/TextDecoder.cpp

@@ -113,17 +113,64 @@ namespace
 
         Napi::Value DecodeUtf16(Napi::Env env, const std::string& data) const
         {
-            // Trailing odd byte is dropped: the WHATWG decoder would emit U+FFFD for it, but
-            // every producer we care about hands over whole code units.
-            const size_t unitCount = data.size() / 2;
-            std::u16string units(unitCount, u'\0');
-            for (size_t index = 0; index < unitCount; ++index)
+            // WHATWG UTF-16 decoder in replacement mode: unpaired surrogates and a
+            // leftover odd byte become U+FFFD. A lead surrogate followed by a
+            // non-trail is one replacement, then the second unit is reprocessed.
+            const bool littleEndian = m_encoding == Encoding::Utf16LittleEndian;
+            std::u16string units;
+            units.reserve(data.size() / 2 + 1);
+
+            constexpr char16_t LEAD_MIN = 0xD800;
+            constexpr char16_t LEAD_MAX = 0xDBFF;
+            constexpr char16_t TRAIL_MIN = 0xDC00;
+            constexpr char16_t TRAIL_MAX = 0xDFFF;
+            constexpr char16_t REPLACEMENT = 0xFFFD;
+
+            bool pendingLead = false;
+            char16_t lead = 0;
+            size_t byteIndex = 0;
+            while (byteIndex + 1 < data.size())
             {
-                const auto first = static_cast<unsigned char>(data[index * 2]);
-                const auto second = static_cast<unsigned char>(data[index * 2 + 1]);
-                units[index] = m_encoding == Encoding::Utf16LittleEndian
+                const auto first = static_cast<unsigned char>(data[byteIndex]);
+                const auto second = static_cast<unsigned char>(data[byteIndex + 1]);
+                byteIndex += 2;
+                const auto unit = littleEndian
                     ? static_cast<char16_t>(first | (second << 8))
                     : static_cast<char16_t>(second | (first << 8));
+
+                if (pendingLead)
+                {
+                    pendingLead = false;
+                    if (unit >= TRAIL_MIN && unit <= TRAIL_MAX)
+                    {
+                        units.push_back(lead);
+                        units.push_back(unit);
+                        continue;
+                    }
+
+                    units.push_back(REPLACEMENT);
+                    // Fall through and reprocess `unit` as a standalone code unit.
+                }
+
+                if (unit >= LEAD_MIN && unit <= LEAD_MAX)
+                {
+                    pendingLead = true;
+                    lead = unit;
+                }
+                else if (unit >= TRAIL_MIN && unit <= TRAIL_MAX)
+                {
+                    units.push_back(REPLACEMENT);
+                }
+                else
+                {
+                    units.push_back(unit);
+                }
+            }
+
+            // End-of-queue: an unpaired lead and/or leftover odd byte is one error.
+            if (pendingLead || byteIndex < data.size())
+            {
+                units.push_back(REPLACEMENT);
             }
 
             if (!units.empty() && units.front() == u'\uFEFF')

Tests/UnitTests/Scripts/tests.ts

@@ -1851,6 +1851,28 @@ describe("TextDecoder", function () {
         expect(result).to.equal("\u{1F600}\0A");
         expect(result.length).to.equal(4);
     });
+
+    it("should replace a trailing odd UTF-16 byte with U+FFFD", function () {
+        expect(new TextDecoder("utf-16le").decode(new Uint8Array([0x48, 0x00, 0x00]))).to.equal("H\uFFFD");
+        expect(new TextDecoder("utf-16be").decode(new Uint8Array([0x00, 0x48, 0x00]))).to.equal("H\uFFFD");
+        expect(new TextDecoder("utf-16le").decode(new Uint8Array([0x00]))).to.equal("\uFFFD");
+        expect(new TextDecoder("utf-16be").decode(new Uint8Array([0x00]))).to.equal("\uFFFD");
+    });
+
+    it("should replace unpaired UTF-16 surrogates with U+FFFD", function () {
+        // Lone lead U+D800.
+        expect(new TextDecoder("utf-16le").decode(new Uint8Array([0x00, 0xD8]))).to.equal("\uFFFD");
+        expect(new TextDecoder("utf-16be").decode(new Uint8Array([0xD8, 0x00]))).to.equal("\uFFFD");
+        // Lone trail U+DC00.
+        expect(new TextDecoder("utf-16le").decode(new Uint8Array([0x00, 0xDC]))).to.equal("\uFFFD");
+        expect(new TextDecoder("utf-16be").decode(new Uint8Array([0xDC, 0x00]))).to.equal("\uFFFD");
+        // Lead followed by BMP 'A': replacement, then reprocess 'A'.
+        expect(new TextDecoder("utf-16le").decode(new Uint8Array([0x00, 0xD8, 0x41, 0x00]))).to.equal("\uFFFDA");
+        expect(new TextDecoder("utf-16be").decode(new Uint8Array([0xD8, 0x00, 0x00, 0x41]))).to.equal("\uFFFDA");
+        // Unpaired lead plus leftover odd byte is a single end-of-queue replacement.
+        expect(new TextDecoder("utf-16le").decode(new Uint8Array([0x00, 0xD8, 0x00]))).to.equal("\uFFFD");
+        expect(new TextDecoder("utf-16be").decode(new Uint8Array([0xD8, 0x00, 0x00]))).to.equal("\uFFFD");
+    });
 });
 
 describe("TextEncoder", function () {