Bug 1675755 Comment 17 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

I finally managed to get this uploaded to Pernosco with source code and everything. It was still a pain to track down, but I think I have it.

The problem is that a `Shape` is given a cross-zone pointer to a `BaseShape`. These should very much not be cross-zone edges, and are not handled as such. As a result, there is a GC that does not include the `Shape`'s `Zone`, so nothing marks the `BaseShape` and it gets freed. In detail:

There's `obj`, a `js::LexicalEnvironmentObject`.

- major GC #9: `obj` is traced as the environment chain of an Interpreter frame
- `obj->shape` runs `Shape::cachify`, which creates a new owned `BaseShape*` in the wrong `Zone` (because the `cx`'s is not in the `Shape`'s `Realm`.)
- major GC #10: `obj` is not traced because its zone is not collected. The base shape is freed.
- major GC #11: `obj` is traced as a Realm's global lexical environment, which traces obj->shape->baseShape and crashes.

The stack where the `BaseShape` is constructed is:
```
#0  js::Shape::makeOwnBaseShape (this=0x232b42655600, cx=0x55d7f3ef1630) at js/src/vm/Shape.cpp:136
#1  js::Shape::ensureOwnBaseShape (this=0x232b42655600, cx=<optimized out>) at js/src/vm/Shape.h:1055
#2  js::Shape::cachify (cx=0x55d7f3ef1630, shape=0x232b42655600) at js/src/vm/Shape.cpp:205
#3  0x00007f4d94ec8af7 in js::Shape::maybeCreateCacheForLookup (this=0x232b42655600, cx=0x55d7f3ef1630) at js/src/vm/Shape-inl.h:54
#4  js::Shape::search<(js::MaybeAdding)0> (cx=0x55d7f3ef1630, start=0x232b42655600, id=...) at js/src/vm/Shape-inl.h:83
#5  js::Shape::search (this=0x232b42655600, cx=0x55d7f3ef1630, id=...) at js/src/vm/Shape-inl.h:37
#6  js::LookupOwnPropertyInline<(js::AllowGC)1> (cx=0x55d7f3ef1630, obj=..., id=..., propp=..., donep=<optimized out>) at js/src/vm/NativeObject-inl.h:795
```
I'm honestly not familiar with when the right time is to switch `Realm`s here -- at the low-level, before allocating, or at the semantically meaningful level of looking up a property. But I'll do one and find out via a review. ;-)
I finally managed to get this uploaded to Pernosco with source code and everything. It was still a pain to track down, but I think I have it.

The problem is that a `Shape` is given a cross-zone pointer to a `BaseShape`. These should very much not be cross-zone edges, and are not handled as such. As a result, there is a GC that does not include the `Shape`'s `Zone`, so nothing marks the `BaseShape` and it gets freed. In detail:

There's `obj`, a `js::LexicalEnvironmentObject`.

- major GC #9: `obj` is traced as the environment chain of an Interpreter frame
- `obj->shape` runs `Shape::cachify`, which creates a new owned `BaseShape*` in the wrong `Zone` (because the `cx`'s is not in the `Shape`'s `Realm`.)
- major GC #10: `obj` is not traced because its zone is not collected. The base shape is freed.
- major GC #11: `obj` is traced as a Realm's global lexical environment, which traces obj->shape->baseShape and crashes.

The stack where the `BaseShape` is constructed is:
```
#0  js::Shape::makeOwnBaseShape (this=0x232b42655600, cx=0x55d7f3ef1630) at js/src/vm/Shape.cpp:136
#1  js::Shape::ensureOwnBaseShape (this=0x232b42655600, cx=<optimized out>) at js/src/vm/Shape.h:1055
#2  js::Shape::cachify (cx=0x55d7f3ef1630, shape=0x232b42655600) at js/src/vm/Shape.cpp:205
#3  0x00007f4d94ec8af7 in js::Shape::maybeCreateCacheForLookup (this=0x232b42655600, cx=0x55d7f3ef1630) at js/src/vm/Shape-inl.h:54
#4  js::Shape::search<(js::MaybeAdding)0> (cx=0x55d7f3ef1630, start=0x232b42655600, id=...) at js/src/vm/Shape-inl.h:83
#5  js::Shape::search (this=0x232b42655600, cx=0x55d7f3ef1630, id=...) at js/src/vm/Shape-inl.h:37
#6  js::LookupOwnPropertyInline<(js::AllowGC)1> (cx=0x55d7f3ef1630, obj=..., id=..., propp=..., donep=<optimized out>) at js/src/vm/NativeObject-inl.h:795
#7  js::LookupPropertyInline<(js::AllowGC)1> (cx=0x55d7f3ef1630, obj=..., id=..., objp=..., propp=...) at js/src/vm/NativeObject-inl.h:881
#8  js::LookupProperty (cx=0x55d7f3ef1630, obj=..., id=..., objp=..., propp=...) at js/src/vm/JSObject.cpp:2174
#9  0x00007f4d950f2419 in js::DebuggerObject::forceLexicalInitializationByName (cx=0x55d7f3ef1630, object=..., id=..., result=@0x7ffde1b170af: false) at js/src/debugger/Object.cpp:2384
#10 0x00007f4d950f20f9 in js::DebuggerObject::CallData::forceLexicalInitializationByNameMethod (this=0x7ffde1b170f0) at js/src/debugger/Object.cpp:1084
#11 0x00007f4d950ffbe3 in js::DebuggerObject::CallData::ToNative<&js::DebuggerObject::CallData::forceLexicalInitializationByNameMethod> (cx=0x55d7f3ef1630, argc=<optimized out>, vp=<optimized out>) at js/src/debugger/Object.cpp:243
#12 0x00007f4d94d2396b in CallJSNative (cx=0x55d7f3ef1630, native=0x7f4d950ffb20 <js::DebuggerObject::CallData::ToNative<&js::DebuggerObject::CallData::forceLexicalInitializationByNameMethod>(JSContext*, unsigned int, JS::Value*)>, reason=<optimized out>, args=...) at js/src/vm/Interpreter.cpp:507
#13 js::InternalCallOrConstruct (cx=0x55d7f3ef1630, args=..., construct=<optimized out>, reason=<optimized out>) at js/src/vm/Interpreter.cpp:599
#14 0x00007f4d94d24227 in InternalCall (cx=<optimized out>, args=..., reason=js::CallReason::Call) at js/src/vm/Interpreter.cpp:664
#15 0x00007f4d952c6ee9 in js::jit::DoCallFallback (cx=0x55d7f3ef1630, frame=0x7ffde1b17690, stub=<optimized out>, argc=<optimized out>, vp=<optimized out>, res=...) at js/src/jit/BaselineIC.cpp:3010
#16 0x000026f80999ed68 in ?? ()
```
I'm honestly not familiar with when the right time is to switch `Realm`s here -- at the low-level, before allocating, or at the semantically meaningful level of looking up a property. But I'll do one and find out via a review. ;-)

Back to Bug 1675755 Comment 17