872823 - Move -A option to a OOMAfterAllocations function

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

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

It will be nice to move the -A option into the shell, probably as a OOMAfterAllocations function or something. This might help the fuzzers.

We can still get the original CLI behaviour by running -e "OOMAfterAllocations(<some number>)".

(decoder, sfink, Jesse and I were somehow part of this conversation at some point)

Improvements to this function are welcome, though. :)

Comment 1

[Christian Holler (:decoder)]

Another advantage I see is that this could be placed anywhere in the script, making the number less dependent on the total memory usage of the script/startup/etc. When I report a bug with -A then the number is usually only valid a few revisions of m-c then the test doesnt reproduce anymore. That makes it even harder to get these bugs fixed.

Comment 2

[Steve Fink [:sfink] [:s:]]

Attachment: implement oomAfterAllocations testing function

The number is interpreted as the number of additional allocations after the oomAfterAllocations() call (well, after the body of the function).

In trying it out, I already found an OOM bug in jstypedarray.cpp. :-)

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 750171 [details] [diff] [review]
implement oomAfterAllocations testing function

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

::: js/src/builtin/TestingFunctions.cpp
@@ +739,5 @@
> +        JS_ReportError(cx, "count argument required");
> +        return false;
> +    }
> +
> +    int32_t count = args[0].toInt32();

This should deal with the case where it's not an int32. Otherwise the fuzzers will complain.

@@ +745,5 @@
> +        JS_ReportError(cx, "count argument must be positive");
> +        return false;
> +    }
> +
> +    OOM_maxAllocations = OOM_counter + count;

This should be inside #ifdef DEBUG. Also, you should probably set the return value to something.

@@ +939,5 @@
>  "  to count only things of that kind."),
>  
> +    JS_FN_HELP("oomAfterAllocations", OOMAfterAllocations, 1, 0,
> +"oomAfterAllocations(count)",
> +"  After 'count' js_malloc memory allocations, fail the allocation (return NULL)"),

The help text should end with a period to be consistent.

Comment 4

[Steve Fink [:sfink] [:s:]]

Attachment: implement oomAfterAllocations testing function

Ugh. I hope you don't mind the re-review request. It looks like I neglected to update my previous patch before uploading, since I had already fixed 2 out of 3 of your issues. I've made a few further updates. I made the whole function DEBUG-only, which is probably the only thing that needs review.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 751770 [details] [diff] [review]
implement oomAfterAllocations testing function

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

::: js/src/builtin/TestingFunctions.cpp
@@ +945,5 @@
> +#ifdef DEBUG
> +    JS_FN_HELP("oomAfterAllocations", OOMAfterAllocations, 1, 0,
> +"oomAfterAllocations(count)",
> +"  After 'count' js_malloc memory allocations, fail every following allocation\n"
> +"(return NULL)."),

Could you indent this line two spaces (after the opening quote) so it lines up?

Comment 6

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/0b4e06782cda

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0b4e06782cda