Closed
Bug 1465709
Opened 7 years ago
Closed 7 years ago
Hook rust OOM handler on rustc 1.28
Categories
(Toolkit :: Crash Reporting, defect)
Toolkit
Crash Reporting
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(2 files)
59 bytes,
text/x-review-board-request
|
froydnj
:
review+
|
Details |
1.26 KB,
patch
|
RyanVM
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
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•7 years 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•7 years 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•7 years 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•7 years ago
|
||
Still holding on to it because the api is going to move behind its own feature().
Comment hidden (mozreview-request) |
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•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Comment 11•7 years ago
|
||
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)
Comment 12•7 years ago
|
||
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d81bb2c26824
Fixup moz.build error logic from bug 1465709. r=me
Assignee | ||
Comment 13•7 years 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•7 years 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 15•7 years ago
|
||
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-
Comment 16•7 years ago
|
||
bugherder |
You need to log in
before you can comment on or make changes to this bug.
Description
•