Prefix before mozilla::detail::MutexImpl::mutexLock should be stripped
Categories
(Socorro :: Signature, task, P2)
Tracking
(Not tracked)
People
(Reporter: sg, Assigned: willkg)
References
Details
Attachments
(2 files)
Prefixes before mozilla::detail::MutexImpl::mutexLock
are platform-specific and do not add information. If they were stripped, it would be much easier to identify cross-platform issues. This is particularly striking on Linux, where the prefix is not symbolized: https://crash-stats.mozilla.org/search/?signature=%40libc.%2AmutexLock.%2A&date=%3E%3D2020-10-21T09%3A06%3A00.000Z&date=%3C2021-01-21T09%3A06%3A00.000Z&_facets=signature&_sort=-date&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature
But it would also make sense to strip the prefixes on Windows (RtlAcquireSRWLockExclusive
) and OS X (pthread_mutex_lock
, pthread_mutex_trylock
or __pthread_mutex_lock
).
Assignee | ||
Comment 1•4 years ago
|
||
I'm not sure what you mean by "strip". That's not a word we use in signature generation algorithm and there are a few different things that result in a signature that doesn't have that symbol.
Documentation for signature generation is here:
https://socorro.readthedocs.io/en/latest/signaturegeneration.html
with the particular bits I think are pertinent here being here:
https://socorro.readthedocs.io/en/latest/signaturegeneration.html#signature-generation-algorithm
We could make that symbol a sentinel. If we did that, then the signature would start with symbols after the mozilla::detail::MutexImpl::mutexLock
symbol. We do that with some of the panic symbols since we want to skip all the panic machinery.
Here's an example:
- before:
libc.so@0xb0318 | mozilla::detail::MutexImpl::mutexLock | mozilla::layers::CompositorBridgeParent::GetGeckoContentControllerForRoot
- after:
mozilla::layers::CompositorBridgeParent::GetGeckoContentControllerForRoot
We could add that symbol to the irrelevant list. If we did that, then the symbol wouldn't show up in the signature at all.
- before:
libc.so@0xb0318 | mozilla::detail::MutexImpl::mutexLock | mozilla::layers::CompositorBridgeParent::GetGeckoContentControllerForRoot
- after:
libc.so@0xb0318 | mozilla::layers::CompositorBridgeParent::GetGeckoContentControllerForRoot
You mention the libc.so@0xb0318
symbol being problematic, so I suspect you want to make mozilla::detail::MutexImpl::mutexLock
a sentinel.
Does that sound like what you're looking for?
Reporter | ||
Comment 2•4 years ago
|
||
Sorry for my bad terminology! Originally, I was thinking of:
- before:
libc.so@0xb0318 | mozilla::detail::MutexImpl::mutexLock | mozilla::layers::CompositorBridgeParent::GetGeckoContentControllerForRoot
- after:
mozilla::detail::MutexImpl::mutexLock | mozilla::layers::CompositorBridgeParent::GetGeckoContentControllerForRoot
or (theoretically, I don't think we have exactly this case) - before:
libc.so@0xb0318 | trunc | mozilla::detail::MutexImpl::mutexLock | mozilla::layers::CompositorBridgeParent::GetGeckoContentControllerForRoot
- after:
mozilla::detail::MutexImpl::mutexLock | mozilla::layers::CompositorBridgeParent::GetGeckoContentControllerForRoot
and therefore remove/strip everything before mozilla::detail::MutexImpl::mutexLock
.
However, removing anything up to and including mozilla::detail::MutexImpl::mutexLock
seems to be fine as well. In particular, since this might well get inlined in some build variants.
So, yes, making mozilla::detail::MutexImpl::mutexLock
a sentinel sounds good!
Assignee | ||
Comment 3•4 years ago
|
||
Cool! No worries about terms--I just want to make sure we're talking about the same thing.
Grabbing this to work on today.
Assignee | ||
Comment 4•4 years ago
|
||
Simon: mozilla::detail::MutexImpl::mutexLock
is in the prefix list, so it gets included in the signature. If I add that symbol to the sentinels list, then we get this:
app@socorro:/app$ socorro-cmd signature 846be766-c658-44e3-b843-feedf0210121
Crash id: 846be766-c658-44e3-b843-feedf0210121
Original: libc.so@0x90c70 | mozilla::detail::MutexImpl::mutexLock | mozilla::MozPromise<T>::ThenInternal | mozilla::MozPromise<T>::ThenValue<T>::DoResolveOrRejectInternal
New: mozilla::detail::MutexImpl::mutexLock | mozilla::MozPromise<T>::ThenInternal | mozilla::MozPromise<T>::ThenValue<T>::DoResolveOrRejectInternal
Same?: False
I can also remove mozilla::detail::MutexImpl::mutexLock
from the prefix list and then it won't show up in the signature, if you want.
Which do you prefer?
Reporter | ||
Comment 5•4 years ago
|
||
Then let it stay in the prefix list. That actually matches my original idea then.
Depending on the situation, either way might be better, but by letting it on the prefix list, the signatures can still be distinguished where relevant, and it's now comparatively easy to check two signatures for cases where the two signatures actually cover the same case.
Assignee | ||
Comment 6•4 years ago
|
||
Assignee | ||
Comment 7•4 years ago
|
||
Assignee | ||
Comment 8•4 years ago
|
||
This was pushed to production in bug #1688681.
Simon: Want me to reprocess these crash reports?
Reporter | ||
Comment 9•4 years ago
|
||
Given that we could get a better overview of the crashes of Bug 1687597 then, reprocessing would be great!
Assignee | ||
Comment 10•4 years ago
|
||
I reprocessed them going back 6 months. Marking this as FIXED.
Reporter | ||
Comment 11•4 years ago
|
||
Hm, that does not seem to have worked. I still see signatures such as https://crash-stats.mozilla.org/signature/?signature=RtlAcquireSRWLockExclusive%20%7C%20mozilla%3A%3Adetail%3A%3AMutexImpl%3A%3Alock%20%7C%20mozilla%3A%3Adom%3A%3AWorkerPrivate%3A%3AResetWorkerPrivateInWorkerThread&date=%3E%3D2020-10-28T12%3A14%3A00.000Z&date=%3C2021-01-28T12%3A14%3A00.000Z&_sort=-date
Or am I misunderstanding something here?
Assignee | ||
Comment 12•4 years ago
|
||
This bug was for adding mozilla::detail::MutexImpl::mutexLock
and the signature you're asking about has mozilla::detail::MutexImpl::lock
.
I can add mozilla::detail::MutexImpl::lock
as well. Does that help?
Reporter | ||
Comment 13•4 years ago
|
||
Oh, my fault, I didn't see this "subtle" difference in the signatures.
I now checked all signatures containing mozilla::detail::MutexImpl::
. In the posix implementation, mozilla::detail::MutexImpl::lock
just calls mozilla::detail::MutexImpl::mutexLock
, and either of those therefore gets always inlined (or almost always?). In the Windows implementation however, there is no mozilla::detail::MutexImpl::mutexLock
.
The same signature stuff also applies to mozilla::detail::MutexImpl::unlock
(there's no mozilla::detail::MutexImpl::mutexUnlock
).
It could also apply to mozilla::detail::MutexImpl::tryLock
and mozilla::detail::MutexImpl::mutexTryLock
, but there are very few crashes for that.
If it's not much of an additional effort, it would be good to do the same for:
mozilla::detail::MutexImpl::lock
mozilla::detail::MutexImpl::unlock
mozilla::detail::MutexImpl::tryLock
mozilla::detail::MutexImpl::mutexTryLock
If it's extra effort, the last two might be left out for now.
Sorry again for not looking more closely at this from the beginning.
Assignee | ||
Comment 14•4 years ago
|
||
Signature generation is an exercise in tinkering, so it's totally fine to go back and forth over iterations. Signature generation is also pretty easy to tinker with. It's not much effort to try some more variations. So, it's all good! :)
I'll reopen this and tinker with comment #13 ideas either today or Monday.
Assignee | ||
Comment 15•4 years ago
•
|
||
Example crash reports:
mozilla::detail::MutexImpl::lock
: bp-c3d49021-9800-4179-8aa2-da3b80210202mozilla::detail::MutexImpl::unlock
: bp-ed1edbbb-fe64-46e7-94a6-dfe2c0210202mozilla::detail::MutexImpl::tryLock
: bp-5fc5d064-fae8-4147-a54c-b79360210128mozilla::detail::MutexImpl::mutexTryLock
: bp-8192b329-b002-40df-8c90-a55010210118
Assignee | ||
Comment 16•4 years ago
|
||
Assignee | ||
Comment 17•4 years ago
|
||
Assignee | ||
Comment 18•4 years ago
|
||
I deployed this earlier today as part of bug #1690378. I reprocessed 222,000 or so crash reports that had mozilla::detail::MutexImpl
in the signature.
Marking this as FIXED.
Description
•