964803 - Cleanup/Improve OOM testing code in the JS shell

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

Attachment: 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

[Jan de Mooij [:jandem]]

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

[Christian Holler (:decoder)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/338a45ec7af9

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/338a45ec7af9