520678 - SkipList Add "nsTArray_base::ShiftData(unsigned int, unsigned int, unsigned int, unsigned int)" and "PL_DHashTableOperate" to ignore-frame-list

Status: VERIFIED FIXED

(Socorro :: General, task)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Crashes that happen on nsTArray_base::ShiftData is probably due to calls on destroyed arrays, so the bug is likely one level up the callstack. So it would be great if we could add this function to the list of functions that we prepend using the [foo | bar | baz] syntax.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Lars: Are you the right assignee for this one?

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Can we also add PL_DHashTableOperate.

Comment 3

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

We need to come up with a more efficient way to make these configuration changes.  All I can do is turn around and make a request to IT for the configuration change.  If you could direct the request first to IT, cc'ng me on the request, I could rubber stamp it and it would get into production more quickly.

A request to IT needs to specify that it is a "configuration change request for Socorro processor" - it needs to go on to say that a certain signature needs to be added to the 'prefixSignatureRegEx' configuration parameter.

It is important to give the exact signature when making the request. "nsTArray_base::ShiftData" doesn't appear to be correct.  Did you really mean, "nsTArray_base::ShiftData(unsigned int, unsigned int, unsigned int, unsigned int)"?

After writing all that I realize that I'll probably be stuck with writing these configuration requests forever... I've written Bug 521916 for this issue

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I agree it'd be great if we could make this smoother. Not sure I followed the rest of your comment though, would you like me to reassign this to someone in IT (and update to the exact signature in the subject)? If so, who?

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(Updating summary since that needs to happen no matter who does this)

Comment 6

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

You need not take any additional action.  

By writing out what it takes to make a properly worded IT request, I realized that it was too complicated for anyone not versed in the Socorro internals.  So, I drew a deep breath and submitted the request myself.

You should continue to make these types of requests through me.

Comment 7

[chris hofmann]

lars, any update?  I also posted a comment in Bug 519771 that I'm interested in thoughts on.   maybe this collective informantion is just some kind of future meta data that we try and add on top through some extra analysis path.

Comment 8

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

This task was completed with an IT request in Bug 521916.  If there is any reason to reopen this bug, please feel free to do so.

Comment 9

[Stephen Donner [:stephend] Not actively reading bugmail]

Jonas, mind taking verification on this?  Thanks!

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

nsTArray_base::ShiftData(unsigned int, unsigned int, unsigned int, unsigned int) still does not appear to be on the skip list.

http://crash-stats.mozilla.com/topcrasher/byversion/Firefox/3.5.5/7

lists "memmove | nsTArray_base::ShiftData(unsigned int, unsigned int, unsigned int, unsigned int)" as one of the crashers (currently as #29)

Comment 11

[Frank Griswold [:griswolf] [:fgriswold]]

We have an error in our 'prefix' regex for nsTArray_base::ShiftData(unsigned int, unsigned int, unsigned int, unsigned int), so it was not being recognized. I'll try to get that fixed today.

Comment 12

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

is this working as expected now?  I always see "nsTArray_base::ShiftData..." as a prefix component of compound signatures when querying for the last few days.  I'm closing it again.  Reopen if necessary.

"nsTArray_base::ShiftData(unsigned int, unsigned int, unsigned int, unsigned int) | nsTArray<unsigned char>::~nsTArray<unsigned char>()"

"memmove | nsTArray_base::ShiftData(unsigned int, unsigned int, unsigned int, unsigned int) | nsObserverList::FillObserverArray(nsCOMArray<nsIObserver>&)"

Comment 13

[Wayne Mery (:wsmwk)]

(In reply to comment #12)
> is this working as expected now?  

Jonas, et al, 
I amm also wondering if this working correctly. Shouldn't socorro be giving a different signature for bp-18083e26-1154-43db-8ef9-54cd82100806 ??

Frank, Lars, can you tell us exactly what skiplist string was used. Note: I can't see the bugs at https://wiki.mozilla.org/Breakpad/SkipList#Changelog ... so I have no idea what skiplists have actually been implemented in the past year. 

Part of my reason for asking is I may need further differentiation of stacks via Bug 555599. But first I'd like to know what was implemented in this bug is working correctly.

Comment 14

[Wayne Mery (:wsmwk)]

fgriswold, lars, please see comment 11 thru comment 13. the skiplist doesn't seem to be working. 
example 1 bp-18083e26-1154-43db-8ef9-54cd82100806 comment 13 above
example 2 bp-152833f6-faf1-4ad0-8e4b-453272091215 from bug 534858

Comment 15

[Laura Thomson :laura]

Current skiplist config for prefixes:
prefixSignatureRegEx.default = '@0x0|strchr|strstr|strlen|PL_strlen|strcmp|wcslen|memcpy|memmove|memcmp|malloc|realloc|.*free|arena_dalloc_small|arena_alloc|arena_dalloc|nsObjCExceptionLogAbort(\(.*?\)){0,1}|libobjc.A.dylib@0x1568.|nsCOMPtr_base::assign_from_qi(nsQueryInterface,nsIDconst&)|objc_msgSend|_purecall|nsTArray_base::ShiftData(unsigned int, unsigned int, unsigned int, unsigned int)|PL_DHashTableOperate|EtwEventEnabled|RtlpFreeHandleForAtom|RtlpDeCommitFreeBlock|RtlpAllocateAffinityIndex|RtlAddAccessAllowedAce|RtlQueryPerformanceFrequency|RtlpWaitOnCriticalSection|RtlpWaitForCriticalSection|_PR_MD_ATOMIC_(INC|DEC)REMENT|nsCOMPtr.*|nsRefPtr.*|operator new\([^,)]+\)|CFRelease'

Comment 16

[Wayne Mery (:wsmwk)]

Thanks Laura. Do the parenthesis need to be escaped in the string?
 nsTArray_base::ShiftData(unsigned int, unsigned int, unsigned int, unsigned int)
as in 
 nsTArray_base::ShiftData\(unsigned int, unsigned int, unsigned int, unsigned int\)


I'm unsure, because 

1. It's clear nsTArray_base::ShiftData... isn't being skipped, per http://crash-stats.mozilla.com/query/query?product=Firefox&version=ALL%3AALL&range_value=2&range_unit=days&date=08%2F26%2F2010+15%3A06%3A58&query_search=signature&query_type=contains&query=nsTArray_base%3A%3AShiftData%28unsigned+int%2C+unsigned+int%2C+unsigned+int%2C+unsigned+int%29&build_id=&process_type=any&hang_type=any&do_query=1 

2. But nsCOMPtr_base::assign_from_qi(nsQueryInterface,nsIDconst&) is working and has no escaped parenthesis.  per http://crash-stats.mozilla.com/query/query?product=Thunderbird&version=ALL%3AALL&range_value=2&range_unit=days&date=08%2F26%2F2010+14%3A58%3A39&query_search=signature&query_type=contains&query=nsCOMPtr_base%3A%3Aassign_from_qi&build_id=&process_type=any&hang_type=any&do_query=1

Comment 17

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

all literal parens within a skiplist rule should be escaped.  Since the rules are simply regular expressions and parens have their own meaning within regular expressions, the backslash is mandatory to match a literal '(' or ')'.  

The reason that the second signature in comment #16 is working is because it is matched by the rule 'nsCOMPtr.*'

Comment 18

[Laura Thomson :laura]

This is no longer a problem in prod config - I don't know when it changed, but these no longer need escaping (expressions no longer contain parens).  Resolving.

Comment 19

[Matt Brandt [:mbrandt]]

(In reply to Laura Thomson :laura from comment #18)
> This is no longer a problem in prod config - I don't know when it changed,
> but these no longer need escaping (expressions no longer contain parens). 
> Resolving.

Bumping to QA verified. I'm seeing the same behavior as :laura.