Add support for skipping a dll in the signature

RESOLVED FIXED

Status

task
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jrmuizel, Assigned: adrian)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Reporter

Description

3 years ago
For example we'd like both of these reports to have the same signature:
https://crash-stats.mozilla.com/report/index/2f6372d4-299e-474b-a596-626ef2160513
https://crash-stats.mozilla.com/report/index/67d0b018-db42-4732-b43c-6fb8f2160513

I think the ideal signature for these reports would be:
'igdumdim32.dll | CreateDecodeDeviceLH'
Assignee

Comment 1

3 years ago
It is not possible with our current algorithm to do what you're asking. However, we could skip the `igdumdim32.dll` frames entirely, and have the signature of the first crash report become `CreateDecodeDeviceLH`. Would that work for you? 

For the second report you linked to, the first frame is @0x0 which is something I think we want to keep in signatures. So what I suggested won't work, the signature would stay the same. We could either skip `@0x0` in signatures, and then it would work as the first report, or we would have to change our algorithm to be able to remove frames after we have started accumulating signatures (note, our algorithm is explained here: https://github.com/mozilla/socorro/tree/master/socorro/siglists#signature-generation-algorithm ). 

But I'm wondering, isn't the actual problem that we have no symbols for that file?
Reporter

Comment 2

3 years ago
(In reply to Adrian Gaudebert [:adrian] from comment #1)
> It is not possible with our current algorithm to do what you're asking.
> However, we could skip the `igdumdim32.dll` frames entirely, and have the
> signature of the first crash report become `CreateDecodeDeviceLH`. Would
> that work for you? 

Nope, we need the igdumdim32.dll in the signature still.

> For the second report you linked to, the first frame is @0x0 which is
> something I think we want to keep in signatures. So what I suggested won't
> work, the signature would stay the same. We could either skip `@0x0` in
> signatures, and then it would work as the first report, or we would have to
> change our algorithm to be able to remove frames after we have started
> accumulating signatures (note, our algorithm is explained here:
> https://github.com/mozilla/socorro/tree/master/socorro/siglists#signature-
> generation-algorithm ). 
> 
> But I'm wondering, isn't the actual problem that we have no symbols for that
> file?

We don't have access to symbols for that file (and graphics drivers in general), so we need something that will work without symbols.
Assignee

Comment 3

3 years ago
OK, so doing that will require changing our algorithm. I'm thinking we could merge all signatures missing symbols (so things like *.dll*) into just one fragment of the final signature. That means for your two examples above you would get ``igdumdim32.dll | CreateDecodeDeviceLH`` and ``@0x0 | igdumdim32.dll | CreateDecodeDeviceLH`` as signatures. 

Proposal: 

In our signature generation algorithm, merge all consecutive frames with no symbols and the same module name into just one frame using that module name. 

Example:

Signature for this crash: https://crash-stats.mozilla.com/report/index/67d0b018-db42-4732-b43c-6fb8f2160513 would become: ``@0x0 | igdumdim32.dll`` (or ``@0x0 | igdumdim32.dll | CreateDecodeDeviceLH`` if we add ``igdumdim32.dll`` to our prefix list). 

Should that apply to all cases, or just a defined list of modules like ``igdumdim32.dll``?
Reporter

Comment 4

3 years ago
(In reply to Adrian Gaudebert [:adrian] from comment #3)
> OK, so doing that will require changing our algorithm. I'm thinking we could
> merge all signatures missing symbols (so things like *.dll*) into just one
> fragment of the final signature. That means for your two examples above you
> would get ``igdumdim32.dll | CreateDecodeDeviceLH`` and ``@0x0 |
> igdumdim32.dll | CreateDecodeDeviceLH`` as signatures. 

That sounds good to me.


> Should that apply to all cases, or just a defined list of modules like
> ``igdumdim32.dll``?

I think it would be good to start with a defined list of modules. If it works well we can expand it to everything.
Assignee

Comment 5

3 years ago
Benjamin, what do you think about that plan? Anything I should be cautious about?
Flags: needinfo?(benjamin)
Assignee

Updated

3 years ago
Assignee: nobody → adrian

Comment 6

3 years ago
If we're talking about the plan from comment 3, I think that makes sense, but we should prototype and test carefully before deploying (and notify dev-platform).

I expect this will work best for all modules without symbols, but you're welcome to do the safe whitelist approach first.
Flags: needinfo?(benjamin)
Reporter

Comment 7

3 years ago
Any idea approximately when we can expect this change?
Flags: needinfo?(adrian)
Here's an example where this would have made a big difference - bug 1277626.  Instead of a crash with 454 reports, and an unrelated crash of 203 reports, we'd see a single crash with a total of 1665 reports.  That's have escalated it to our attention a lot sooner.
Assignee

Comment 9

3 years ago
Starting to work on this now. Hopefully this can land in our staging site early next week.
Status: NEW → ASSIGNED
Flags: needinfo?(adrian)

Comment 11

3 years ago
Commit pushed to master at https://github.com/mozilla/socorro

https://github.com/mozilla/socorro/commit/f1b5a0aeed3c596a324076912ffa641276313bfd
Fixes bug 1274345 - Added support from triming signatures missing symbols. (#3363)

* Fixes bug 1274345 - Added support from triming signatures missing symbols.

* nits

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Reporter

Comment 13

3 years ago
(In reply to Adrian Gaudebert [:adrian] from comment #12)
> This is landed and working on stage! 
> 
> Should we add `igdumdim32.dll` to the prefix list, so that the next frame
> gets added to the signature?

Yes. I think that will be essential for this to work well.
Reporter

Comment 15

3 years ago
It looks like this is working great on stage.

Can you add atiumdag.dll, atiumd6a.dll, aticfx32.dll, atiuxpag.dll, igd10umd32.dll, igdumd32.dll, atidxx32.dll, atiumdva.dll, nvoglnt.dll, nvwgf2um.dll, nvumdshim.dll, and nvd3dum.dll to the same same lists as igdumdim32.dll
Reporter

Comment 16

3 years ago
and also atiu9pag.dll
Assignee

Comment 17

3 years ago
Jeff, does this look good to you? Note that we have made it a lot easier to make this kind of changes yourself, see the documentation here: https://github.com/mozilla/socorro/tree/master/socorro/siglists
Reporter

Comment 18

3 years ago
Looks good to me.

Comment 19

3 years ago
Commit pushed to master at https://github.com/mozilla/socorro

https://github.com/mozilla/socorro/commit/1a6ec0c5e28bd7606b21232b06ac9e1604ac5eef
Bug 1274345 - Added more DLLs to the siglists. (#3374)

r=willkg,jeff
Reporter

Comment 20

3 years ago
When can we expect this to hit staging and master?
Flags: needinfo?(adrian)
Sadly, I'm very excited about this.

Comment 25

3 years ago
Commits pushed to master at https://github.com/mozilla/socorro

https://github.com/mozilla/socorro/commit/198d9a41ce39dcf9fa24fda9c3cbf9817d6888e7
Revert "Bug 1274345. Add some 64bit dlls to the siglists"

https://github.com/mozilla/socorro/commit/a92272ee56d0385f14d6984c03958b2c3ebb1414
Merge pull request #3386 from mozilla/revert-3383-master

Revert "Bug 1274345. Add some 64bit dlls to the siglists"

Comment 26

3 years ago
Commits pushed to master at https://github.com/mozilla/socorro

https://github.com/mozilla/socorro/commit/44169b69043c285547fe01d9a31b0186ca33acff
Revert "Revert "Bug 1274345. Add some 64bit dlls to the siglists""

https://github.com/mozilla/socorro/commit/6cc2a963a3232cee8cfc1e377e17300e90fa3d25
Merge pull request #3387 from mozilla/revert-3386-revert-3383-master

Revert "Revert "Bug 1274345. Add some 64bit dlls to the siglists""
You need to log in before you can comment on or make changes to this bug.