(not currently active) Ted Mielczarek Reporter
	Description • 9 years ago

bug 1163831 and bug 1171437 are going to change the way Socorro generates signatures by collapsing template parameters (so foo<a::b>(x<a>, y<c::d>) -> foo<T>(x<T>, y<T>)) and removing function parameters (so foo<T>(x<T>, y<T>) -> foo<T>).

We should change the crash signature fields in existing bugs to match. We might want to preserve the signatures we have and just append the modified ones since Socorro isn't changing the signature of existing crashes (AFAIK).

	Robert Kaiser
	Comment 1 • 9 years ago

If so, we should only do that on open bugs. I had planned to update anything in topcrash lists manually. If we do anything automatically, we should make sure to not add the stripped signatures where they already are on (I did it on a few bugs already in preparation).

	Robert Kaiser
	Comment 2 • 9 years ago

Ted, are you planning on writing up something automated for this, or do you know if someone else does? I would otherwise do that manually and only for topcrasher or otherwise "important" bugs.

	(not currently active) Ted Mielczarek Reporter
	Comment 3 • 9 years ago

No, I was hoping to coerce someone else to do it.

	:glob ✱ Assignee
	Comment 4 • 9 years ago

be aware we'll have to write a script to do this, so this will take at least a week to get around to.

it isn't clear where "T" comes from in your first example:
> foo<a::b>(x<a>, y<c::d>) -> foo<T>(x<T>, y<T>))

can you explain how to derive T here?  some real-life examples would be helpful too.

> We might want to preserve the signatures we have and just append the
> modified ones since Socorro isn't changing the signature of existing crashes (AFAIK).

if i understand you correctly wouldn't it be weird to have each signature duplicated, but only one with a working link?  wouldn't it be better to have socorro support both forms when searching?

in any event, "we might want to" isn't a definite statement of requirements .. let us know when you've decided if you want duplicate signatures or not.

	Robert Kaiser
	Comment 5 • 9 years ago

(In reply to Byron Jones ‹:glob› from comment #4)
> can you explain how to derive T here?  some real-life examples would be
> helpful too.

We always replace anything within < and > with "T" (short for "template").

> if i understand you correctly wouldn't it be weird to have each signature
> duplicated, but only one with a working link?

No, we do that in many cases already. And the old link will actually continue to work for a while. And making Socorro to a non-exact match there would be a bit weird and complicated on their side. The clean way to do this is to add the new variants to the bugs.

> in any event, "we might want to" isn't a definite statement of requirements
> .. let us know when you've decided if you want duplicate signatures or not.

I said that because I do not want duplicate signatures at all as they would be confusing, but I didn't know how easy it is to respect this.

	(not currently active) Ted Mielczarek Reporter
	Comment 6 • 9 years ago

I'm not sure if lars can provide a standalone snippet for transforming a signature given the new rules, that would be useful so you don't have to reinvent that wheel (and possibly miss some corner cases).

I'd defer to Kairo on the other point, it sounds like we should try to transform any existing signatures, and if we get something different, append that to the signature list so that crashes processed both before and after the switch can be found.

	K Lars Lohn [:lars] [:klohn]
	Comment 7 • 9 years ago

here is the code for a command line tool that will take an old style signature and make a new style one from it.  https://gist.github.com/twobraids/3bd2ea5e127b6f64d7d8

	Robert Kaiser
	Comment 8 • 9 years ago

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #6)
> I'd defer to Kairo on the other point, it sounds like we should try to
> transform any existing signatures, and if we get something different, append
> that to the signature list so that crashes processed both before and after
> the switch can be found.

Exactly.

	Robert Kaiser
	Comment 9 • 9 years ago

We'd like to get to applying the signature generation changes soon, would it be feasible to do this automated update on the Bugzilla side and how long would it take?

	:glob ✱ Assignee
	Comment 10 • 9 years ago

sorry for the delays in responding here .. the gist in comment 7 looks promising but it appears to rely on the socorro.processor.signature_utilities library.  to save me a treasure hunt, are you able to link directly to the code that performs the translation between the styles?

	Robert Kaiser
	Comment 11 • 9 years ago

Lars, can you help glob there?

	K Lars Lohn [:lars] [:klohn]
	Comment 12 • 9 years ago

(In reply to Byron Jones ‹:glob› from comment #10)

> ...  to save me a treasure hunt,
> are you able to link directly to the code that performs the translation
> between the styles?

Here is a link to the location within the socorro processor code that can generate the new form of the signature: The  method "_collapse" in the base class of the CSignatureTool class hierarchy is responsible for both C++ template and argument collapsing.  https://github.com/mozilla/socorro/blob/master/socorro/processor/signature_utilities.py#L110

I'll gladly coordinate and assist in any way I can to help with this process.  On Aug31/Sep01 I'm on call for jury duty, but still may be available for consultation.

	:glob ✱ Assignee
	Comment 13 • 9 years ago

perl port of signature collapsing.  need to wrap it into a script that actually updates open bugs.

results (first line input, 2nd line output):

f3(s,t,u)
f3

::(anonymous namespace)::f3(s,t,u)
::(anonymous namespace)::f3

operator()(s,t,u)
operator()

Alpha<Bravo<Charlie>, Delta>::Echo<Foxtrot>
Alpha<T>::Echo<T>

f<3>(s,t,u)
f<T>

	:glob ✱ Assignee
	Comment 14 • 9 years ago

here's some read-world examples:

[@ nsBaseHashtable<nsCStringHashKey, nsAutoPtr<mozilla::net::CacheEntryTable>, mozilla::net::CacheEntryTable*>::EnumerateRead(PLDHashOperator (*)(nsACString_internal const&, mozilla::net::CacheEntryTable*, void*), void*)]
[@ nsBaseHashtable<T>::EnumerateRead]

[@ js::jit::GetPropertyIC::update(JSContext*, JS::Handle<JSScript*>, unsigned int, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>)]
[@ js::jit::GetPropertyIC::update]

[@ OOM | large | mozalloc_abort(char const* const) | mozalloc_handle_oom(unsigned int) | moz_xmalloc | jArray<wchar_t, int>::newJArray(int) ]
[@ OOM | large | mozalloc_abort | mozalloc_handle_oom | moz_xmalloc | jArray<T>::newJArray ]

kairo - do these look right to you?

	Robert Kaiser
	Comment 15 • 9 years ago

Thanks, those look good to me.

For all Crash Signatures fields that are non-empty and can be parsed into one of more [@…] blocks, we should run this for every one of those blocks and add the output of this to the existing entries in the Crash Signatures field, if it's not in there yet.

	:glob ✱ Assignee
	Comment 16 • 9 years ago

kairo - another batch for your feedback:

[@ nsBaseHashtable<nsCStringHashKey, nsAutoPtr<mozilla::net::CacheEntryTable>, mozilla::net::CacheEntryTable*>::EnumerateRead(PLDHashOperator (*)(nsACString_internal const&, mozilla::net::CacheEntryTable*, void*), void*)]
[@ nsBaseHashtable<nsCStringHashKey, nsAutoPtr<mozilla::net::CacheEntryTable>, mozilla::net::CacheEntryTable*>::EnumerateRead(PLDHashOperator (*)(nsACString_internal const&, mozilla::net::CacheEntryTable*, void*), void*)] [@ nsBaseHashtable<T>::EnumerateRead]

[@ js::jit::GetPropertyIC::update(JSContext*, JS::Handle<JSScript*>, unsigned int, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>)]
[@ js::jit::GetPropertyIC::update(JSContext*, JS::Handle<JSScript*>, unsigned int, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>)] [@ js::jit::GetPropertyIC::update]

[@ OOM | large | mozalloc_abort(char const* const) | mozalloc_handle_oom(unsigned int) | moz_xmalloc | jArray<wchar_t, int>::newJArray(int)]
[@ OOM | large | mozalloc_abort(char const* const) | mozalloc_handle_oom(unsigned int) | moz_xmalloc | jArray<wchar_t, int>::newJArray(int)] [@ OOM | large | mozalloc_abort | mozalloc_handle_oom | moz_xmalloc | jArray<T>::newJArray]

[@ mozilla::image::SurfaceCacheImpl::LookupBestMatch(mozilla::image::Image* const, mozilla::image::SurfaceKey const&, mozilla::Maybe<unsigned int> const&)] [@ mozilla::image::SurfaceCacheImpl::LookupBestMatch(mozilla::image::Image* const , mozilla::image::SurfaceKey const&, mozilla::Maybe<T> const&)]
[@ mozilla::image::SurfaceCacheImpl::LookupBestMatch(mozilla::image::Image* const, mozilla::image::SurfaceKey const&, mozilla::Maybe<unsigned int> const&)] [@ mozilla::image::SurfaceCacheImpl::LookupBestMatch(mozilla::image::Image* const , mozilla::image::SurfaceKey const&, mozilla::Maybe<T> const&)] [@ mozilla::image::SurfaceCacheImpl::LookupBestMatch]

[@ nsThread::ProcessNextEvent(bool, bool*) | NS_ProcessNextEvent(nsIThread*, bool) | nsXMLHttpRequest::Send(nsIVariant*, mozilla::dom::Nullable<nsXMLHttpRequest::RequestBody> const&)] [@ nsThread::ProcessNextEvent(bool , bool*) | NS_ProcessNextEvent(nsIThread*, bool) | nsXMLHttpRequest::Send(nsIVariant*, mozilla::dom::Nullable<T> const&)] [@ nsThread::ProcessNextEvent | NS_ProcessNextEvent | nsXMLHttpRequest::Send]
[@ nsThread::ProcessNextEvent(bool, bool*) | NS_ProcessNextEvent(nsIThread*, bool) | nsXMLHttpRequest::Send(nsIVariant*, mozilla::dom::Nullable<nsXMLHttpRequest::RequestBody> const&)] [@ nsThread::ProcessNextEvent(bool , bool*) | NS_ProcessNextEvent(nsIThread*, bool) | nsXMLHttpRequest::Send(nsIVariant*, mozilla::dom::Nullable<T> const&)] [@ nsThread::ProcessNextEvent | NS_ProcessNextEvent | nsXMLHttpRequest::Send]

	Robert Kaiser
	Comment 17 • 9 years ago

Yes, that looks good, thanks!

	Robert Kaiser
	Comment 18 • 9 years ago

What does it do for the following cases?

bug 1141089:

[@ mozilla::ContainerState::InvalidateForLayerChange(nsDisplayItem*, mozilla::layers::PaintedLayer*) ]
[@ mozilla::ContainerState::ProcessDisplayItems(nsDisplayList*) ]

(should add short versions of both)


bug 1158189:

[@ shutdownhang | WaitForSingleObjectEx | WaitForSingleObject | PR_Wait | nsThread::ProcessNextEvent(bool, bool*) | NS_ProcessNextEvent(nsIThread*, bool) | mozilla::net::nsHttpConnectionMgr::Shutdown()]
[@ shutdownhang | WaitForSingleObjectEx | WaitForSingleObject | PR_Wait | mozilla::ReentrantMonitor::Wait(unsigned int) | nsThread::ProcessNextEvent(bool, bool*) | NS_ProcessNextEvent(nsIThread*, bool) | mozilla::net::nsHttpConnectionMgr::Shutdown()]
[@ shutdownhang | WaitForSingleObjectEx | PR_Wait | mozilla::ReentrantMonitor::Wait(unsigned int) | nsThread::ProcessNextEvent(bool, bool*) | NS_ProcessNextEvent(nsIThread*, bool) | mozilla::net::nsHttpConnectionMgr::Shutdown()]
[@ shutdownhang | WaitForSingleObjectEx | WaitForSingleObject | PR_Wait | nsThread::ProcessNextEvent | NS_ProcessNextEvent | mozilla::net::nsHttpConnectionMgr::Shutdown]
[@ shutdownhang | ntdll.dll@0x3c6bc]

(should add short versions of the two variants that are not in there yet)


Note that in most cases, for readability, we are putting line breaks between the [@…] blocks, so let's 1) makes sure it deals with that and 2) do that for those additions as well (sorry that it breaks the format you used for putting the samples in here).

	:glob ✱ Assignee
	Comment 19 • 9 years ago

> [@ mozilla::ContainerState::InvalidateForLayerChange(nsDisplayItem*,
> mozilla::layers::PaintedLayer*) ]
> [@ mozilla::ContainerState::ProcessDisplayItems(nsDisplayList*) ]

[@ mozilla::ContainerState::InvalidateForLayerChange(nsDisplayItem*, mozilla::layers::PaintedLayer*) ]
[@ mozilla::ContainerState::ProcessDisplayItems(nsDisplayList*) ]
[@ mozilla::ContainerState::InvalidateForLayerChange ]
[@ mozilla::ContainerState::ProcessDisplayItems ]

> [@ shutdownhang | WaitForSingleObjectEx | WaitForSingleObject | PR_Wait |
> nsThread::ProcessNextEvent(bool, bool*) | NS_ProcessNextEvent(nsIThread*,
> bool) | mozilla::net::nsHttpConnectionMgr::Shutdown()]
> [@ shutdownhang | WaitForSingleObjectEx | WaitForSingleObject | PR_Wait |
> mozilla::ReentrantMonitor::Wait(unsigned int) |
> nsThread::ProcessNextEvent(bool, bool*) | NS_ProcessNextEvent(nsIThread*,
> bool) | mozilla::net::nsHttpConnectionMgr::Shutdown()]
> [@ shutdownhang | WaitForSingleObjectEx | PR_Wait |
> mozilla::ReentrantMonitor::Wait(unsigned int) |
> nsThread::ProcessNextEvent(bool, bool*) | NS_ProcessNextEvent(nsIThread*,
> bool) | mozilla::net::nsHttpConnectionMgr::Shutdown()]
> [@ shutdownhang | WaitForSingleObjectEx | WaitForSingleObject | PR_Wait |
> nsThread::ProcessNextEvent | NS_ProcessNextEvent |
> mozilla::net::nsHttpConnectionMgr::Shutdown]
> [@ shutdownhang | ntdll.dll@0x3c6bc]

[@ shutdownhang | WaitForSingleObjectEx | WaitForSingleObject | PR_Wait | nsThread::ProcessNextEvent(bool, bool*) | NS_ProcessNextEvent(nsIThread*, bool) | mozilla::net::nsHttpConnectionMgr::Shutdown()]
[@ shutdownhang | WaitForSingleObjectEx | WaitForSingleObject | PR_Wait | mozilla::ReentrantMonitor::Wait(unsigned int) | nsThread::ProcessNextEvent(bool, bool*) | NS_ProcessNextEvent(nsIThread*, bool) | mozilla::net::nsHttpConnectionMgr::Shutdown()]
[@ shutdownhang | WaitForSingleObjectEx | PR_Wait | mozilla::ReentrantMonitor::Wait(unsigned int) | nsThread::ProcessNextEvent(bool, bool*) | NS_ProcessNextEvent(nsIThread*, bool) | mozilla::net::nsHttpConnectionMgr::Shutdown()]
[@ shutdownhang | WaitForSingleObjectEx | WaitForSingleObject | PR_Wait | nsThread::ProcessNextEvent | NS_ProcessNextEvent | mozilla::net::nsHttpConnectionMgr::Shutdown]
[@ shutdownhang | ntdll.dll@0x3c6bc]
[@ shutdownhang | WaitForSingleObjectEx | WaitForSingleObject | PR_Wait | mozilla::ReentrantMonitor::Wait | nsThread::ProcessNextEvent | NS_ProcessNextEvent | mozilla::net::nsHttpConnectionMgr::Shutdown]
[@ shutdownhang | WaitForSingleObjectEx | PR_Wait | mozilla::ReentrantMonitor::Wait | nsThread::ProcessNextEvent | NS_ProcessNextEvent | mozilla::net::nsHttpConnectionMgr::Shutdown]

	Robert Kaiser
	Comment 20 • 9 years ago

Great, that's what I'd expect! \o/

	K Lars Lohn [:lars] [:klohn]
	Comment 21 • 9 years ago

:glob, great job on a faithful and complete translation of my original Python code into Perl

	:glob ✱ Assignee
	Comment 22 • 9 years ago

parsing free-form text is always interesting.

	David Lawrence [:dkl]
	Comment 23 • 9 years ago

Comment on attachment 8667794 [details] [diff] [review]
1178094_1.patch

Review of attachment 8667794 [details] [diff] [review]:
-----------------------------------------------------------------

Overall worked except for a couple times it ran out of memory executing. I was able to run again from where it left off. If you could execute in batches it would help.
Also, I noticed issues when running the script more than once. Some bugs were getting updated again every time.

Example Bug 1152026:

Run1

ChildFinder::NoteJSObject(JSObject*)

to

ChildFinder::NoteJSObject(JSObject*)
ChildFinder::NoteJSObject 

Run2

ChildFinder::NoteJSObject(JSObject*)
ChildFinder::NoteJSObject 

to

ChildFinder::NoteJSObject(JSObject*)
ChildFinder::NoteJSObject
ChildFinder::NoteJSObject ChildFinder::NoteJSObject 

Run3

ChildFinder::NoteJSObject(JSObject*)
ChildFinder::NoteJSObject
ChildFinder::NoteJSObject ChildFinder::NoteJSObject 

to

ChildFinder::NoteJSObject(JSObject*)
ChildFinder::NoteJSObject
ChildFinder::NoteJSObject ChildFinder::NoteJSObject
ChildFinder::NoteJSObject ChildFinder::NoteJSObject ChildFinder::NoteJSObject ChildFinder::NoteJSObject

and so on...

Other changes where there are @[ ] look fine.

	:glob ✱ Assignee
	Comment 24 • 9 years ago

- don't create a bug object unless we need to update the bug
- process in batches of 100
- improve handling of malformed signatures

	David Lawrence [:dkl]
	Comment 25 • 9 years ago

Comment on attachment 8670659 [details] [diff] [review]
1178094_2.patch

Review of attachment 8670659 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl

	:glob ✱ Assignee
	Comment 26 • 9 years ago

To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   c0de3b6..0c703a9  master -> master

i'll execute the script once it's pushed to production (early next week).

	:glob ✱ Assignee
	Comment 27 • 9 years ago

done!  2044 bugs updated.

	:glob ✱ Assignee
	Comment 28 • 9 years ago

bugs updated: http://mzl.la/1L9k2iE

	Robert Kaiser
	Comment 29 • 9 years ago

Awesome, thanks! I created a post at https://home.kairo.at/blog/2015-10/shortening_crash_signatures_dropping_arg about these changes and also sent that message to dev-platform, dev-quality and stability@m.o - we will flip the switch on Socorro probably tomorrow.