assertion failed: RUST_JSID_IS_STRING(id)

[mateon1]

Found on 101xp.com, 100% reproducible (currently reducing a testcase jdm found the issue immediately).

Crashing JS snippet: delete localStorage[5] - "deleting localStorage objects using a key that is not a string violates an assumption that we made" (Source: jdm on IRC)

assertion failed: RUST_JSID_IS_STRING(id) (thread ScriptThread PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(0) }, at /shared/dev/rust/servo/components/script/dom/bindings/conversions.rs:144)
stack backtrace:
   0:     0x558367ec92e4 - backtrace::backtrace::libunwind::trace
                        at /shared/dev/rust/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.2/src/backtrace/libunwind.rs:53
                         - backtrace::backtrace::trace<closure>
                        at /shared/dev/rust/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.2/src/backtrace/mod.rs:42
   1:     0x558367ec386f - backtrace::capture::{{impl}}::new
                        at /shared/dev/rust/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.2/src/capture.rs:64
   2:     0x5583635f6ccc - servo::main::{{closure}}
                        at /shared/dev/rust/servo/ports/servo/main.rs:130
   3:     0x55836a8dd316 - std::panicking::rust_panic_with_hook
                        at /checkout/src/libstd/panicking.rs:612
   4:     0x5583660282d6 - std::panicking::begin_panic<&str>
                        at /checkout/src/libstd/panicking.rs:572
   5:     0x558365a98a21 - script::dom::bindings::conversions::string_jsid_to_string
                        at /shared/dev/rust/servo/components/script/dom/bindings/conversions.rs:144
   6:     0x558367ad5808 - script::dom::bindings::codegen::Bindings::StorageBinding::StorageBinding::delete::{{closure}}
                        at /shared/dev/rust/servo/target/debug/build/script-d00ee28330b6ecd4/out/Bindings/StorageBinding.rs:990
   7:     0x55836763163f - core::ops::function::FnOnce::call_once<closure,()>
                        at /checkout/src/libcore/ops/function.rs:223
   8:     0x5583672d4e75 - std::panic::{{impl}}::call_once<bool,closure>
                        at /checkout/src/libstd/panic.rs:296
   9:     0x5583661c0d94 - std::panicking::try::do_call<std::panic::AssertUnwindSafe<closure>,bool>
                        at /checkout/src/libstd/panicking.rs:480
  10:     0x55836a8e43bc - panic_unwind::__rust_maybe_catch_panic
                        at /checkout/src/libpanic_unwind/lib.rs:99
  11:     0x558366106bd5 - std::panicking::try<bool,std::panic::AssertUnwindSafe<closure>>
                        at /checkout/src/libstd/panicking.rs:459
  12:     0x558367504749 - std::panic::catch_unwind<std::panic::AssertUnwindSafe<closure>,bool>
                        at /checkout/src/libstd/panic.rs:361
  13:     0x5583673ab292 - js::panic::wrap_panic<std::panic::AssertUnwindSafe<closure>,bool>
                        at /shared/dev/rust/servo/.cargo/git/checkouts/rust-mozjs-8611526964119dd6/3de4ff3/src/panic.rs:22
  14:     0x55836724056f - script::dom::bindings::codegen::Bindings::StorageBinding::StorageBinding::delete
                        at /shared/dev/rust/servo/target/debug/build/script-d00ee28330b6ecd4/out/Bindings/StorageBinding.rs:989
  15:     0x558368111d29 - _ZNK22ForwardingProxyHandler7delete_EP9JSContextN2JS6HandleIP8JSObjectEENS3_I4jsidEERNS2_14ObjectOpResultE
                        at /shared/dev/rust/servo/.cargo/git/checkouts/rust-mozjs-8611526964119dd6/3de4ff3/src/jsglue.cpp:410
  16:     0x55836866a4a7 - _ZN2js5Proxy7delete_EP9JSContextN2JS6HandleIP8JSObjectEENS4_I4jsidEERNS3_14ObjectOpResultE
                        at /shared/dev/rust/servo/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/834ce35/mozjs/js/src/proxy/Proxy.cpp:160
  17:     0x55836866c6ea - _ZN2js20proxy_DeletePropertyEP9JSContextN2JS6HandleIP8JSObjectEENS3_I4jsidEERNS2_14ObjectOpResultE
                        at /shared/dev/rust/servo/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/834ce35/mozjs/js/src/proxy/Proxy.cpp:602
  18:     0x5583684fc972 - _ZN2js14DeletePropertyEP9JSContextN2JS6HandleIP8JSObjectEENS3_I4jsidEERNS2_14ObjectOpResultE
                        at /shared/dev/rust/servo/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/834ce35/mozjs/js/src/jsobjinlines.h:225
  19:     0x558368766aff - Interpret
                        at /shared/dev/rust/servo/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/834ce35/mozjs/js/src/vm/Interpreter.cpp:2502
  20:     0x55836875c055 - _ZN2js9RunScriptEP9JSContextRNS_8RunStateE
                        at /shared/dev/rust/servo/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/834ce35/mozjs/js/src/vm/Interpreter.cpp:399
  21:     0x55836875c4a2 - _ZN2js23InternalCallOrConstructEP9JSContextRKN2JS8CallArgsENS_14MaybeConstructE
                        at /shared/dev/rust/servo/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/834ce35/mozjs/js/src/vm/Interpreter.cpp:471
  22:     0x55836875c6fa - InternalCall
                        at /shared/dev/rust/servo/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/834ce35/mozjs/js/src/vm/Interpreter.cpp:498
  23:     0x55836875c78c - _ZN2js4CallEP9JSContextN2JS6HandleINS2_5ValueEEES5_RKNS_13AnyInvokeArgsENS2_13MutableHandleIS4_EE
                        at /shared/dev/rust/servo/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/834ce35/mozjs/js/src/vm/Interpreter.cpp:517
  24:     0x55836856e384 - _ZN2js9fun_applyEP9JSContextjPN2JS5ValueE
                        at /shared/dev/rust/servo/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/834ce35/mozjs/js/src/jsfun.cpp:1325
  25:     0x55836878df7b - _ZN2js12CallJSNativeEP9JSContextPFbS1_jPN2JS5ValueEERKNS2_8CallArgsE
                        at /shared/dev/rust/servo/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/834ce35/mozjs/js/src/jscntxtinlines.h:232
  26:     0x55836875c3d3 - _ZN2js23InternalCallOrConstructEP9JSContextRKN2JS8CallArgsENS_14MaybeConstructE
                        at /shared/dev/rust/servo/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/834ce35/mozjs/js/src/vm/Interpreter.cpp:453
  27:     0x55836875c6fa - InternalCall
                        at /shared/dev/rust/servo/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/834ce35/mozjs/js/src/vm/Interpreter.cpp:498
  28:     0x55836875c724 - _ZN2js13CallFromStackEP9JSContextRKN2JS8CallArgsE
                        at /shared/dev/rust/servo/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/834ce35/mozjs/js/src/vm/Interpreter.cpp:504
  29:     0x55836814b880 - DoCallFallback
                        at /shared/dev/rust/servo/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/834ce35/mozjs/js/src/jit/BaselineIC.cpp:5970
  30:     0x7f6fa993fe23 - <unknown>
ERROR:servo: assertion failed: RUST_JSID_IS_STRING(id)
Pipeline failed in hard-fail mode.  Crashing!

[jdm]

This triggers the assertion:

<script>delete localStorage[5]</script>

[jdm]

Given that the failing function is called string_jsid_to_string, it looks like the generated code is making a faulty assumption here that the key will already be a string.

[toidiu]

@jdm I can take a look at this.

[jdm]

Great! The code in question is generated from CodegenRust.py. please ask questions if anything is unclear!

[toidiu]

hi @jdm i am a bit lost and need guidance 😕 the code base if massive.

• what is the source that is generating 'js::glue::RUST_JSID_IS_STRING' and 'js::jsapi::HandleId'

also is there something that gives an overview of how the repo is organized and what its doing(purpose of codegen, why python, other relevant stuff for this issue)

[toidiu]

@jdm
Took a closer look at the stack trace and see that most of the things are cpp libraries. The only rust code seems to be StorageBinding.rs:990 so only concerning myself with that mainly.

I am still not sure what the desired behavior should be.

• check if string
• call a different function if not string (which function)
• what are the other possible values? (u32?)

[jdm]

This is the code that is generating the string_jsid_to_string call that is failing: https://dxr.mozilla.org/servo/rev/f1da967ef707bbc77f023daf28093201e96e19c5/components/script/dom/bindings/codegen/CodegenRust.py#4753
We have a jsid_to_string function that we should call instead which handles non-string values more appropriately.

[jdm]

I recommend matching on the result of that and returning false if it's None.

[toidiu]

@jdm @emilio I am a bit stalled on this one and in need of some direction. From what I can tell the crash happens in a cpp lib?

Discussion of the current status is here: #18546

[ysimonson]

I think this is fixed. I created a test page with the source code <script>delete localStorage[5]</script>, and navigated to it via ./mach run http://localhost:8000/test.html. No panic.

[jdm]

@ysimonson what about delete localStorage[Symbol()]?

[ysimonson]

Panic!

ERROR:servo: Not a string-convertible JSID?
called `Result::unwrap()` on an `Err` value: RecvError (thread LayoutThread PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(NonZero(NonZero(1))) }, at libcore/result.rs:916)
stack backtrace:
   0:        0x1172e8804 - backtrace::backtrace::trace::h602035e546ae0f06
                        at /Users/yusuf/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.2/src/backtrace/mod.rs:42
   1:        0x1172df54c - backtrace::capture::Backtrace::new::h6bb9ed882561a6c9
                        at /Users/yusuf/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.2/src/capture.rs:64
   2:        0x10f7c123e - servo::main::_$u7b$$u7b$closure$u7d$$u7d$::h16f26bb40f883f6e
                        at ports/servo/main.rs:147
   3:        0x1172f4420 - std::panicking::rust_panic_with_hook::h47a0773bed022912
                        at libstd/panicking.rs:577
   4:        0x1172f423e - std::panicking::begin_panic::h357aa9770ebb9277
                        at libstd/panicking.rs:537
   5:        0x1172f4193 - std::panicking::begin_panic_fmt::h8146069d366cb090
                        at libstd/panicking.rs:521
   6:        0x1172f4102 - rust_begin_unwind
                        at libstd/panicking.rs:497
   7:        0x11732ab33 - core::panicking::panic_fmt::hde64b9c715e999e9
                        at libcore/panicking.rs:71
   8:        0x1101daeab - core::result::unwrap_failed::hc7ebde2f65cdd712
                        at /Users/travis/build/rust-lang/rust/src/libcore/macros.rs:23
   9:        0x1101d0656 - _$LT$core..result..Result$LT$T$C$$u20$E$GT$$GT$::unwrap::hfe65db05a80f2ebf
                        at /Users/travis/build/rust-lang/rust/src/libcore/result.rs:782
  10:        0x11069b89e - layout_thread::LayoutThread::handle_request::hc4535dd309c3cbe1
                        at components/layout_thread/lib.rs:610
  11:        0x11069ae6f - layout_thread::LayoutThread::start::hc6bc6852e76a39c6
                        at components/layout_thread/lib.rs:555
  12:        0x110322e21 - _$LT$layout_thread..LayoutThread$u20$as$u20$layout_traits..LayoutThreadFactory$GT$::create::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::h8cd8c3854c67742c
                        at components/layout_thread/lib.rs:313
  13:        0x1106e29dd - profile_traits::mem::ProfilerChan::run_with_memory_reporting::h9c2ab43573250ea6
                        at /Users/yusuf/servo/components/profile_traits/mem.rs:63
  14:        0x1103233ea - _$LT$layout_thread..LayoutThread$u20$as$u20$layout_traits..LayoutThreadFactory$GT$::create::_$u7b$$u7b$closure$u7d$$u7d$::h98a2899416eb926e
                        at components/layout_thread/lib.rs:312
  15:        0x11060f0d4 - std::sys_common::backtrace::__rust_begin_short_backtrace::h35c4be475f4d12b4
                        at /Users/travis/build/rust-lang/rust/src/libstd/sys_common/backtrace.rs:133
  16:        0x11025aeeb - std::thread::Builder::spawn::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::h19a3ab362e128357
                        at /Users/travis/build/rust-lang/rust/src/libstd/thread/mod.rs:406
  17:        0x1104f4a34 - _$LT$std..panic..AssertUnwindSafe$LT$F$GT$$u20$as$u20$core..ops..function..FnOnce$LT$$LP$$RP$$GT$$GT$::call_once::h3a4155c6a5a96cbb
                        at /Users/travis/build/rust-lang/rust/src/libstd/panic.rs:293
  18:        0x1103a5274 - std::panicking::try::do_call::h15356930134b84ae
                        at /Users/travis/build/rust-lang/rust/src/libstd/panicking.rs:479
  19:        0x11731f20e - __rust_maybe_catch_panic
                        at libpanic_unwind/lib.rs:102
  20:        0x1103a514c - std::panicking::try::hffc5523524b2d199
                        at /Users/travis/build/rust-lang/rust/src/libstd/panicking.rs:458
  21:        0x1104f4f8c - std::panic::catch_unwind::hfc5eb93d21977a36
                        at /Users/travis/build/rust-lang/rust/src/libstd/panic.rs:358
  22:        0x11025ad18 - std::thread::Builder::spawn::_$u7b$$u7b$closure$u7d$$u7d$::h388e693032906ad0
                        at /Users/travis/build/rust-lang/rust/src/libstd/thread/mod.rs:405
  23:        0x11025af45 - _$LT$F$u20$as$u20$alloc..boxed..FnBox$LT$A$GT$$GT$::call_box::h50f7fea03e2e73ed
                        at /Users/travis/build/rust-lang/rust/src/liballoc/boxed.rs:788
  24:        0x117308187 - std::sys_common::thread::start_thread::h1ef8cf6b973abe77
                        at libstd/sys_common/thread.rs:24
  25:        0x11730b5b8 - std::sys::unix::thread::Thread::new::thread_start::h88d15e6243c319fe
                        at libstd/sys/unix/thread.rs:90
  26:     0x7fff70c866c0 - _pthread_body
  27:     0x7fff70c8656c - _pthread_start
ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError

(For posterity: while the error message shows up immediately, the backtrace does not show up until you close the window.)

[jdm]

Looks like that comes from this code. The equivalent Gecko code calls a function that indicates whether a symbol is present or not.

[jdm]

We should probably make our implementation of jsid_to_string match Gecko's ConvertIdToString implementation.