ScriptTask::mouse_over_targets is not traced

[jdm]

That's kind of bad, and if we had the lint that picked up Vec<JS<T>> we would have caught this before now.

[JIoJIaJIu]

I want to take it, may you describe more, please.

[jdm]

We need to make a few changes:

• just as all DOM objects are traced by the GC via the #[jstraceable] attribute, the ScriptTask needs to be as well
• we need to add an extra GC hook that specifically traces the ScriptTask object (see https://github.com/servo/servo/pull/4620/files#diff-eeefd668773bcf179c9bdcf09d495b27R409 for an example of setting a hook)
• that means we need to store a pointer to ScriptTask in thread-local storage, so we can retrieve it from the hook. See https://github.com/servo/servo/pull/4620/files#diff-7fa3011afc74ee7695cb180db1da7791R393 for a similar example of this; our thread_local object should be Option<*const ScriptTask> instead.

[JIoJIaJIu]

thank you, i'll do it

[JIoJIaJIu]

Is'it right?
I tried don't copy implementation from https://github.com/servo/servo/pull/4620/files#diff-7fa3011afc74ee7695cb180db1da7791R303 for more easily merge and review
How i understand, right way to store just script_task.mouse_over_targets as future RootedVec and don't root all script_target ?
Also i can't look any trace with RUST_LOG=debug ./march servo, could you explain at what time GC should call extern function?
Should i test it someway?

[jdm]

Hmm, I suppose it might be possible to use a RootedVec in the future, but I would feel more comfortable if all of ScriptTask was guaranteed to be traced. Instead of a separate struct and rooting collections, I would prefer to store an Option<*const ScriptTask> in thread-local storage, and have the extern function just access that and call the trace method on it (this would require having ScriptTask use the jstraceable attribute, which implements the JSTraceable trait automatically). This would mean that we would not have to change the mouse_over_targets field at all.

As for testing it, there should definitely be a final GC that occurs at shutdown, but you can also force one by adding window.gc() in a script block in an HTML page. You could add a println to the extern function to be sure it's being called.

[JIoJIaJIu]

Look it, please, like that?
How should i implement removing from SCRIPT_TASK_ROOTS? Implement Drop trait for ScriptTask?
It will be called automatically when references will end?

[jdm]

Yes, adding the removal to the destructor would be great. Also there are a couple issues with the latest code:

• there's only ever at most one ScriptTask for the thread, so we don't need a HashSet
• storing the pointer to the ScriptTask in ScriptTask::new doesn't work, since we move the value when returning and the stored pointer is no longer valid

[JIoJIaJIu]

when i tried window.gc(), i got

thread 'ScriptTask' panicked at 'DOMRefCell<T> already mutably borrowed', /home/jiojiajiu/workspace/opensource/servo/components/script/dom/bindings/cell.rs:125
stack backtrace:
   1:     0x7fc1ab136f90 - sys::backtrace::write::ha4a9415e174db438Rsy
   2:     0x7fc1ab14bf70 - failure::on_fail::hac5db0a0a6a39fd6LWF
   3:     0x7fc1ab11b380 - rt::unwind::begin_unwind_inner::h6e98ac6f3dbe1e23iBF
   4:     0x7fc1a9e14780 - rt::unwind::begin_unwind::h7826163975826071217
   5:     0x7fc1aa4260e0 - dom::bindings::cell::DOMRefCell<T>::borrow::h18387299031793431724
   6:     0x7fc1aa426030 - dom::bindings::cell::DOMRefCell<T>.JSTraceable::trace::h1428091579037181142
   7:     0x7fc1aa418400 - script_task::ScriptTask...dom..bindings..trace..JSTraceable::trace::he65a819eed4069ebvuG
   8:     0x7fc1aa417d20 - script_task::trace_script_tasks::closure.89239
   9:     0x7fc1aa417800 - thread_local::Key<T>::with::h12470583157952388181
  10:     0x7fc1aa4177a0 - script_task::trace_script_tasks::__rust_abi

The problem in implementation trace for fields?

 192     /// A handle to the information pertaining to page layout                                                                                                                                
 193     page: DOMRefCell<Rc<Page>>,   

The script task has some DOMRefCell struct, what is the right workflow and tool for debug? gdb?

[jdm]

That was fixed two hours ago by #5131 :)