Assertion failure: fallibleScope_ ([OOM] Cannot allocate a new chunk in an infallible scope.), at js/src/ds/LifoAlloc.cpp:105

RESOLVED FIXED in Firefox 57

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 months ago
2 months ago

People

(Reporter: bc, Assigned: nbp)

Tracking

(Blocks: 1 bug, {assertion})

56 Branch
mozilla57
assertion
Points:
---

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox55 wontfix, firefox56 wontfix, firefox57 fixed)

Details

(URL)

Attachments

(2 attachments)

(Reporter)

Description

2 months ago
Created attachment 8906303 [details]
Linux crash report

1. http://iogames.space/snowfight-io/
   wait for the game to complete loading or reload.

2. Assertion failure: fallibleScope_ ([OOM] Cannot allocate a new chunk in an infallible scope.), at /mozilla/builds/nightly/mozilla/js/src/ds/LifoAlloc.cpp:105

Beta 56, Nightly 57 on Fedora 26 with 16G. I didn't get close to OOM.

The attached log is from a run on 2017-08-22. Bughunter didn't reproduce this today so I don't have a current crash report. Testing locally  didn't give me a stack in the log.
(Assignee)

Comment 1

2 months ago
I will investigate, but I suspect this might be a duplicate of Bug 1398105.

This assertion exists on debug builds and is not really an OOM, it corresponds to an allocation site which relies too much on the infallible allocator.  On release builds, if the allocation site is not checked, then it will likely fail later with a nullptr SEGV.
(Assignee)

Comment 2

2 months ago
Created attachment 8906595 [details] [diff] [review]
Use a fallible allocator for LoopAliasInfo allocations.

Based on the crash report, this is the only allocation in this loop which
can cause such issue.

Strangely enough, we would expect the vectors to reserve enough ballast
space for this one to never trigger, but it might if we have purely
math-oriented code with tons of loops.
Attachment #8906595 - Flags: review?(tcampbell)
(Assignee)

Updated

2 months ago
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Comment on attachment 8906595 [details] [diff] [review]
Use a fallible allocator for LoopAliasInfo allocations.

Review of attachment 8906595 [details] [diff] [review]:
-----------------------------------------------------------------

It also surprises me that this fails, but it is an allocation in a loop under user-controlled bounds so should be fixed anyways.
Attachment #8906595 - Flags: review?(tcampbell) → review+

Comment 4

2 months ago
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/061de9b749ef
Use a fallible allocator for LoopAliasInfo allocations. r=tcampbell
https://hg.mozilla.org/mozilla-central/rev/061de9b749ef
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Do you think this is worth considering for uplift to 56 since it's such a small patch and affects real-world websites? Or should it just ride the 57 train?
status-firefox55: --- → wontfix
status-firefox56: --- → affected
status-firefox-esr52: --- → wontfix
Flags: needinfo?(nicolas.b.pierron)
(Assignee)

Comment 7

2 months ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #6)
> Do you think this is worth considering for uplift to 56 since it's such a
> small patch and affects real-world websites? Or should it just ride the 57
> train?

As this assertion triggered on a real website, I will request an up-lift. Usually these assertions are found by fuzzers and never reported on any website, and we do not consider them for up-lift.

Thanks for raising the question.
Flags: needinfo?(nicolas.b.pierron)
(Assignee)

Comment 8

2 months ago
Comment on attachment 8906595 [details] [diff] [review]
Use a fallible allocator for LoopAliasInfo allocations.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 937540 (?)
[User impact if declined]: Tiny crashing rate.
[Is this code covered by automated tests?]: No, because it is hard to reproduce.
[Has the fix been verified in Nightly?]: No, only guesses based on the stack trace that this would be the only crash reason possible.  Note, all crashes with this assertion have a common pattern, i-e an unbounded loop with an infallible allocation.  Which is exactly what is fixed by this patch.
[Needs manual test from QE? If yes, steps to reproduce]: No, no step to reproduce the issue.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Replace an infallible allocation (crashing on allocation failures), by a fallible allocation which report the allocation failure (as already done by other branches in the same function).
[String changes made/needed]: None.
Attachment #8906595 - Flags: approval-mozilla-beta?
(Assignee)

Comment 9

2 months ago
(In reply to Nicolas B. Pierron [:nbp] from comment #1)
> This assertion exists on debug builds and is not really an OOM, it
> corresponds to an allocation site which relies too much on the infallible
> allocator.  On release builds, if the allocation site is not checked, then
> it will likely fail later with a nullptr SEGV.

Correcting my-self, on release build a crash would only happen on OOM.
The OOM is controlled by AutoEnterOOMUnsafeRegion [1] class, which will cause a crash to be reported [2].

This is no longer a nullptr SEGV as mentioned before.

[1] http://searchfox.org/mozilla-central/rev/e76c0fee79a34a3612740b33508276f4b37c3ee8/js/src/ds/LifoAlloc.h#293
[2] no results reported with this crash reason over the last 6 months: https://crash-stats.mozilla.com/search/?moz_crash_reason=%3D%5Bunhandlable%20oom%5D%20LifoAlloc%3A%3AallocInfallible&date=%3E%3D2017-03-14T10%3A15%3A43.000Z&date=%3C2017-09-14T10%3A15%3A43.000Z&page=1&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform
Comment on attachment 8906595 [details] [diff] [review]
Use a fallible allocator for LoopAliasInfo allocations.

56 is on m-r now.
Attachment #8906595 - Flags: approval-mozilla-beta? → approval-mozilla-release?
If this isn't a high crash rate, I'd like this to wait and ride with 57 since we're building the 56 release candidate now.

If the point is that this crash might never get reported correctly and might be widespread please n-i to me to explain or find me on irc and we can reconsider. Thanks
Flags: needinfo?(nicolas.b.pierron)
Attachment #8906595 - Flags: approval-mozilla-release? → approval-mozilla-release-
(Assignee)

Comment 12

2 months ago
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #11)
> If this isn't a high crash rate, I'd like this to wait and ride with 57
> since we're building the 56 release candidate now.

This is fine for me.

> If the point is that this crash might never get reported correctly and might
> be widespread please n-i to me to explain or find me on irc and we can
> reconsider. Thanks

Crash-stat should report these OOM correctly. There is just no volume in the last 6 months.
Our debug builds are deliberately changed to be picky about these assertions to ensure the correctness of the allocator usage.
Flags: needinfo?(nicolas.b.pierron)
status-firefox56: affected → wontfix
You need to log in before you can comment on or make changes to this bug.