733493 - Improve OOM testing code in the JS shell

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

In bug 624094, pbiggar added the necessary code for testing out-of-memory conditions using the JS shell switch -A. I'd like to add two improvements to this code:

1. Add the JS_OOM_POSSIBLY_FAIL() macro call also to the LifoAlloc::alloc and NewGCThing functions, in order to be able to trigger more fine-grained OOM conditions.

2. Extend the JS_OOM_POSSIBLY_FAIL() macro to print backtraces of failed allocations with a special configure flag --enable-oom-backtrace. I found this very helpful during debugging because it easily gives an impression which allocation failed.

If you don't like the second part, then we can of course omit this.

Comment 1

[Christian Holler (:decoder)]

Attachment: Patch

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 603396 [details] [diff] [review]
Patch

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

r=me with the error-handling fix.

::: js/public/Utility.h
@@ +131,5 @@
> +        fprintf(stderr, "Forcing artificial memory allocation function failure:\n");\
> +        for (OOM_traceIdx = 0; OOM_traceIdx < OOM_traceSize; ++OOM_traceIdx) {\
> +            fprintf(stderr, "#%d %s\n", OOM_traceIdx, OOM_traceSymbols[OOM_traceIdx]);\
> +        }\
> +    } while (0)

This should skip printing if OOM_traceSymbols is null, and it should
  free(OOM_traceSymbols);
afterwards.

I would put the code in a separate function, defined in jsutil.cpp alongside OOM_maxAllocations and OOM_counter, but it's your call.

Comment 3

[Christian Holler (:decoder)]

Attachment: Updated patch

Updated patch to fix problems mentioned in review :)

Jason: I tried moving the function to jsutil.cpp but I always ended up with either a compiler or linker error. Is this supposed to work somehow? The macro lives in an extern C block I think.

In this version of the patch, I moved the code to a function PrintBacktrace but I didn't succeed putting it to jsutil.cpp. Maybe I'm just being blind ;)

Comment 4

[Jason Orendorff [:jorendorff]]

(In reply to Christian Holler (:decoder) from comment #3)
> Jason: I tried moving the function to jsutil.cpp but I always ended up with
> either a compiler or linker error. Is this supposed to work somehow? The
> macro lives in an extern C block I think.

Oh, you would need something like this:

// Utility.h or jsapi.h
extern JS_PUBLIC_API(void) JS_PrintBacktrace();

// jsutil.cpp or jsapi.cpp
JS_PUBLIC_API(void)
PrintBacktrace()
{
    ...
}

Don't worry about it. This is fine. Let's get it in.

Comment 5

[Christian Holler (:decoder)]

Comment on attachment 604509 [details] [diff] [review]
Updated patch

Thanks Jason! :) Carrying r+ from last review.

Garçon, err, Gary, can you land this on inbound for me? ;) *runs*

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> Garçon, err, Gary, can you land this on inbound for me? ;) *runs*

Yessir, http://hg.mozilla.org/integration/mozilla-inbound/rev/6169d8aa7a9d

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Backed out - either this patch or the patch in bug 727326 set mozilla-inbound tbpl on fire.

http://hg.mozilla.org/integration/mozilla-inbound/rev/499b56d4809e

Please submit the patch to the tryserver first.

Comment 8

[Christian Holler (:decoder)]

Attachment: Updated patch

Fixed build bustage, stupid mistake that I had fixed locally but not refreshed. The #ifdef was not covering the backtrace function added.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Relanded.
https://hg.mozilla.org/integration/mozilla-inbound/rev/48ad947e93ea

Comment 10

[Daniel Holbert [:dholbert]]

Merged to m-c...
 the initial cset:   https://hg.mozilla.org/mozilla-central/rev/6169d8aa7a9d
 and backout:        https://hg.mozilla.org/mozilla-central/rev/499b56d4809e
 and re-landed cset: https://hg.mozilla.org/mozilla-central/rev/48ad947e93ea