Closed Bug 1465709 Opened 2 years ago Closed 2 years ago

Hook rust OOM handler on rustc 1.28

Categories

(Toolkit :: Crash Reporting, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(2 files)

Bug 1458161 added a rust OOM handler based on an unstable API that was removed in 1.27, replaced with something that didn't allow to get the failed allocation size.

Latest 1.28 nightly (2018-05-31) has https://github.com/rust-lang/rust/pull/50880 merged, which allows to hook the OOM handler and get the failed allocation size again.
Comment on attachment 8982149 [details]
Bug 1465709 - Hook rust OOM handler on rustc 1.28.

https://reviewboard.mozilla.org/r/248172/#review254416

Seems a bit odd for the API to change that much?
Attachment #8982149 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #3)
> Comment on attachment 8982149 [details]
> Bug 1465709 - Hook rust OOM handler on rustc 1.28.
> 
> https://reviewboard.mozilla.org/r/248172/#review254416
> 
> Seems a bit odd for the API to change that much?

OOM handling was moved off GlobalAlloc before its stabilization.
I'm going to hold on before landing this because the oom api might still change in 1.28.
Still holding on to it because the api is going to move behind its own feature().
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/5182bca90d06
Hook rust OOM handler on rustc 1.28. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/5182bca90d06
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Blocks: 1469766
I was looking at this patch just now because it broke SearchFox for some reason (presumably rust needs to be updated in SearchFox, though I don't know why it would break a week later), and it looks to me like part of this patch might be wrong?

+elif not CONFIG['MOZ_AUTOMATION']:
+    # We don't want builds on automation to unwillingly stop annotating OOM
+    # crash reports from rust.
+    error('Builds on automation must use a version of rust that supports OOM '
+          'hooking')

Isn't the "not" erroneous? It sounds like you want this to produce an error when MOZ_AUTOMATION is set. Maybe I'm just confused. (Also, I feel like "unwillingly" should be "unwittingly"...)
Flags: needinfo?(mh+mozilla)
Depends on: 1469971
No longer depends on: 1469971
(In reply to Andrew McCreight [:mccr8] from comment #11)
> Isn't the "not" erroneous? 

It is. Thanks.
Flags: needinfo?(mh+mozilla)
Approval Request Comment
[Feature/Bug causing the regression]: Regression from this bug
[User impact if declined]: Unintended build failure with rustc 1.27 or >= 1.29 for downstreams.
[Is this code covered by automated tests?]: N/A
[Has the fix been verified in Nightly?]: N/A
[Needs manual test from QE? If yes, steps to reproduce]: N/A
[List of other uplifts needed for the feature/fix]: bug 1469766
[Is the change risky?]: No
[Why is the change risky/not risky?]: It's a moz.build change only, that changes the condition of a configure error.
[String changes made/needed]: N/A
Attachment #8986600 - Flags: approval-mozilla-beta?
Comment on attachment 8986600 [details] [diff] [review]
Fixup moz.build error logic, landed as d81bb2c26824

IIUC, this bug only affects 62, which is still being kept in sync between m-c and m-b until the version bump on Monday. So no need to uplift :)
Attachment #8986600 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Depends on: 1470363
Duplicate of this bug: 1470363
You need to log in before you can comment on or make changes to this bug.