Hook rust OOM handler on rustc 1.28

RESOLVED FIXED in Firefox 62

Status

()

defect
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

11 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

11 months ago
mozreview-review
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+
(Assignee)

Comment 4

11 months ago
(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.
(Assignee)

Comment 5

11 months ago
I'm going to hold on before landing this because the oom api might still change in 1.28.
Comment hidden (mozreview-request)
(Assignee)

Comment 7

11 months ago
Still holding on to it because the api is going to move behind its own feature().
Comment hidden (mozreview-request)

Comment 9

10 months ago
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/5182bca90d06
Hook rust OOM handler on rustc 1.28. r=froydnj

Comment 10

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5182bca90d06
Status: NEW → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
(Assignee)

Updated

10 months ago
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
(Assignee)

Comment 13

10 months ago
(In reply to Andrew McCreight [:mccr8] from comment #11)
> Isn't the "not" erroneous? 

It is. Thanks.
Flags: needinfo?(mh+mozilla)
(Assignee)

Comment 14

10 months ago
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.