411349 - Better signatures for crashes with just an address as the top frame

Status: RESOLVED FIXED

(Socorro :: General, task)

Description

[Samuel Sidler (old account; do not CC)]

Reported by ted.mielczarek, Jun 05, 2007

https://crash-reports.mozilla.com/reports/report/list?signature=%400x0

Crashes that just have an address at the top of the stack produce
non-useful signatures.  We should improve the signature algorithm to walk
down the stack until it finds a frame that is not just an address
(module@address would be ok).  It can default to the top frame if there are
no non-address frames.

Comment 1

[(not currently active) Ted Mielczarek]

Attachment: proposed fix

Here's a proposed fix. I haven't tested this because I don't have a processor to play with.

Comment 2

[(not currently active) Ted Mielczarek]

Lars: I don't suppose you could test this patch for me? I can give you a minidump with the right kind of stack if you need one.

Comment 3

[Jesse Ruderman]

I think the following items from http://crash-stats.mozilla.com/topcrasher/byversion/Firefox/3.0b5 should get the same treatment.

xpsp2res.dll@0x202020
xpsp2res.dll@0x202113
(These are just crashing @ 0x20202020 or @ 0x20202113, but xpsp2res.dll happens to occupy 0x20000000, so they show up as offsets within the dll.)

nsObjCExceptionLogAbort 
(see bug 422823 comment 15)

nsCOMPtr_base::assign_from_qi(nsQueryInterface, nsID const&)

memcpy
memcmp

strchr
strstr
strcmp

Comment 4

[(not currently active) Ted Mielczarek]

So ideally we would also have a table with a list of signatures that should not wind up as the crash signature, and the processor could just skip all of those. It could just read the entire table periodically and use it as an array, so it shouldn't be a big perf deal.

Comment 5

[Tony Mechelynck [:tonymec]]

See also bug 455257.

Comment 6

[K Lars Lohn [:lars] [:klohn]]

ted's proposed fix for this is severely bit rotted.  I am reworking it.  I will create a system that allows for a list of regular expressions matching forbidden signatures.  I will drop down the stack until I find something that isn't forbidden.

Initially, this list will be in Processor's configuration.  At some later date we may move it into the database so we can allow changes via a WebApp from developers.

Please comment on this bug giving me regular expressions for things that you'd like to see declared as forbidden.

Comment 7

[Jesse Ruderman]

I can't think of anything I want to forbid in addition to the list in comment 3.  Sam/dbaron, can you think of other functions for which it is better to group crashes based on what called them?

Comment 8

[Benjamin Smedberg]

Lars, I think we want two different kinds:

a: skip this frame entirely
b: include this frame in the signature along with the previous frame

regexes for a:

/^@0x../ # (any bare address that isn't 0x0

regexes for b:

/^@0x0/ # bare 0x0 - null-deref crash
/^strchr$/ # what's interesting is not this function, but the caller/thisfunc pair
/^memcpy$/
/^malloc$/
/^realloc$/
/^free$/
/^arena_dalloc_small$/

Thus for http://crash-stats.mozilla.com/report/index/b7de751f-2128-4aa4-9571-4db320081114

the signature would be "free |  	XPCWrappedNative::CallMethod(XPCCallContext&,XPCWrappedNative::CallMode)"

Make sense?

Comment 9

[Jesse Ruderman]

I think I'd stick 0x0 with the other 0xes.  My experience with Firefox debug builds is that a bug that can call (not just dereference, but call) 0x0 can also call other addresses.

Comment 10

[timeless]

there are a number of free variants, like tls_free. i'm pretty sure i could come up w/ some other things i'd want added. i'd probably also want abort and assert to be skipped and the frames near them.

Comment 11

[Samuel Sidler (old account; do not CC)]

(In reply to comment #9)
> I think I'd stick 0x0 with the other 0xes.  My experience with Firefox debug
> builds is that a bug that can call (not just dereference, but call) 0x0 can
> also call other addresses.

I agree with this and comment 8 and I can't think of anything else to add to the list in comment 3 and comment 8.

Comment 12

[K Lars Lohn [:lars] [:klohn]]

This has been implemented in accordance with comment #8.  It awaits staging on the Socorro new_partition branch.  Should staging of that branch be delayed because of our database backup conundrum, I will merge it independently over to trunk for a quicker deployment.

Comment 13

[K Lars Lohn [:lars] [:klohn]]

This change was placed into production along with the new partitioning scheme.  After 12 hours of running a complication was found:

Under the scheme in comment #8 above, bare addresses were to be ignored when devising signatures.  However, in real data coming in, sometimes the crashing thread has nothing but bare addresses in the stack - no proper symbols at all.  This resulted in an empty signature.  

The code was modified to detect this case and fall back to the address on the top of the stack for the signature.