1395240 - Implement stackTest function for recursion checks similar to oomTest

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Christian Holler (:decoder)]

Attachment: js-recursion.patch

Similar to what oomTest does, it would be possible to fail the stack recursion check after a certain amount of checks, then do this iteratively until we exhausted all failure points for the given function.

The motivation to do so is the same as for OOM checking: Early returns due to stack space exhaustion can have the same impact as early returns due to OOM.

I've duplicated the oomTest function to a function called stackTest that performs these steps for the recursion checks by hooking into the CheckRecursionLimit function family.


Most of the code is identical to the code for OOM testing, but I still have some open questions:

1) Do we need something like cx->runtime()->hadOutOfMemory for recursion checks? The OOMTest code uses this variable but I don't know what the exact purpose is. We don't have a similar property for recursion checks, other than the private overRecursed_ attribute in the JSContext.

2) I've done a simple experiment and added "oomTest = stackTest;" to prologue.js and then ran all the jit-tests, just to see what happens. I only saw a handful of failures (one assertion which might be a real bug), but also a few "InternalError: too much recursion". Shouldn't the latter be cleared by the call to clearPendingException? Is the over recursion exception special in any way?


Attached is a patch for initial feedback. Note that this is WIP and has some code commented out that we need to remove/replace later. I still feel like this is pretty close to something we can start using for testing.

Comment 1

[Jan de Mooij [:jandem]]

Comment on attachment 8902777 [details] [diff] [review]
js-recursion.patch

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

Looks good to me!

> 1) Do we need something like cx->runtime()->hadOutOfMemory for recursion
> checks? The OOMTest code uses this variable but I don't know what the exact
> purpose is. We don't have a similar property for recursion checks, other
> than the private overRecursed_ attribute in the JSContext.

I don't think we need this.

> 2) I've done a simple experiment and added "oomTest = stackTest;" to
> prologue.js and then ran all the jit-tests, just to see what happens. I only
> saw a handful of failures (one assertion which might be a real bug), but
> also a few "InternalError: too much recursion". Shouldn't the latter be
> cleared by the call to clearPendingException? Is the over recursion
> exception special in any way?

clearPendingException will clear over recursion. Did these tests use the Debugger object? In the shell EnvironmentPreparer::invoke uses AutoReportException, which prints exceptions to stderr, so it's possible that's what you're seeing here (in that case the exception is printed *under* the stack test function).

::: js/public/Utility.h
@@ +154,5 @@
>      return counter >= maxAllocations;
>  }
>  
> +/*
> + * Out of stack space testing support, similar to OOM testing functions.

I think this will fail in opt builds, you probably need some no-op definitions in the #else branch. It might be safer (as in: no effect on performance) to use a macro or #ifdef the code in jsfriendapi.h

@@ +196,5 @@
> +    return false;
> +}
> +
> +inline bool
> +HadSimulatedStackOOM() {

Nit: { on its own line

::: js/src/builtin/TestingFunctions.cpp
@@ +1735,5 @@
> +            if (verbose)
> +                fprintf(stderr, "  allocation %d\n", allocation);
> +
> +            MOZ_ASSERT(!cx->isExceptionPending());
> +            //MOZ_ASSERT(!cx->runtime()->hadOutOfMemory);

Nit: rm

@@ +1753,5 @@
> +                MOZ_ASSERT(!cx->isExceptionPending(),
> +                           "Thunk execution succeeded but an exception was raised - "
> +                           "missing error check?");
> +            } else if (expectExceptionOnFailure) {
> +/*                MOZ_ASSERT(cx->isExceptionPending(),

Nit: remove the /* */

::: js/src/jsfriendapi.h
@@ +1073,5 @@
>  {
>      int stackDummy;
> +
> +    if (js::oom::ShouldFailWithStackOOM()) {
> +        return false;

Nit: no {}

::: js/src/jsutil.cpp
@@ +99,5 @@
>      failAlways = false;
>  }
>  
> +void
> +SimulateStackOOMAfter(uint64_t allocations, uint32_t thread, bool always) {

Style nit: { on its own line and below (looks like the OOM functions have the same issue).

Comment 2

[Christian Holler (:decoder)]

Attachment: Bug 1395240 - Implement stackTest function for JS stack OOM testing.

Review commit: https://reviewboard.mozilla.org/r/181870/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181870/

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8910437 [details]
Bug 1395240 - Implement stackTest function for JS stack OOM testing.

https://reviewboard.mozilla.org/r/181870/#review187486

LGTM.

The terms allocation(s) and OOM are a bit confusing in this context, but it's consistent with the OOM code so it's probably fine.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8910437 [details]
Bug 1395240 - Implement stackTest function for JS stack OOM testing.

Jon, do you have any thoughts on this?

Comment 5

[Jon Coppeard (:jonco)]

Comment on attachment 8910437 [details]
Bug 1395240 - Implement stackTest function for JS stack OOM testing.

I like this idea.  It will be interesting to see what it finds.

I agree about the terminology but can't think of anything better right now.

Comment 6

[Christian Holler (:decoder)]

Comment on attachment 8910437 [details]
Bug 1395240 - Implement stackTest function for JS stack OOM testing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181870/diff/1-2/

Comment 7

[Christian Holler (:decoder)]

(In reply to Jon Coppeard (:jonco) from comment #5)
> Comment on attachment 8910437 [details]
> Bug 1395240 - Implement stackTest function for JS stack OOM testing.
> 
> I like this idea.  It will be interesting to see what it finds.
> 
> I agree about the terminology but can't think of anything better right now.

Thanks! I changed "allocations" to "checks" to make this a little better. As discussed before with jandem, I kept the term "OOM" because in fact this is also an out-of-memory situation (just not for heap but instead for stack memory).

I triggered a try run now to ensure the builds don't break, then I will land this.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a69946757ded
Implement stackTest function for JS stack OOM testing. r=jandem

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a69946757ded