Last Comment Bug 964803 - Cleanup/Improve OOM testing code in the JS shell
: Cleanup/Improve OOM testing code in the JS shell
Status: RESOLVED FIXED
[qa-][adv-main31-]
: sec-want
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Linux
: -- critical (vote)
: mozilla31
Assigned To: Christian Holler (:decoder)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 872823
Blocks: 912928 988097
  Show dependency treegraph
 
Reported: 2014-01-28 07:45 PST by Christian Holler (:decoder)
Modified: 2014-07-09 16:59 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
js-oom-cleanup.patch (4.70 KB, patch)
2014-01-28 07:45 PST, Christian Holler (:decoder)
jdemooij: review+
Details | Diff | Splinter Review

Description User image Christian Holler (:decoder) 2014-01-28 07:45:44 PST
Created attachment 8366658 [details] [diff] [review]
js-oom-cleanup.patch

Currently, the JS shell can be tested for OOM behavior using the oomAfterAllocations function. This function works together with macros in js/Public.h. There are several issues:

1. The backtrace code stuff in there isn't used anymore. I initially added that code, but later found out that using a scripted gdb is easier, so we should just rip out that stuff.

2. We have two macros, JS_OOM_POSSIBLY_FAIL() and JS_OOM_POSSIBLY_FAIL_REPORT(cx). The second macro was added because some places where OOM could happen did not call js_ReportOutOfMemory, which made it impossible to break on that function to get an OOM backtrace. However, the number of places that would need JS_OOM_POSSIBLY_FAIL_REPORT has increased and I don't see a big advantage in keeping that macro (also because it will report OOM where no OOM should be reported). Instead, we should just use JS_OOM_POSSIBLY_FAIL() and add an empty function that we can break on in gdb. This function must not be inlined etc, and should only be activated with a configure flag (we can recycle the flag from 1. for that purpose and just rename it).

Patch is attached.
Comment 1 User image Jan de Mooij [:jandem] 2014-01-28 13:52:10 PST
Comment on attachment 8366658 [details] [diff] [review]
js-oom-cleanup.patch

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

Looks good; nice cleanup!

::: js/public/Utility.h
@@ +81,5 @@
>  extern JS_PUBLIC_DATA(uint32_t) OOM_maxAllocations; /* set in builtins/TestingFunctions.cpp */
>  extern JS_PUBLIC_DATA(uint32_t) OOM_counter; /* data race, who cares. */
>  
> +#ifdef JS_OOM_BREAKPOINT
> +static JS_NEVER_INLINE void js_failedAllocBreakpoint() { asm(""); }

Nit: s/JS_NEVER_INLINE/MOZ_NEVER_INLINE (bug 964016 got rid of JS_NEVER_INLINE)

@@ +92,4 @@
>      do \
>      { \
>          if (++OOM_counter > OOM_maxAllocations) { \
> +            JS_OOM_CALLBPFUNC();\

Nit: I think either JS_OOM_CALL_BREAKPOINT_FUNC() or JS_OOM_CALL_BP_FUNC() is a bit clearer.
Comment 2 User image Christian Holler (:decoder) 2014-03-25 16:33:09 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/338a45ec7af9
Comment 3 User image Ryan VanderMeulen [:RyanVM] 2014-03-26 18:12:36 PDT
https://hg.mozilla.org/mozilla-central/rev/338a45ec7af9

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