2021274 - Differential behavior on Object.keys with ion vs. baseline

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Christian Holler (:decoder)]

Attachment: test_objkeys_bug.js

The attached testcase shows differential behavior on mozilla-central 20260303-887fb4b92f0c when run with --fuzzing-safe --no-threads --differential-testing vs. --fuzzing-safe --no-threads --differential-testing --no-ion.

This is likely a debug-only bug but we should fix this as it breaks the --differential-testing property.

  Root Cause Analysis: Object.keys Scalar Replacement Bug                                                                                                                                                                                                                                                                
                                                                                                                                                                                                                                                                                                                         
  The Differential                                                                                                                                                                                                                                                                                                       
                                                                                                                                                                                                                                                                                                                         
  The test produces different results depending on whether Ion JIT is enabled:                                                                                                                                                                                                           
                                                                                                                                                                                                                                                                                                                         
  - Default (Ion enabled): Object.keys(obj) returns ["m", "z"] (alphabetical order) — WRONG                                                                                                                                                                                                                              
  - --no-ion (baseline only): Object.keys(obj) returns ["z", "m"] (insertion order) — CORRECT                                                                                                                                                                                                                            
                                                                                                                                                                                                                                                                                                                         
  Root Cause: Two inconsistent code paths                                                                                                                                                                                                                                                                                
                                                                                                                                                                                                                                                                                                                         
  The bug is caused by two different Object.keys code paths that produce different property orders in debug builds with --differential-testing:                                                                                                                                                                          
                                                                                                                                                                                                                                                                                                                         
  Path 1 — Baseline / non-scalar-replaced Ion (jit::ObjectKeys() → obj_keys() → TryEnumerableOwnPropertiesNative<Keys>):                                                                                                                                                                                                 
  1. Tries LookupInShapeIteratorCache — initially no cached iterator exists                                                                                                                                                                                                                                              
  2. Falls to slow path at Object.cpp:1698: iterates via ShapePropertyIter (reverse insertion order): m, z, b, a                                                                                                                                                                                                         
  3. Filters non-enumerable: m, z                                                                                                                                                                                                                                                                                        
  4. Calls std::reverse() at Object.cpp:1728 → ["z", "m"] (correct insertion order)                                                                                                                                                                                                                                      
  5. This path does NOT go through PropertyEnumerator::snapshot() and therefore does NOT apply the debug sort                                                                                                                                                                                                            
                                                                                                                                                                                                                                                                                                                         
  Path 2 — Ion with scalar replacement (MObjectToIterator → GetIteratorForObjectKeys):                                                                                                                                                                                                                                   
  1. ObjectKeysReplacer in ScalarReplacement.cpp:4009 replaces MObjectKeys with MObjectToIterator + MLoadIteratorElement                                                                                                                                                                                                 
  2. At runtime, MObjectToIterator tries inline cache from shape → misses → OOL call to GetIteratorForObjectKeys
  3. GetIteratorForObjectKeys (Iteration.cpp:1325) → GetIteratorImpl → PropertyEnumerator::snapshot()
  4. enumerateNativeProperties iterates shape in reverse, then reversePropsAndIndicesAfter → ["z", "m"] (correct order in the keys vector)
  5. BUT THEN: PropertyEnumerator::snapshot() at Iteration.cpp:693-722 has a #ifdef DEBUG block that sorts property IDs alphabetically when SupportDifferentialTesting() is true
  6. After the sort: ["m", "z"] (alphabetical order — WRONG for Object.keys)
  7. Creates NativeIterator with properties in wrong order, caches it in shape cache
  8. MLoadIteratorElement reads from this sorted iterator → returns properties in alphabetical order
                                                                              
  The problematic code     
                                                                              
  Iteration.cpp:693-722:
  #ifdef DEBUG                  
    if (js::SupportDifferentialTesting() && !supportsIndices()) {
      // This sort was intended to normalize for-in enumeration order
      // to prevent false positives in differential testing.
      // But it's inadvertently applied to Object.keys iterators too.
      jsid* ids = props_.begin();                   
      size_t n = props_.length();                                                                                                                           
      RootedIdVector tmp(cx);
      if (!tmp.resize(n)) return false;
      PodCopy(tmp.begin(), ids, n);
      if (!MergeSort(ids, n, tmp.begin(), SortComparatorIds(cx)))
        return false;
    }
  #endif  
                                                                              
  This sort was designed to normalize for...in enumeration order across interpreter/JIT to avoid false positives in fuzzers. But when Ion's scalar replacement of Object.keys reuses PropertyEnumerator::snapshot() to create a NativeIterator, the sort gets applied to Object.keys property order — creating an
  inconsistency with the TryEnumerableOwnPropertiesNative code path that doesn't sort.
                                                                              
  Why it only triggers with specific property patterns
                                                                                                                                                            
  The bug is only observable when:
  1. Non-enumerable properties exist (created via Object.defineProperty with enumerable: false) — these create a mixed shape where the ShapePropertyIter order matters
  2. Enumerable properties whose insertion order differs from alphabetical order — if insertion order matches alphabetical order, both paths produce the same result and the bug is masked
  3. Enough iterations (4000+) to trigger Ion compilation with scalar replacement

  Confirmation

  - --ion-scalar-replacement=off → PASS (MObjectKeys stays, calls jit::ObjectKeys() → TryEnumerableOwnPropertiesNative → no sort)
  - Making the keys array escape (prevents scalar replacement) → PASS
  - --no-ion → PASS (baseline always uses TryEnumerableOwnPropertiesNative → no sort)

Comment 1

[Nicolas B. Pierron [:nbp] {away}]

Alex & Iain, I think this is likely to be a regression from Bug 1995077, can you have a look?

Comment 2

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1995077

Comment 3

[Iain Ireland [:iain]]

Attachment: (secure)

This code exists to avoid non-determinism caused by enumerate hooks. That non-determinism shouldn't be a problem for the optimized Object.keys path: that path doesn't support proxies, so lazily resolved global properties don't matter, and AFAICT other lazily resolved properties are non-enumerable, so they don't show up anyway.

I'm not including the test because it's only interesting with --differential-testing, which is only available in debug builds. Locally it fixes the testcase from the bug.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/c533fe4c8c5f
https://hg.mozilla.org/integration/autoland/rev/f0048cfce9d2
Don't sort Object.keys for differential testing r=spidermonkey-reviewers,jandem

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f0048cfce9d2