Consider using as_unsafe_cell when tracing RefCell values during GC

[jdm]

We don't have any guarantees about when GCs will run, so it's entirely possible (even likely?) that we'll end up double-borrowing a RefCell by triggering a GC while there is a mutable borrow outstanding. I think it's safe to just use as_unsafe_cell during this, since it's a read-only operation.

[Manishearth]

What if we manipulate some form of list of JS objects during the GC pass? We might end up accessing freed memory or something.

[Manishearth]

Though the solution for that seems to lie in Mutex land. I guess any member that is a pointer of JS pointers should be designed carefully so that this doesn't happen.

[Ms2ger]

it's a read-only operation

But it won't be, will it?

[jdm]

Mmm, right, I forgot that the pointer for JS<T> can change with the upgrade. That wouldn't be affected by changing RefCell's behaviour here, though, would it?

[jdm]

This is showing up as intermittent test failures. I'm inclined to proceed with this change.

thread 'ScriptTask' panicked at 'DOMRefCell<T> already mutably borrowed', /Users/servo/buildbot/slave/mac2/build/components/script/dom/bindings/cell.rs:125
stack backtrace:
   1:        0x10b091927 - sys::backtrace::write::h084c3267f5afd4a3hvy
   2:        0x10b0a7893 - failure::on_fail::hc5ca3b9ac2e803f33NF
   3:        0x10b074b58 - rt::unwind::begin_unwind_inner::h6a7a87805596eb8avvF
   4:        0x109eefc57 - rt::unwind::begin_unwind::h17906050615791405580
   5:        0x10a4ca604 - dom::bindings::cell::DOMRefCell<T>::borrow::h4190820532651473826
   6:        0x10a4ca306 - dom::bindings::cell::DOMRefCell<T>.JSTraceable::trace::h4310069758099585448
   7:        0x10a3c547e - page::Page...dom..bindings..trace..JSTraceable::trace::h34094fa2e51a9b61nMC
   8:        0x10a3c5230 - dom::bindings::trace::Rc<T>.JSTraceable::trace::h10571701282402279690
   9:        0x10a249be0 - dom::window::Window...dom..bindings..trace..JSTraceable::trace::hb18f653be69f3d17BNy
  10:        0x10a249a2f - dom::bindings::codegen::Bindings::WindowBinding::_trace::__rust_abi
  11:        0x10a2499c2 - dom::bindings::codegen::Bindings::WindowBinding::_trace::h39c07b65037cebcfgqR
  12:        0x10a7d7217 - _ZN2js8GCMarker19processMarkStackTopERNS_11SliceBudgetE
  13:        0x10a7d498b - _ZN2js8GCMarker14drainMarkStackERNS_11SliceBudgetE
  14:        0x10a705831 - _ZL23IncrementalCollectSliceP9JSRuntimexN2js8gcreason6ReasonENS1_18JSGCInvocationKindE
  15:        0x10a704b21 - _ZL7GCCycleP9JSRuntimebxN2js18JSGCInvocationKindENS1_8gcreason6ReasonE
  16:        0x10a702c49 - _ZL7CollectP9JSRuntimebxN2js18JSGCInvocationKindENS1_8gcreason6ReasonE
  17:        0x10a6d9ff5 - _ZN2js14DestroyContextEP9JSContextNS_18DestroyContextModeE
  18:        0x10a6233f5 - rust::Cx.Drop::drop::h503f8f2ff371de25nGc
  19:        0x10a020912 - js..rust..Cx::glue_drop.75599::hbfcf92397bb6bc04
  20:        0x10a0205f7 - rc::Rc<T>.Drop::drop::h15097111596020488334
  21:        0x10a02047e - alloc..rc..Rc<js..rust..Cx>::glue_drop.75596::h18e0f1da96576639
  22:        0x10a13f815 - page..JSPageInfo::glue_drop.78096::h2a8d4d20267b2faf
  23:        0x10a13f7dd - core..option..Option<page..JSPageInfo>::glue_drop.78093::h8a17648a00b3a043
  24:        0x10a521bfc - script_task::shut_down_layout::hc3357913e4075cf9uIE
  25:        0x10a518ad8 - script_task::ScriptTask::handle_exit_pipeline_msg::h1c583daf5bdca9bbg2D
  26:        0x10a4f53be - script_task::ScriptTask::handle_msgs::h91e61256ecf1861d6HD
  27:        0x10a4f3752 - script_task::ScriptTask::start::h518925f16b2ccefcWHD
  28:        0x1099e1681 - script_task::ScriptTask.ScriptTaskFactory::create::closure.10036
  29:        0x1099e0bf2 - task::spawn_named_with_send_on_failure::closure.10023
  30:        0x1099e0afc - thunk::Thunk<(), R>::new::closure.10019
  31:        0x1099e09f7 - thunk::F.Invoke<A, R>::invoke::h1909875558069162853
  32:        0x109916201 - thunk::Thunk<A, R>::invoke::h3460697802619677365
  33:        0x109916f3a - thread::Builder::spawn_inner::closure.5172
  34:        0x109916ebf - rt::unwind::try::try_fn::__rust_abi::h4178472243586226322
  35:        0x109916e5a - rt::unwind::try::try_fn::h4178472243586226322
  36:        0x10b0a9279 - rust_try_inner
  37:        0x10b0a9266 - rust_try
  38:        0x10991634c - rt::unwind::try::h4866605895635158878
  39:        0x10991508f - thread::Builder::spawn_inner::closure.5024
  40:        0x109917f79 - thunk::Thunk<(), R>::new::closure.5196
  41:        0x109917e61 - thunk::F.Invoke<A, R>::invoke::h14132929894331977433
  42:        0x10b095353 - sys::thread::thread_start::h8d91e5be653c10c05CB
  43:     0x7fff8e0db772 - _pthread_start

[aweinstock314]

My current understanding of this is that

• RefCell defers borrow-checking to runtime
• For objects managed by the GC, both the primary owner of the object and the GC need to access the object
• When the GC is triggered while the object is mutably borrowed, the GC attempts a second mutable borrow, causing RefCell to panic
• The suggested fix is to have the GC use RefCell::as_unsafe_cell to bypass the borrow-checking, which is reasoned to be safe in this instance (unsure exactly why)

Is this understanding approximately correct?

[jdm]

Yes! Although the GC doesn't do a second mutable borrow right now, it just tries to immutably borrow the value again, which fails if there is an outstanding mutable borrow. This situation is safe because the code is running on the same thread, so there won't be any data race.