1565033 - Add memory checking to the out-of-process crash generator

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, enhancement)

Description

[Gabriele Svelto [:gsvelto]]

A significant number of crashes we receive are caused by faulty hardware with broken memory being a common cause of heap corruption. If we could add a simple form of user-space memory testing (à la memtester) to the exception handler we might be able to identify faulty machines and flag crash reports accordingly, inform the user or both.

This bug is meant to track the implementation of the checking itself.

Comment 1

[Worcester12345]

Thunderbird slows everything down to a crawl before this crashes. It will sometimes give a "script error". Disk thrashing is also happening at this time. It did not do this with earlier versions, or not as bad at least. Version 68 may have started this, but it is much worse now. One particular email with many links will crash it every time.

Comment 2

[Gabriele Svelto [:gsvelto]]

I'm changing the title to reflect the fact that we'll move all crash generation out-of-process so what we could do is the following: take the minidump as usual and report it to the main process, use the crash generation process to map the memory of the crashed process, scan it with a memtest algorithm and flag the crash reports if we find bad bits.

Comment 3

[Gabriele Svelto [:gsvelto]]

There's a user-space memory tester called memtester which we could take inspiration from (or maybe reuse?).

Comment 4

[Gian-Carlo Pascutto [:gcp]]

This might also be of interest: https://memtest.org/readme#memory-testing-philosophy

Comment 5

[Paul Bone [:pbone]]

I was speaking with @gcp and suggested that it could be a good idea to make the badram scanning a separate library so that it can be used both by the crash reporter and by some future in-process bad ram scanning. I filed bug 1897102 before I remembered this bug. I think it's still good to have separate bugs for the library and then the integration in the crash reporter.

Comment 6

[Brian Tsoi [:btsoi]]

Attachment: Bug 1565033: Add memory testing to crash reporter client.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/33b07d5e29b4
Add memory testing to crash reporter client. r=afranchuk,fluent-reviewers,supply-chain-reviewers,cmartin,flod,gsvelto

Comment 8

[Brian Tsoi [:btsoi]]

Attachment: Request for data collection review form

Comment 9

[Will Kahn-Greene [:willkg] ET needinfo? me]

Comment on attachment 9464424 [details]
Request for data collection review form

Hi! I'm no longer a Data Steward. Please find someone else to do the data review.

Comment 10

[Brian Tsoi [:btsoi]]

I realized I misunderstood the documentation and the data review should be done before the patch is landed.
Still the feature is completely switched off (controlled by a pref) in the landed patch, and I intend to switch it on later with another patch, so no data is currently being collected.

Comment 11

[Serban Stanca [:SerbanS]]

Backed out for causing build bustages.

• Backout link
• Push with failures
• Failure Log

Comment 12

[Brian Tsoi [:btsoi]]

Backed out due to Rust version difference. memtest crate currently doesn't work with Rust 1.76. I thought we have Rust 1.82 now but the version bump got backed out.
I could consider rewriting the parts of the crate that don't compile with 1.76 and release a new version. For now I'll wait for the data review first.

Comment 13

[Chris H-C :chutten|PTO (back Oct 23)]

Comment on attachment 9464424 [details]
Request for data collection review form

Actually, there's a new Data Review process in town. Starting May 7, 2024, all data collections in projects reviewed in Phabricator also perform data review in Phabricator: https://wiki.mozilla.org/Data_Collection#Firefox_Desktop.2C_Firefox_and_Focus_for_Android.2C_Gecko_.28from_May_7.2C_2024.29

In short, following what the Herald message says, code authors and code reviewers together determine what level of review is needed for new or expanded data collections. You might never need to fill out a data review request form again! (For instrumentation in projects reviewed in Phabricator, anyway)

And I see you've done this already with data-classification-low, so you're good to go as far as I can tell?

Comment 14

[Brian Tsoi [:btsoi]]

Ah in that case yes we're good to go.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b2a854b53890
Add memory testing to crash reporter client. r=afranchuk,fluent-reviewers,supply-chain-reviewers,cmartin,flod,gsvelto

Comment 16

[Serban Stanca [:SerbanS]]

Backed out together with Bug 1620998.

• Backout link

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ff7173030c9e
Add memory testing to crash reporter client. r=afranchuk,fluent-reviewers,supply-chain-reviewers,cmartin,flod,gsvelto

Comment 18

[Mayank Bansal]

This patch Increased the installer size by 43KB on Windows. I guess some increase is expected here. The increase is below any reportable thresholds . This comment is only a FYI.

Comment 19

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/ff7173030c9e