1921592 - Assertion failure: !obj->nonCCWRealm()->realmFuses.optimizeGetIteratorFuse.intact(), at jit/CacheIR.cpp:14824

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: debug stack

Iterator.prototype.return = 0;
for (let i = 0; i < 1; i++) {
  let [x] = (function () {
    return [0];
  })();
}

(gdb) bt
#0  js::jit::OptimizeGetIteratorIRGenerator::tryAttachArray (this=this@entry=0x7fffffffca20) at /home/i32g7900a/trees/mozilla-central/js/src/jit/CacheIR.cpp:14823
#1  0x00005555581e852e in js::jit::OptimizeGetIteratorIRGenerator::tryAttachStub (this=0x7fffffffca20) at /home/i32g7900a/trees/mozilla-central/js/src/jit/CacheIR.cpp:14778
#2  0x0000555557eb8386 in js::jit::TryAttachStub<js::jit::OptimizeGetIteratorIRGenerator, JS::Handle<JS::Value>&> (cx=0x7ffff7636200, frame=<optimized out>, stub=0x7ffff64f4178, name=<optimized out>, args=...) at /home/i32g7900a/trees/mozilla-central/js/src/jit/BaselineIC.cpp:505
#3  js::jit::DoOptimizeGetIteratorFallback (cx=0x7ffff7636200, frame=<optimized out>, stub=0x7ffff64f4178, value=..., res=...) at /home/i32g7900a/trees/mozilla-central/js/src/jit/BaselineIC.cpp:2591
#4  0x00003b664cb5587b in ?? ()
#5  0x00007ffff64f4160 in ?? ()
/snip

This has occurred since m-c rev 8532c04bf9a8 (Mar 2024), when Jan fixed a new CLI pref issue. To reproduce on this rev, run with --setpref=experimental.iterator_helpers=true. The current regressor points to bug 1896390 but that merely turned on Iterator Helpers.

Run with --fuzzing-safe --no-threads --ion-eager, compile with AR=ar sh ../configure --enable-debug --enable-debug-symbols --with-ccache --enable-nspr-build --enable-ctypes --enable-gczeal --enable-rust-simd --disable-tests --disable-bootstrap, tested on m-c rev 649e6cb6fde3.

Setting s-s just in case. Jan, do you mind taking a look?

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/1cc5c9da21b9
user:        Matthew Gaudet
date:        Wed Jan 10 20:37:04 2024 +0000
summary:     Bug 1871597 - Update assertions around OptimizedGetIterator fuse status r=iain

Found a possible original regressor. Switching needinfo? over to Matt.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Oh wait, Matt's out for a week, Iain's out for a while too. Back to you, Jan!

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 4

[Jan de Mooij [:jandem]]

I think what's happening is that we're not marking Iterator.prototype as having a fuse, so we don't pop the fuse when adding the Iterator.prototype.return property. There's some additional complexity in this area because the iterator prototype is set up differently depending on whether iterator helpers is enabled or disabled.

This is likely not a security issue but just a correctness bug, so I'll leave this for Matt to take a closer look at when he's back.

Let's keep this bug hidden for now.

Comment 5

[Jan de Mooij [:jandem]]

Also, with iterator helpers now enabled on release, it might make sense to fix bug 1910717 first. In particular we should then get rid of ProtoKind::IteratorProto and replace it with getPrototype(JSProto_Iterator).

Comment 6

[Matthew Gaudet (he/him) [:mgaudet]]

So one thing that's interesting about this is that it's not caught by assertRealmFuseInvariants

Also interesting is that there's definitely a fuse supposed to catch this

Something very odd is going on though:

(rr) print cx->global()->maybeGetIteratorPrototype()
$3 = (js::NativeObject *) 0x0
(rr) print obj
$4 = (js::NativeObject * const) 0xfe263541148 [object Iterator.prototype]
(rr) bt 
#0  js::Watchtower::watchPropertyAdd (cx=0x758537736200, obj=(js::NativeObject * const) 0xfe263541148 [object Iterator.prototype], id=$jsid("return")) at /home/matthew/unified-git/js/src/vm/Watchtower.h:91

Somehow we have an iterator prototype which isn't being returned from maybeGetIteratorPrototype?

Also somehow this object, despite being the Object Iterator.prototype doesn't have the UsedAsPrototype objectflag?

(rr) print obj
$6 = (js::NativeObject *) 0xfe263541148 [object Iterator.prototype]
(rr) call js::DumpObject(obj)

{
  "address": "(JSObject*)0xfe263541148",
  "nonCCWGlobal": "<global @ (JSObject*)0xfe26353e030>",
  "clasp": "<Iterator.prototype @ (JSClass*)0x5575788721a8>",
  "shape": "<(js::Shape*)0xfe263561940, objectFlags=[HasInterestingSymbol]>",
  "shape.base": {
    "address": "(js::BaseShape*)0xfe26353c1c0",
    "realm": "(JS::Realm*)0x7585377c8900",
    "proto": "(JSObject*)0xfe263541040"
  },
  "elementsHeader": "<(js::ObjectElements*)0x5575744ebc58, flags=[], init=0, capacity=0, length=0>",
  "properties": {
    "map": "<Function map @ (JSObject*)0xfe2635620b0> (map=(js::CompactPropMap*)0xfe263543c58, index=0, configurable, writable, slot=0)",
    "filter": "<Function filter @ (JSObject*)0xfe263562100> (map=(js::CompactPropMap*)0xfe263543c58, index=1, configurable, writable, slot=1)",
    "take": "<Function take @ (JSObject*)0xfe263562150> (map=(js::CompactPropMap*)0xfe263543c58, index=2, configurable, writable, slot=2)",
    "drop": "<Function drop @ (JSObject*)0xfe2635621a0> (map=(js::CompactPropMap*)0xfe263543c58, index=3, configurable, writable, slot=3)",
    "flatMap": "<Function flatMap @ (JSObject*)0xfe2635621f0> (map=(js::CompactPropMap*)0xfe263543c58, index=4, configurable, writable, slot=4)",
    "reduce": "<Function reduce @ (JSObject*)0xfe263562240> (map=(js::CompactPropMap*)0xfe263543c58, index=5, configurable, writable, slot=5)",
    "toArray": "<Function toArray @ (JSObject*)0xfe263562290> (map=(js::CompactPropMap*)0xfe263543c58, index=6, configurable, writable, slot=6)",
    "forEach": "<Function forEach @ (JSObject*)0xfe2635622e0> (map=(js::CompactPropMap*)0xfe263543c58, index=7, configurable, writable, slot=7)",
    "some": "<Function some @ (JSObject*)0xfe263562330> (map=(js::LinkedPropMap*)0xfe26355bef0, index=0, configurable, writable, slot=8)",
    "every": "<Function every @ (JSObject*)0xfe263562380> (map=(js::LinkedPropMap*)0xfe26355bef0, index=1, configurable, writable, slot=9)",
    "find": "<Function find @ (JSObject*)0xfe2635623d0> (map=(js::LinkedPropMap*)0xfe26355bef0, index=2, configurable, writable, slot=10)",
    "Symbol.iterator": "<Function [Symbol.iterator] @ (JSObject*)0xfe263562420> (map=(js::LinkedPropMap*)0xfe26355bef0, index=3, configurable, writable, slot=11)",
    "Symbol.toStringTag": "getter=0xfe26355fd60, setter=0xfe26355fd98 (map=(js::LinkedPropMap*)0xfe26355bef0, index=4, configurable, slot=12)",
    "constructor": "getter=0xfe26355fdd0, setter=0xfe26355fe08 (map=(js::LinkedPropMap*)0xfe26355bef0, index=5, configurable, slot=13)"
  }
}

Oof. Something is messed up here. I do suspect this isn't a security issue, but I also agree with Jan, probably want to clean this up first before trying to fix this directly.

Comment 7

[Matthew Gaudet (he/him) [:mgaudet]]

Oh particularly gross: it seems part of the issue is that the Iterator.prototype here is actually being populated by the resolve hook. :S

Ok. I think first step is to take care of Jan's Comment 5 suggestion.

Comment 8

[Matthew Gaudet (he/him) [:mgaudet]]

here's a pernosco trace in case anyone wants to follow along

Comment 9

[Matthew Gaudet (he/him) [:mgaudet]]

OK: I think root cause here is esentially that we only watch the iterator prototype for changes when it's actually used as a prototype.

Which it's not when you access it as Iterator.prototype.

I have some patches. And some breakage. Working on it.

Comment 10

[Matthew Gaudet (he/him) [:mgaudet]]

Alright, so one considered direction that I'll have to pick up tomorrow is to create prototypes with the IsUsedAsPrototype flag set, by setting the flag in GenericCreatePrototype

This ends up failing the last assertion in basic/shape-teleporting-invalidation.js (Pernosco Link), as we end up in a situation where Object.prototype being marked as having an invalidated teleporting, which happens when defining the "constructor" property in Iterator.prototype; this new property now shadows the original, where I guess it used to not?

Comment 11

[Daniel Veditz [:dveditz]]

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #9)

OK: I think root cause here is esentially that we only watch the iterator prototype for changes when it's actually used as a prototype.
Which it's not when you access it as Iterator.prototype.

Does that change your assessment from comment 6 that it's not a security problem?

Comment 12

[Matthew Gaudet (he/him) [:mgaudet]]

I can concretely say this isn't a security issue. The code which asserts is already conservatively choosing to not use the fuse. This is a case where being careful has paid dividends -- I consciously chose not to remove the comprehensive checking code that preceeds this assertion. The assertion is there to guarantee that in the case the checking code fails, the fuse state correctly reflects this -- this bug being proof that we should definitely be cautious.

However! There could be correctness issues here: If we had a function prior to this which has been Warp compiled, we could continue executing that body after the mutation of Iterator.prototype.return -- what a use would observe would be the failure to invoke the return method. I have spend the last half hour trying to write a test case where this happens, but I have thus far been unable to. It seems as if the mere act of doing iteration creates the iterator prototype?

I have a patch for this that I like, but building on top of bug 1910717 seems most sensible, as otherwise we have more to consider.

Comment 13

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1921592 - Mark Prototypes created through resolveConstructor as prototypes r?jandem

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/26d6b104ffc6
Mark Prototypes created through resolveConstructor as prototypes r=jandem

Comment 15

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/26d6b104ffc6