Improve OOM testing code in the JS shell

RESOLVED FIXED in mozilla13

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: decoder, Assigned: decoder)

Tracking

Trunk
mozilla13
All
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:want])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

7 years ago
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.
(Assignee)

Comment 1

7 years ago
Created attachment 603396 [details] [diff] [review]
Patch
Attachment #603396 - Flags: review?(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.
Attachment #603396 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 3

6 years ago
Created attachment 604509 [details] [diff] [review]
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 ;)
Attachment #603396 - Attachment is obsolete: true
(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.
(Assignee)

Comment 5

6 years ago
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*
Attachment #604509 - Flags: review+
Attachment #604509 - Flags: checkin?(gary)
> Garçon, err, Gary, can you land this on inbound for me? ;) *runs*

Yessir, http://hg.mozilla.org/integration/mozilla-inbound/rev/6169d8aa7a9d
Attachment #604509 - Flags: checkin?(gary) → checkin+
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.
(Assignee)

Comment 8

6 years ago
Created attachment 604679 [details] [diff] [review]
Updated patch

Fixed build bustage, stupid mistake that I had fixed locally but not refreshed. The #ifdef was not covering the backtrace function added.
Attachment #604509 - Attachment is obsolete: true
Attachment #604679 - Flags: review+
Relanded.
https://hg.mozilla.org/integration/mozilla-inbound/rev/48ad947e93ea
Target Milestone: --- → mozilla13
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
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.