Closed Bug 1398507 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, defect)

56 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: bc, Assigned: nbp)

References

()

Details

(Keywords: assertion)

Attachments

(2 files)

Attached file 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.
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.
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: 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+
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
Closed: 7 years ago
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?
Flags: needinfo?(nicolas.b.pierron)
(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)
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?
(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-
(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)
You need to log in before you can comment on or make changes to this bug.