Closed
Bug 1163831
Opened 10 years ago
Closed 10 years ago
Collapse all <...> in crash signatures to <T>
Categories
(Socorro :: General, task)
Socorro
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: away, Unassigned)
References
Details
The full type of IntRect, in the debug symbols, is:
mozilla::gfx::BaseRect<int,mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits>,mozilla::gfx::IntPointTyped<mozilla::gfx::UnknownUnits>,mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits>,mozilla::gfx::IntMarginTyped<mozilla::gfx::UnknownUnits> >
As with any data structure rename, some spurious "new" signatures spiked after bug 1158122. But in this case the new ones are so long that they get cut off. For example: bp-96522516-106f-449f-90f7-4a2182150509 is in IntRect::IsEmpty, but you'd never know that from the signature. I can't even tell whether they are all IsEmpty, or if there are multiple crash sites getting put in the same truncated bucket.
What can we do about this?
Comment 1•10 years ago
|
||
I more and more start to believe that we should change signature generation somewhat. IMHO, we probably (in classic signatures generated from a C/C++ stack) should strip off everything in normal or angle brackets.
Stripping bracketed content could also help reduce the COMDAT-folding explosion we see in cases like nsCOMPtr<Fifty_random_signatures_that_all_mean_the_same_crash>.
I am not yet sure whether we want to do this for all bracketed text or just based on a list. Perhaps we could add a "rewrite list" in addition to the skip-list and ignore-list. For starters we might do IntRect/nsCOMPtr/nsTArray.
On the other hand, I think I'm willing to try your idea first (just delete it all) and see whether it causes any problems.
Comment 3•10 years ago
|
||
I'd really like to see if signatures would be better to handle if we just strip all pieces in rounded or angle brackets. In theory, it was a good idea to have them, but signatures get very long, and even the rounded brackets (argument lists of functions) create more difficulty than they help with, as it's frequent enough that types get changed or arguments added/removed while the crashes remain the same. I think I never really saw it helping in terms of pointing to a specific overloaded function.
Comment 4•10 years ago
|
||
We already do a small form of this, in bug 481445 we decided to replace integer template parameters with "int". I think replacing all template parameters with T in signatures would probably be alright. As dmajor points out in comment 2 we often get the wrong function name anyway, since the identical function bodies have been COMDAT folded together, so nsCOMPtr<T>::foo() would actually be an improvement in many cases. For cases where it's not, presumably we could still leave the original function name as provided by Breakpad in the stack so that it'd be clearer what's going on.
On the Socorro side this fix would go into CSignatureToolBase:
https://github.com/mozilla/socorro/blob/47b804ed6b1bd1efa399d151e4cc02ac0d094fad/socorro/processor/signature_utilities.py#L67
Comment 5•10 years ago
|
||
It looks like David, Ted and I agree that it would be good to collapse all signature pieces between < and > into <T> - lars, can we get that implemented?
Flags: needinfo?(lars)
Summary: gfx::IntRect produces horrible crash signatures → Collapse all <...> in crash signatures to <T>
As a test case, Alpha<Bravo<Charlie>, Delta>::Echo<Foxtrot> should become Alpha<T>::Echo<T>
Comment 7•10 years ago
|
||
we currently have a normalization rule that applies to templates with integer parameters:
f<3>(x, y, z) --> f<int>(x, y, z)
does this new rule supersede this older template rule?
f<3>(x, y, z) --> f<T>(x, y, z)
Flags: needinfo?(lars)
Comment 9•10 years ago
|
||
Commit pushed to master at https://github.com/mozilla/socorro
https://github.com/mozilla/socorro/commit/f1b5fcd223aba6b774c1698f67e4fa7ed96a3cd3
Merge pull request #2832 from twobraids/collapse_templates_in_signatures
fixes Bug 1163831 - collapse templates in signatures
Updated•10 years ago
|
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 10•9 years ago
|
||
This change should be announced on dev-platform so that we can avoid confusion
like in bug 1167557 where we thought the crash had magically stopped occurring.
I suspect we have many other bugs on file that are affected so everyone needs
to be aware of this so we don't close bugs that still occurs.
Comment 11•9 years ago
|
||
BTW, would it be possible to update Bugzilla signatures that has template args
with and *additional* signature that collapses the args to "T" to catch new
crashes with the new signature scheme?
Comment 12•9 years ago
|
||
Oh wow. Thanks! I will mention this on mailing lists and in the channel meeting.
Reporter | ||
Comment 13•9 years ago
|
||
While we're at it we should mention that bug 1171437 is coming soon as well.
Comment 14•9 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #10)
> This change should be announced on dev-platform
It will be but there is another even larger change to signatures coming in the next 2 weeks and I wanted to talk about both in the same email but had very little time before my vacation and Whistler.
Comment 15•9 years ago
|
||
(In reply to David Major [:dmajor] from comment #13)
> While we're at it we should mention that bug 1171437 is coming soon as well.
That's exactly my plan, things just got short in the week before I left for vacation and Whistler.
(In reply to Mats Palmgren (:mats) from comment #11)
> BTW, would it be possible to update Bugzilla signatures that has template
> args
> with and *additional* signature that collapses the args to "T" to catch new
> crashes with the new signature scheme?
Feel free to write up something for it and endure all the bugmail annoyance that comes with it. I will only update bugs that are currently interesting and will do so only once we activated bug 1171437 as well.
Comment 16•9 years ago
|
||
I don't see why you can't announce these changes right now even if not all of them
are deployed yet. We almost closed bug 1167557 as WFM b/c we were completely
unaware of this change. I think you need to notify people *before* massive changes
like this.
Comment 17•9 years ago
|
||
Because I needed some fricking vacation and you are getting me back into the stressed, grumpy situation I was in before I had some and pushing me into not having any relaxing effects of it left. Thanks for that.
Comment 18•9 years ago
|
||
All that said, now that I cooled down a bit and am not stuck in travel stress, thanks for the pointer and we will do better in communication for future changes that will affect a larger amount of signatures.
You need to log in
before you can comment on or make changes to this bug.
Description
•