1756505 - Remove the ThreadIdNameMapping annotation in favor of the minidump thread names stream

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, enhancement)

Description

[Gabriele Svelto [:gsvelto]]

The only platforms which still rely on the ThreadIdNameMapping annotation are Linux and Android. On Linux we already have a writer for the minidump thread names stream for child processes as part of the oxidized minidump writer but for the main thread we still rely on Breakpad. We should thus do three things here:

• Add a Breakpad writer for the thread id name mapping minidump stream on Linux (like we did in bug 1658831 for macOS)
• Make sure it works on main process crashes on Linux
• Make sure it works on both main and child process crashes on Android
• Remove the ThreadIdNameMapping annotation and all its associated machinery

Bonus: this will speed up thread creation on Linux and cut per-process memory consumption by a few KiBs.

Comment 1

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1756505 - Implement a writer for the thread names' minidump stream on Linux/Android r=gankra

Comment 2

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1756505 - Remove the ThreadIdNameMapping annotation and all the associated machinery r=gankra

Depends on D139318

Comment 3

[Gabriele Svelto [:gsvelto]]

FYI I tested this both on Linux/x86-64 and Android/ARM and it seems to be working fine everywhere. It'll be a nice improvement for Fenix which has been often missing thread names.

Comment 4

[Gabriele Svelto [:gsvelto]]

I've updated the patch adding a note about this change removing a potential deadlock at crash time which is the whole reason why this is blocking bug 1756069.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d0e31f4a0ee1
Implement a writer for the thread names' minidump stream on Linux/Android r=Gankra
https://hg.mozilla.org/integration/autoland/rev/3622bb57fb67
Remove the ThreadIdNameMapping annotation and all the associated machinery r=Gankra

Comment 6

[Atila Butkovits]

Backed out for causing hazard failures.

Backout link: https://hg.mozilla.org/integration/autoland/rev/23b93818145a47dea75de0627b4949d508e33659

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=Fpwf8gcWQVmFzARslEX4pA.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&searchStr=linux%2Cx64%2Cdebug%2Chazard-linux64-haz%2Fdebug%2Ch&revision=3622bb57fb67c29a7ddf6de5fdda9049af34c9e4

Failure log: https://treeherder.mozilla.org/logviewer?job_id=369859758&repo=autoland&lineNumber=45896

Comment 7

[Gabriele Svelto [:gsvelto]]

This is not a real hazard failure, it's an ICE :(

https://treeherder.mozilla.org/logviewer?job_id=369859758&repo=autoland&lineNumber=45912

Comment 8

[Gabriele Svelto [:gsvelto]]

Steve, I'm hitting an ICE in the GC hazards detector, can you help me sort it out?

Comment 9

[Steve Fink [:sfink] [:s:]]

I'll take a look. Still trying to reproduce locally.

Comment 10

[Steve Fink [:sfink] [:s:]]

I don't see this in the code it's processing, but sixgill is crashing because it thinks it's seeing something like

struct S {
  int header;
  char remainder[0];
};

void f() {
  S s;
  for (auto c : s.remainder) { ... }
}

gcc constructs a RANGE_EXPR node with size_t begin = 0 and ssize_t end = -1 for this. sixgill expected both begin and end to be unsigned.

Comment 11

[Steve Fink [:sfink] [:s:]]

I think it's coming from this type:

typedef struct {
  uint32_t number_of_thread_names;
  MDRawThreadName thread_names[0];
} MDRawThreadNamesList;

More specifically, it looks like it does this when it generates the initialization code for MDRawThreadNamesList while instantiating TypedMDRVA<MDRawThreadNamesList> and compiling its constructor. Or something like that.

Anyway, the code's fine, the hazard analysis just couldn't handle it.

Comment 12

[Gabriele Svelto [:gsvelto]]

Thanks for looking into this!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d303b8d7d95f
Implement a writer for the thread names' minidump stream on Linux/Android r=Gankra
https://hg.mozilla.org/integration/autoland/rev/353c6089fe2a
Remove the ThreadIdNameMapping annotation and all the associated machinery r=Gankra

Comment 14

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/d303b8d7d95f
https://hg.mozilla.org/mozilla-central/rev/353c6089fe2a

Comment 15

[Andrej (:aglavic)]

== Change summary for alert #33567 (as of Thu, 17 Mar 2022 16:57:32 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
2%	Base Content Resident Unique Memory	macosx1015-64-shippable-qr		8,238,421.33 -> 8,069,802.67
2%	Base Content Resident Unique Memory	macosx1015-64-shippable-qr		8,238,933.33 -> 8,072,192.00

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=33567