Last Comment Bug 991249 - Make IonBuilder memory ballast contract explicit and testable
: Make IonBuilder memory ballast contract explicit and testable
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla44
Assigned To: Nicolas B. Pierron [:nbp]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 1212389 1243397
Blocks: 912928 988953
  Show dependency treegraph
 
Reported: 2014-04-02 12:29 PDT by Jason Orendorff [:jorendorff]
Modified: 2016-01-28 09:09 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Ensure that we can check for OOMs when we run out of ballast space. (1.14 KB, patch)
2015-10-06 06:57 PDT, Nicolas B. Pierron [:nbp]
jdemooij: feedback+
Details | Diff | Splinter Review
Ensure that we can check for OOMs when we run out of ballast space. (2.20 KB, patch)
2015-10-08 05:22 PDT, Nicolas B. Pierron [:nbp]
jdemooij: review+
Details | Diff | Splinter Review

Description User image Jason Orendorff [:jorendorff] 2014-04-02 12:29:02 PDT
Our OOM testing machinery hits false positives in IonBuilder where it tries to allocate something from the TempAlloc, and we inject a failure there, and of course it crashes, because it's supposed to be impossible for an error to occur there, due to ballast.

In debug builds, we should track how many bytes of ballast we have left, and assert it never reaches 0.

If that assertion ever hits, it'll be easy to add another ensureBallast() call somewhere.
Comment 1 User image Nicolas B. Pierron [:nbp] 2015-10-06 06:37:03 PDT
Most of this issue seems to be fixed by Bug 1155618.
The code path of the allocator got moved into the allocImpl functions, which is no longer used by the allocInfallible which has the AutoEnterOOMUnsafeRegion section which reports an unhandlable OOM.

I think we should not report them as unhandlable oom … I will work on that.
Comment 2 User image Nicolas B. Pierron [:nbp] 2015-10-06 06:57:57 PDT
Created attachment 8670245 [details] [diff] [review]
Ensure that we can check for OOMs when we run out of ballast space.

This patch prevent us from reporting unhandlable oom for the TempAlloc (only
user of the Lifo allocInfallible function).

The TempAllocator issues can be handled by allocating more ballast space in
for-loops which are doing multiple allocations.
Comment 3 User image Nicolas B. Pierron [:nbp] 2015-10-06 07:03:39 PDT
We should not land this bug before much more fuzzing is made on this patch, as this replace a failure by some code which is considered to be infallible.  Maybe it would make sense to have a MOZ_RELEASE_ASSERT instead.
Comment 4 User image Jan de Mooij [:jandem] 2015-10-07 03:49:29 PDT
Comment on attachment 8670245 [details] [diff] [review]
Ensure that we can check for OOMs when we run out of ballast space.

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

Yeah, I think the idea makes sense but this should be a MOZ_RELEASE_ASSERT.
Comment 5 User image Nicolas B. Pierron [:nbp] 2015-10-08 05:22:36 PDT
Created attachment 8671337 [details] [diff] [review]
Ensure that we can check for OOMs when we run out of ballast space.

This patch makes sure that the TempAllocator used by IonMonkey reports
non-handlable oom as issues, such that we can ensure that we do not fail
because of large allocations made by the compiler.
Comment 7 User image Carsten Book [:Tomcat] 2015-10-28 02:50:26 PDT
https://hg.mozilla.org/mozilla-central/rev/fbf7d94986bb

Note You need to log in before you can comment on or make changes to this bug.