1006876 - Crash with too much recursion through [@ js::SavedStacks::insertFrames] with enableTrackAllocations

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 87c8f870e2b9 (run with --fuzzing-safe):


enableTrackAllocations();
function f(i) {
    f(i ? i + 1 : 1);
}
f(0);

Comment 1

[Christian Holler (:decoder)]

Causes multiple frequent crashes in LangFuzz, marking as a fuzzblocker.

Comment 2

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

This is most likely my doing :-/

Comment 4

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
=== Tinderbox Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20140501084610" and the hash "ed4c52e21a5f".
The "bad" changeset has the timestamp "20140501092310" and the hash "01dd7c0d8f8d".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ed4c52e21a5f&tochange=01dd7c0d8f8d

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: bug-1006876-too-much-recursion-crash.patch

Try push: https://tbpl.mozilla.org/?tree=Try&rev=ccf7f9d55740

Comment 6

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8421411 [details] [diff] [review]
bug-1006876-too-much-recursion-crash.patch

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

The problem here seems to be that when the test overrecurses, js_ReportOverRecursed tries to create an ErrorObject, which triggers a call to saveCurrentStack if tracking allocation sites is enabled. Since we're already overrecursed at this point, saving the ccurrent stack will run out of stack space while iterating over the script frames, causing a crash.

In that light, this fix is not actually correct, because it causes overrecursion to be reported twice. A better fix would be to add the following check to SavedStacksMetadataCallback:

JS_CHECK_RECURSION_DONT_REPORT(cx, return true);

This will cause us to not save the callstack if the metadata callback was triggered in response to an overrecursion error, without reporting the overrecursion error twice.

Since it isn't immediately obvious, I feel that adding a comment explaining why this check is necessary would be welcome as well :-)

r+ with comments addressed

::: js/src/vm/SavedStacks.cpp
@@ +423,5 @@
>      if (iter.done()) {
>          frame.set(nullptr);
>          return true;
>      }
> +    JS_CHECK_RECURSION(cx, return false);

The problem here seems to be that when the test overrecurses, js_ReportOverRecursed tries to create an ErrorObject, which triggers a call to saveCurrentStack if tracking allocation sites is enabled. Since we're already overrecursed at this point, saving the ccurrent stack will run out of stack space while iterating over the script frames, causing a crash.

In that light, this fix is not actually correct, because it causes overrecursion to be reported twice. A better fix would be to add the following check to SavedStacksMetadataCallback:

JS_CHECK_RECURSION_DONT_REPORT(cx, return true);

This will cause us to not save the callstack if the metadata callback was triggered in response to an overrecursion error, without reporting the overrecursion error twice.

Since this isn't immediately obvious, I feel that adding a comment here would be welcome as well :-)

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: bug-1006876-too-much-recursion-crash.patch

Updated with :ejpbruel's feedback.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/60b7603b914e

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Backed out for jit-test failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5dcd3716601e

https://tbpl.mozilla.org/php/getParsedLog.php?id=39591001&tree=Mozilla-Inbound

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: bug-1006876-too-much-recursion-crash.patch

Reflagging :ejpbruel for review because this changes the way we fix the crash. Instead of checking for too much recursion in the saved stacks code, just disable object metadata callbacks while we report too much recursion.

No try push because all the trees are closed :(

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=ec35569cb8c7

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8422047 [details] [diff] [review]
bug-1006876-too-much-recursion-crash.patch

Clearing review until I get a green try push.

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: bug-1006876-too-much-recursion-crash.patch

So it seems that we release all jit code when setting the object metadata callback which does some stuff that eventually fails an assertion in the GC's nursery code. I don't have a great understanding of the code between setting the object metadata callback and the assertion failure, so I opted to take the path of least resistance and just add the recursion check back to SavedStacks::insertFrames.

https://tbpl.mozilla.org/?tree=Try&rev=dde266d21acf

Comment 14

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8423465 [details] [diff] [review]
bug-1006876-too-much-recursion-crash.patch

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

I would have preferred to figure out what's going on here, but I don't blame you for taking the easy route here.

Perhaps add a comment that explains why we put the recursion check here and not where I suggested it?

Sorry that my suggestion ended up costing you this much time!

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: bug-1006876-too-much-recursion-crash.patch

Added a comment describing why the recursion check is in insertFrames instead of saveCurrentStack.

Comment 16

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/7db77c1072cb

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7db77c1072cb