1583047 - Crash in [@ core::ptr::real_drop_in_place | dogear::store::Store::merge_with_driver]

Status: RESOLVED DUPLICATE of bug 1635013

(Firefox :: Sync, defect, P3)

Description

[Julian Seward [:jseward]]

This bug is for crash report bp-69baf1df-335f-4dbd-aee7-e87640190919.

This crash appeared in two separate instances of the OSX nightly of 20190919094654.

Top 10 frames of crashing thread:

0 XUL core::ptr::real_drop_in_place src/libcore/ptr/mod.rs:197
1 XUL dogear::store::Store::merge_with_driver third_party/rust/dogear/src/store.rs:75
2 XUL <bookmark_sync::merger::MergeTask as moz_task::Task>::run toolkit/components/places/bookmark_sync/src/merger.rs:205
3 XUL moz_task::TaskRunnable::allocate::Run xpcom/rust/moz_task/src/lib.rs:83
4 XUL nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1225
5 XUL NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:486
6 XUL mozilla::ipc::MessagePumpForNonMainThreads::Run ipc/glue/MessagePump.cpp:333
7 XUL nsThread::ThreadFunc xpcom/threads/nsThread.cpp:458
8 libnss3.dylib _pt_root nsprpub/pr/src/pthreads/ptthread.c:198
9 libsystem_pthread.dylib _pthread_body 

Comment 1

[Lina Butler (ex-Mozilla)]

Hmmm, I don't have any ideas. Nathan, can you think of why this would be panicking? I'm not even sure what this means, except "it looks really bad".

Some general observations, in no particular order:

• The signature core::option::expect_failed | core::ptr::real_drop_in_place<T> | moz_task::{{impl}}::allocate::Release looks vaguely related—both Dogear and rkv use moz_task for dispatching runnables to background threads. And the assertion that signature trips is pretty scary, too. All other signatures mentioning real_drop_in_place point to Servo.
• Store only holds references, not objects themselves, and it's created and destroyed on the background thread. (Within Driver, AbortController is a pure Rust object, Conn wraps a mozStorageConnection, which uses thread-safe refcounting, and Driver holds ThreadPtrHandles to main-thread objects, so their Release calls get proxied).
• But, if moz_task is buggy, are we releasing those objects incorrectly?

moz_task works by implementing an nsIRunnable that's dispatched to a target thread (and runs a "task", which is Send + Sync), then re-dispatching the runnable back to the original thread and notifying the task that it's done. Could reusing the same runnable be problematic here?

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Lina Cambridge (she/her) [:lina] from comment #1)

Hmmm, I don't have any ideas. Nathan, can you think of why this would be panicking? I'm not even sure what this means, except "it looks really bad".

Some general observations, in no particular order:

• The signature core::option::expect_failed | core::ptr::real_drop_in_place<T> | moz_task::{{impl}}::allocate::Release looks vaguely related—both Dogear and rkv use moz_task for dispatching runnables to background threads. And the assertion that signature trips is pretty scary, too. All other signatures mentioning real_drop_in_place point to Servo.

It's not obvious to me where this signature is coming from, but releases on the wrong thread, particularly with runnables, generally follow this pattern:

• A runnable R holds a RefPtr to some (non-threadsafe) nsIThing
• Allocate an R on some thread T1 (where nsIThing implementations cannot be released)
• Dispatch R to some thread T2 (where nsIThing implementations can be released)
• T1 is still holding a reference to R, but gets suspended
• T2 runs R, drops its reference to R
• T1 gets re-scheduled, drops its reference to R
• R's members get destroyed on T1
• Member RefPtr to nsIThing releases the last reference on T1
• Whoops

But from your description, it doesn't sound like we're dealing with any non-threadsafe nsIThing implementations?

moz_task works by implementing an nsIRunnable that's dispatched to a target thread (and runs a "task", which is Send + Sync), then re-dispatching the runnable back to the original thread and notifying the task that it's done. Could reusing the same runnable be problematic here?

I could see this causing problems, depending on what the task holds. I can't really tell where ThreadBounfRefPtr gets declared as Send + Sync, but this comment suggests that it magically happens?

https://searchfox.org/mozilla-central/source/xpcom/rust/xpcom/src/refptr.rs#122-128

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 4

[Nathan Froyd [:froydnj]]

Since bug 1605052 is in Firefox::Sync, moving this one there too.

Comment 5

[Lina Butler (ex-Mozilla)]

Duping this forward to bug 1635013; there's more discussion there.

Comment 6

[Victor Porof [:vporof][:vp]]

I don't have access to that bug, Lina can you CC me?