Resolve Clippy Warning: Arc not Send and Sync in refcounted.rs

[chickenleaf]

Describe the bug:

Issue: Clippy warns about using Arc with TrustedReference:

warning: usage of an Arc that is not Send and Sync
   --> components/script/dom/bindings/refcounted.rs:269:36
    let refcount = Arc::new(TrustedReference::new(ptr));

Question: Should we implement Send and Sync for TrustedReference, or use a Mutex for thread safety?

Context: Implementing Sync requires safety documentation, needing expert input.

To Reproduce: .\mach cargo-clippy

---

Next Steps:

1. Disable Clippy lint temporarily.
2. Plan for Send/Sync implementation or Mutex usage.
3. Request expert review for best approach.

---

[mrobinson]

Perhaps Rc is enough here.

[mrobinson]

To expand, creating an Arc that isn't Send and Sync means that the Arc itself isn't Send and Sync either. Normally trying to pass an instance of such a struct across a thread boundary would cause an error. It's possible that this type no longer crosses the thread boundary or that it never did. In both of those cases an non-threadsafe Rc should be sufficient. It seems reasonable to try to convert the type to Rc and see if the compiler complains. The only other step would be to see how it is used. Is it used in datastructures that force the implementation of Send and Sync with unsafe code. If so you need to look at bit more carefully.

[chickenleaf]

To expand, creating an Arc that isn't Send and Sync means that the Arc itself isn't Send and Sync either. Normally trying to pass an instance of such a struct across a thread boundary would cause an error. It's possible that this type no longer crosses the thread boundary or that it never did. In both of those cases an non-threadsafe Rc should be sufficient. It seems reasonable to try to convert the type to Rc and see if the compiler complains. The only other step would be to see how it is used. Is it used in datastructures that force the implementation of Send and Sync with unsafe code. If so you need to look at bit more carefully.

So I tried out the Rc change and it is throwing me some 4 errors. I think Arc is needed here? I think the code is being used in a threaded context?

[mrobinson]

So I tried out the Rc change and it is throwing me some 4 errors. I think Arc is needed here? I think the code is being used in a threaded context?

Can you please paste the errors?

[chickenleaf]

So I tried out the Rc change and it is throwing me some 4 errors. I think Arc is needed here? I think the code is being used in a threaded context?

Can you please paste the errors?

err 1:

error[E0308]: mismatched types
   --> components\script\script_runtime.rs:922:36
    |
922 |         ContextForRequestInterrupt(Arc::new(Mutex::new(Some(context))))
    |         -------------------------- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `Rc<Mutex<Option<*mut JSContext>>>`, found `Arc<Mutex<Option<*mut JSContext>>>`
    |         |
    |         arguments to this struct are incorrect
    |
    = note: expected struct `std::rc::Rc<std::sync::Mutex<std::option::Option<*mut js::jsapi::JSContext>>>`
               found struct `std::sync::Arc<std::sync::Mutex<std::option::Option<*mut js::jsapi::JSContext>>>`
note: tuple struct defined here

err -2:

error[E0308]: mismatched types
   --> components\script\dom\bindings\refcounted.rs:179:13
    |
178 |           ) -> (Arc<TrustedReference>, *const LiveDOMReferences) {
    |                ------------------------------------------------- expected `(std::sync::Arc<dom::bindings::refcounted::TrustedReference>, *const dom::bindings::refcounted::LiveDOMReferences)` because of return type
179 | /             LIVE_REFERENCES.with(|r| {
180 | |                 let r = r.borrow();
181 | |                 let live_references = r.as_ref().unwrap();
182 | |                 let refcount = unsafe { live_references.addref(ptr) };
183 | |                 (refcount, live_references as *const _)
184 | |             })
    | |______________^ expected `(Arc<TrustedReference>, *const ...)`, found `(Rc<TrustedReference>, *const _)`
    |
    = note: expected tuple `(std::sync::Arc<dom::bindings::refcounted::TrustedReference>, *const dom::bindings::refcounted::LiveDOMReferences)`
               found tuple `(std::rc::Rc<dom::bindings::refcounted::TrustedReference>, *const _)`

err 3:

error[E0308]: mismatched types
   --> components\script\dom\bindings\refcounted.rs:267:35
    |
256 |     unsafe fn addref(&self, ptr: *const libc::c_void) -> Rc<TrustedReference> {
    |                                                          -------------------- expected `std::rc::Rc<dom::bindings::refcounted::TrustedReference>` because of return type
...
267 |                 Some(refcount) => refcount,
    |                                   ^^^^^^^^ expected `Rc<TrustedReference>`, found `Arc<TrustedReference>`
    |
    = note: expected struct `std::rc::Rc<dom::bindings::refcounted::TrustedReference>`
               found struct `std::sync::Arc<dom::bindings::refcounted::TrustedReference>`

err4:

error[E0308]: mismatched types
    --> components\script\dom\bindings\refcounted.rs:270:34
     |
270  |                     entry.insert(Rc::downgrade(&refcount));
     |                           ------ ^^^^^^^^^^^^^^^^^^^^^^^^ expected `Weak<TrustedReference>`, found a different `Weak<TrustedReference>`
     |                           |
     |                           arguments to this method are incorrect
     |
     = note: expected struct `std::sync::Weak<dom::bindings::refcounted::TrustedReference>`
                found struct `std::rc::Weak<dom::bindings::refcounted::TrustedReference>`

err5:

error[E0308]: mismatched types
    --> components\script\dom\bindings\refcounted.rs:276:30
     |
276  |                 entry.insert(Rc::downgrade(&refcount));
     |                       ------ ^^^^^^^^^^^^^^^^^^^^^^^^ expected `Weak<TrustedReference>`, found a different `Weak<TrustedReference>`
     |                       |
     |                       arguments to this method are incorrect
     |
     = note: expected struct `std::sync::Weak<dom::bindings::refcounted::TrustedReference>`
                found struct `std::rc::Weak<dom::bindings::refcounted::TrustedReference>`
help: the return type of this call is `std::rc::Weak<dom::bindings::refcounted::TrustedReference>` due to the type of the argument passed
    --> components\script\dom\bindings\refcounted.rs:276:17
     |
276  |                 entry.insert(Rc::downgrade(&refcount));
     |                 ^^^^^^^^^^^^^------------------------^
     |                              |
     |                              this argument influences the return type of `insert`

err6:

error[E0308]: mismatched types
   --> components\script\script_runtime.rs:922:36
    |
922 |         ContextForRequestInterrupt(Arc::new(Mutex::new(Some(context))))
    |         -------------------------- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `Rc<Mutex<Option<*mut JSContext>>>`, found `Arc<Mutex<Option<*mut JSContext>>>`
    |         |
    |         arguments to this struct are incorrect
    |
    = note: expected struct `std::rc::Rc<std::sync::Mutex<std::option::Option<*mut js::jsapi::JSContext>>>`
               found struct `std::sync::Arc<std::sync::Mutex<std::option::Option<*mut js::jsapi::JSContext>>>`
note: tuple struct defined here
   --> components\script\script_runtime.rs:918:12
    |
918 | pub struct ContextForRequestInterrupt(Rc<Mutex<Option<*mut RawJSContext>>>);
    |            ^^^^^^^^^^^^^^^^^^^^^^^^^^

[jdm]

You will need to replace every use of Arc in https://github.com/servo/servo/blob/main/components/script/dom/bindings/refcounted.rs with Rc, and replace https://github.com/servo/servo/blob/main/components/script/dom/bindings/refcounted.rs#L31 with use std::rc::Weak;.

[mrobinson]

These errors are due to the fact that other parts of the code were expecting Arc and your change has modified the data structure to use Rc. You'll need to understand each of these errors and determine what kinds of changes are needed to fix them. They do not seem to indicate that anything about thread safety yet.

For instance: ContextForRequestInterrupt(Arc::new(Mutex::new(Some(context)))) should be ContextForRequestInterrupt(Rc::new(Mutex::new(Some(context)))).

[chickenleaf]

alright I shall get into it right away.

[chickenleaf]

I have sent in a PR with the changes recommended. Thank you !!

[sagudev]

We decided to allow lints as clippy warnings are wrong for these cases (this still needs a PR though).