GC zeal completely breaks Servo, it seems #25701

nox · 2020-02-06T12:01:18Z

I rebuilt Servo with the following diff:

--- i/components/script/script_thread.rs
+++ w/components/script/script_thread.rs
@@ -3213,6 +3213,12 @@ impl ScriptThread {
             self.event_loop_waker.as_ref().map(|w| (*w).clone_box()),
         );
 
+        let name = unsafe {
+            let window_jsobject = window.reflector().get_jsobject();
+            std::ffi::CStr::from_ptr((*js::jsapi::JS_GetClass(window_jsobject.get())).name)
+        };
+        assert_eq!(name.to_str().unwrap(), "Window");
+
         // Initialize the browsing context for the window.
         let window_proxy = self.local_window_proxy(
             &window,

When running Servo without GC zeal, things just work; but when setting js.mem.gc.zeal.level to 14, this happens:

assertion failed: `(left == right)`
  left: `"Object"`,
 right: `"Window"` (thread ScriptThread PipelineId { namespace_id: PipelineNamespaceId(1), index: PipelineIndex(1) }, at components/script/script_thread.rs:3220)
stack backtrace:
   0: <servo::backtrace::Print as core::fmt::Debug>::fmt
   1: core::fmt::write
   2: std::io::Write::write_fmt
   3: servo::backtrace::print
   4: servo::main::{{closure}}
   5: std::panicking::rust_panic_with_hook
   6: rust_begin_unwind
   7: std::panicking::begin_panic_fmt
   8: script::script_thread::ScriptThread::load
   9: script::script_thread::ScriptThread::handle_page_headers_available
  10: std::thread::local::LocalKey<T>::with
  11: <script::dom::servoparser::ParserContext as net_traits::FetchResponseListener>::process_response
  12: script::script_thread::ScriptThread::handle_msg_from_constellation
  13: _ZN6script13script_thread12ScriptThread11handle_msgs17h4a00e887af7d79f8E.llvm.744559770162711036
  14: profile_traits::mem::ProfilerChan::run_with_memory_reporting
  15: _ZN3std10sys_common9backtrace28__rust_begin_short_backtrace17h1fce977ad6d2c705E.llvm.8072929449113315268
  16: _ZN3std9panicking3try7do_call17h540ae4d5baf90e5dE.llvm.1071807046001960883
  17: __rust_maybe_catch_panic
  18: core::ops::function::FnOnce::call_once{{vtable.shim}}
  19: <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once
  20: std::sys::unix::thread::Thread::new::thread_start
  21: _pthread_body
  22: _pthread_start

AFAIK, this is extremely wrong.

jdm · 2020-02-13T21:09:40Z

It doesn't explain anything in particular, but be aware that the zeal level is not actully a scale of least-zealous to most-zealous (0..14). It's actually a list of modes that all have different meanings: https://searchfox.org/mozilla-central/source/js/src/gc/GCEnum.h#61-83

Typically to have the most zealous GC behaviour we would use level 2 (GC after a certain number of allocations) and set the frequency to a number like 1 (GC after each allocation).

jdm · 2020-02-14T00:42:47Z

rr shows that the value of the JSObject is changed between define_guarded_properties and define_guarded_methods in

servo/components/script/dom/bindings/codegen/CodegenRust.py

Line 2811 in e697e6c

%(members)s

, indicating that a GC occurred in a way that did not end up updating the reflector pointer stored in the DOM window reflector slot.

nox · 2020-02-14T11:14:15Z

Fascinating. I'll try to fix this and maybe my smup issues will magically go away.

nox · 2020-02-14T11:54:52Z

AFAICT, we don't enter the newly created global's realm before we do stuff with it.

https://searchfox.org/mozilla-central/source/dom/bindings/BindingUtils.h#2875

jdm · 2020-02-14T12:56:53Z

We do in

servo/components/script/dom/bindings/codegen/CodegenRust.py

Line 2803 in e697e6c

let _ac = JSAutoRealm::new(*cx, obj.get());

, but maybe you're talking about

servo/components/script/dom/bindings/interface.rs

Line 151 in e697e6c

// Initialize the reserved slots before doing anything that can GC, to

instead?

jdm · 2020-02-14T13:57:15Z

This is the stack showing a GC occurring that changes the new window reflector object pointer:

#0  0x0000564c0a40eb40 in js::gc::MovingTracer::updateEdge<JSObject>(JSObject**) (this=0x7faa46ce2e80, thingp=0x7faa46ce4958)
    at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/gc/GC.cpp:2460
#1  0x0000564c0a40eabd in js::gc::MovingTracer::onObjectEdge(JSObject**) (this=0x7faa46ce2e80, objp=0x7faa46ce4958)
    at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/gc/GC.cpp:2464
#2  0x0000564c0a4c5e0e in JS::CallbackTracer::dispatchToOnEdge(JSObject**) (this=0x7faa46ce2e80, objp=0x7faa46ce4958)
    at /home/jdm/servo-smup/target/debug/build/mozjs_sys-6c509b7078b296a4/out/dist/include/js/TracingAPI.h:257
#3  0x0000564c0a4c5d9f in DoCallback<JSObject>(JS::CallbackTracer*, JSObject**, char const*) (trc=0x7faa46ce2e80, thingp=0x7faa46ce4958, name=0x564c0fad53f5 "exact-Object")
    at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/gc/Tracer.cpp:45
#4  0x0000564c0a43b736 in js::gc::TraceEdgeInternal<JSObject*>(JSTracer*, JSObject**, char const*) (trc=0x7faa46ce2e88, thingp=0x7faa46ce4958, name=0x564c0fad53f5 "exact-Object")
    at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/gc/Marking.cpp:570
#5  0x0000564c09f7cdfa in js::TraceNullableRoot<JSObject*>(JSTracer*, JSObject**, char const*) (trc=0x7faa46ce2e88, thingp=0x7faa46ce4958, name=0x564c0fad53f5 "exact-Object")
    at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/gc/Tracer.h:168
#6  0x0000564c0a4c9345 in TraceStackOrPersistentRoot<JSObject*>(JSTracer*, JSObject**, char const*) (trc=0x7faa46ce2e88, thingp=0x7faa46ce4958, name=0x564c0fad53f5 "exact-Object")
    at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/gc/RootMarking.cpp:54
#7  0x0000564c0a4c8ca0 in TraceExactStackRootList<JSObject*>(JSTracer*, JS::Rooted<void*>*, char const*) (trc=0x7faa46ce2e88, rooter=0x7faa46ce4948, name=0x564c0fad53f5 "exact-Object")
    at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/gc/RootMarking.cpp:69
#8  0x0000564c0a4b836e in TraceStackRoots(JSTracer*, mozilla::EnumeratedArray<JS::RootKind, (JS::RootKind)15, JS::Rooted<void*>*>&) (trc=0x7faa46ce2e88, stackRoots=...)
    at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/gc/RootMarking.cpp:79
#9  0x0000564c0a4b825d in JS::RootingContext::traceStackRoots(JSTracer*) (this=0x7faa4c00e650, trc=0x7faa46ce2e88)
    at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/gc/RootMarking.cpp:93
#10 0x0000564c0a4baaf8 in TraceExactStackRoots(JSContext*, JSTracer*) (cx=0x7faa4c00e650, trc=0x7faa46ce2e88)
    at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/gc/RootMarking.cpp:97
#11 0x0000564c0a4ba645 in js::gc::GCRuntime::traceRuntimeCommon(JSTracer*, js::gc::GCRuntime::TraceOrMarkRuntime) (this=0x7faa4c001548, trc=0x7faa46ce2e88, traceOrMark=js::gc::GCRuntime::MarkRuntime)
    at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/gc/RootMarking.cpp:360
#12 0x0000564c0a4ba3bf in js::gc::GCRuntime::traceRuntimeForMajorGC(JSTracer*, js::gc::AutoGCSession&) (this=0x7faa4c001548, trc=0x7faa46ce2e88, session=...)
    at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/gc/RootMarking.cpp:285
#13 0x0000564c0a4112a5 in js::gc::GCRuntime::updateRuntimePointersToRelocatedCells(js::gc::AutoGCSession&) (this=0x7faa4c001548, session=...)
    at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/gc/GC.cpp:2895
#14 0x0000564c0a42908b in js::gc::GCRuntime::compactPhase(JS::GCReason, js::SliceBudget&, js::gc::AutoGCSession&) (this=0x7faa4c001548, reason=JS::GCReason::DEBUG_GC, sliceBudget=..., session=...)
    at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/gc/GC.cpp:6663
#15 0x0000564c0a42b76d in js::gc::GCRuntime::incrementalSlice(js::SliceBudget&, JS::GCReason, js::gc::AutoGCSession&) (this=0x7faa4c001548, budget=..., reason=JS::GCReason::DEBUG_GC, session=...)
    at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/gc/GC.cpp:7104
#16 0x0000564c0a42ced0 in js::gc::GCRuntime::gcCycle(bool, js::SliceBudget, JS::GCReason) (this=0x7faa4c001548, nonincrementalByAPI=true, budget=..., reason=JS::GCReason::DEBUG_GC)
    at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/gc/GC.cpp:7398
#17 0x0000564c0a42e353 in js::gc::GCRuntime::collect(bool, js::SliceBudget, JS::GCReason) (this=0x7faa4c001548, nonincrementalByAPI=true, budget=..., reason=JS::GCReason::DEBUG_GC)
    at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/gc/GC.cpp:7569
#18 0x0000564c0a3f8946 in js::gc::GCRuntime::gc(JSGCInvocationKind, JS::GCReason) (this=0x7faa4c001548, gckind=GC_SHRINK, reason=JS::GCReason::DEBUG_GC)
    at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/gc/GC.cpp:7657
#19 0x0000564c0a3f86dc in js::gc::GCRuntime::runDebugGC() (this=0x7faa4c001548) at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/gc/GC.cpp:8193
#20 0x0000564c0a3f8066 in js::gc::GCRuntime::gcIfNeededAtAllocation(JSContext*) (this=0x7faa4c001548, cx=0x7faa4c00e650)
    at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/gc/Allocator.cpp:336
#21 0x0000564c0a3f36c2 in js::gc::GCRuntime::checkAllocatorState<(js::AllowGC)1>(JSContext*, js::gc::AllocKind) (this=0x7faa4c001548, cx=0x7faa4c00e650, kind=js::gc::AllocKind::ACCESSOR_SHAPE)
    at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/gc/Allocator.cpp:300
#22 0x0000564c0a3f5d98 in js::Allocate<js::AccessorShape, (js::AllowGC)1>(JSContext*) (cx=0x7faa4c00e650)
    at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/gc/Allocator.cpp:244
#23 0x0000564c0a09d192 in js::Shape::new_(JSContext*, JS::Handle<js::StackShape>, unsigned int) (cx=0x7faa4c00e650, other=..., nfixed=16)
    at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/vm/Shape-inl.h:112
#24 0x0000564c0a0404bc in js::PropertyTree::inlinedGetChild(JSContext*, js::Shape*, JS::Handle<js::StackShape>) (this=0x7faa4c029a80, cx=0x7faa4c00e650, parent=0x26443a299708, child=...)
    at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/vm/Shape.cpp:1841
#25 0x0000564c0a03deb4 in js::NativeObject::getChildAccessorProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<js::Shape*>, JS::MutableHandle<js::StackShape>) (cx=0x7faa4c00e650, obj=..., parent=..., child=...) at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/vm/Shape.cpp:435
#26 0x0000564c0a03d4c1 in js::NativeObject::addAccessorPropertyInternal(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JS::PropertyKey>, bool (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>), bool (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, JS::Handle<JS::Value>, JS::ObjectOpResult&), unsigned int, js::ShapeTable*, js::ShapeTable::Entry*, js::AutoKeepShapeCaches const&) (cx=0x7faa4c00e650, obj=..., id=..., getter=0x26443a29ba80, setter=0x26443a29bac0, attrs=49, table=0x0, entry=0x0, keep=...)
    at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/vm/Shape.cpp:648
#27 0x0000564c09c2d03c in js::NativeObject::addAccessorProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JS::PropertyKey>, bool (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>), bool (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, JS::Handle<JS::Value>, JS::ObjectOpResult&), unsigned int) (cx=0x7faa4c00e650, obj=..., id=..., getter=0x26443a29ba80, setter=0x26443a29bac0, attrs=49) at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/vm/Shape-inl.h:423
#28 0x0000564c09f17a9e in AddOrChangeProperty<(IsAddOrChange)0>(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JS::PropertyKey>, JS::Handle<JS::PropertyDescriptor>) (cx=0x7faa4c00e650, obj=..., id=..., desc=...) at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/vm/NativeObject.cpp:1443
#29 0x0000564c09f16366 in js::NativeDefineProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JS::PropertyKey>, JS::Handle<JS::PropertyDescriptor>, JS::ObjectOpResult&) (cx=0x7faa4c00e650, obj=..., id=..., desc_=..., result=...) at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/vm/NativeObject.cpp:1745
#30 0x0000564c09eb8904 in js::DefineAccessorProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, JS::Handle<JSObject*>, JS::Handle<JSObject*>, unsigned int, JS::ObjectOpResult&) (cx=0x7faa4c00e650, obj=..., id=..., getter=..., setter=..., attrs=49, result=...) at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/vm/JSObject.cpp:2963
#31 0x0000564c09eb8dd1 in js::DefineAccessorProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, JS::Handle<JSObject*>, JS::Handle<JSObject*>, unsigned int) (cx=0x7faa4c00e650, obj=..., id=..., getter=..., setter=..., attrs=49) at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/vm/JSObject.cpp:2999
#32 0x0000564c0a226ba2 in DefineAccessorPropertyById(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, JS::Handle<JSObject*>, JS::Handle<JSObject*>, unsigned int) (cx=0x7faa4c00e650, obj=..., id=..., getter=..., setter=..., attrs=49) at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/jsapi.cpp:1891
#33 0x0000564c0a2268ff in DefineAccessorPropertyById(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, JSNativeWrapper const&, JSNativeWrapper const&, unsigned int) (cx=0x7faa4c00e650, obj=..., id=..., get=..., set=..., attrs=49) at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/jsapi.cpp:1938
#34 0x0000564c0a22a3d3 in JS_DefineProperties(JSContext*, JS::Handle<JSObject*>, JSPropertySpec const*) (cx=0x7faa4c00e650, obj=..., ps=0x564c12c88148)
    at /home/jdm/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/jsapi.cpp:2942
#35 0x0000564c09bb6ab4 in mozjs::rust::define_properties (cx=0x7faa4c00e650, obj=..., properties=...) at /home/jdm/.cargo/git/checkouts/rust-mozjs-8611526964119dd6/9b0d063/src/rust.rs:1146
#36 0x0000564c078085eb in script::dom::bindings::interface::define_guarded_properties (cx=..., obj=..., properties=..., global=...) at components/script/dom/bindings/interface.rs:366
#37 0x0000564c0867e503 in script::dom::bindings::codegen::Bindings::WindowBinding::WindowBinding::Wrap (cx=..., object=0x7faa3d973800)
    at /home/jdm/servo-smup/target/debug/build/script-f2153caedd910c94/out/Bindings/WindowBinding.rs:9701

I'll ask the SM folks who sit next to me to tell me more about compaction GCs and how those are expected to interact with GC pointers reachable from the rooting stack via Heap objects (which is what the Window reflector reachable from the DOM object via a stack root is).

jdm · 2020-02-14T14:01:03Z

For reference, once I realized that the pointer stored in the Window object was not the same as the pointer stored in the stack root at the end of Window::Wrap, all I needed to do was do watch *obj.ptr in define_guarded_properties and run continue, then the program was interrupted when a GC occurred that changed the pointer value.

jdm · 2020-02-14T14:36:58Z

I figured out what's going on. There are a few subtleties:

we don't create a DomRoot object for the Window until right before we return from WindowBindings::Wrap
we have an empty trace implementation for Reflector
to ensure we trace reflectors properly that are reachable from DOM objects and stack roots of DOM objects, we handle tracing Dom and tracing Root in a special way that explicitly traces reflectors

This means that any time we have a DOM object which is only reachable from JS roots, any GCs that occur will encounter its Reflector and silently ignore it, leading to GC hazards like this one. Any time that DOM object is reachable from a Dom<T> or Root<T> (such as DomRoot), we will trace the reflector as expected and there will be no hazard.

jdm · 2020-02-14T14:38:11Z

When I move the DomRoot to right after create_global_object, the unexpected behaviour goes away and we start hitting zeal hazards that #24069 fixes.

nox · 2020-02-14T14:54:36Z

Very nice, I’ll make a patch and see if it helps the smup.


        Auto merge of #25781 - nox:fixzeal, r=<try>

Introduce a new type MaybeUnreflectedDom<T> (fixes #25701)


        Introduce a new type MaybeUnreflectedDom<T> (fixes #25701)


        Auto merge of #25781 - nox:fixzeal, r=jdm

Introduce a new type MaybeUnreflectedDom<T> (fixes #25701)


        Auto merge of #25781 - nox:fixzeal, r=jdm

Introduce a new type MaybeUnreflectedDom<T> (fixes #25701)


        Auto merge of #25781 - nox:fixzeal, r=jdm

Introduce a new type MaybeUnreflectedDom<T> (fixes #25701)

nox added I-panic I-wrong A-content/script labels Feb 6, 2020

nox added a commit that referenced this issue Feb 18, 2020

Introduce a new type MaybeUnreflectedDom<T> (fixes #25701)

Loading status checks…

930174e

jdm mentioned this issue Feb 21, 2020

Introduce a new type MaybeUnreflectedDom<T> (fixes #25701) #25781

Merged

bors-servo closed this in 14846d0 Mar 2, 2020

servo / servo

GC zeal completely breaks Servo, it seems #25701

GC zeal completely breaks Servo, it seems #25701

nox commented Feb 6, 2020 •

edited

jdm commented Feb 13, 2020

jdm commented Feb 14, 2020

nox commented Feb 14, 2020

nox commented Feb 14, 2020

jdm commented Feb 14, 2020

jdm commented Feb 14, 2020

jdm commented Feb 14, 2020

jdm commented Feb 14, 2020

jdm commented Feb 14, 2020

nox commented Feb 14, 2020

servo / servo

Join GitHub today

GC zeal completely breaks Servo, it seems #25701

GC zeal completely breaks Servo, it seems #25701

Comments

nox commented Feb 6, 2020 • edited

jdm commented Feb 13, 2020

jdm commented Feb 14, 2020

nox commented Feb 14, 2020

nox commented Feb 14, 2020

jdm commented Feb 14, 2020

jdm commented Feb 14, 2020

jdm commented Feb 14, 2020

jdm commented Feb 14, 2020

jdm commented Feb 14, 2020

nox commented Feb 14, 2020

nox commented Feb 6, 2020 •

edited