Closed Bug 1006876 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: decoder, Assigned: fitzgen)

Details

(Keywords: crash, testcase, Whiteboard: [fuzzblocker] [jsbugmon:update])

Crash Data

Attachments

(2 files, 4 obsolete files)

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


enableTrackAllocations();
function f(i) {
    f(i ? i + 1 : 1);
}
f(0);
Causes multiple frequent crashes in LangFuzz, marking as a fuzzblocker.
Whiteboard: [jsbugmon:update,bisect][fuzzblocker]
This is most likely my doing :-/
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Whiteboard: [jsbugmon:update,bisect][fuzzblocker] → [fuzzblocker] [jsbugmon:update]
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 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 :-)
Attachment #8421411 - Flags: review?(ejpbruel) → review+
Updated with :ejpbruel's feedback.
Attachment #8421411 - Attachment is obsolete: true
Attachment #8421856 - Flags: review+
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 :(
Attachment #8421856 - Attachment is obsolete: true
Attachment #8422047 - Flags: review?(ejpbruel)
Comment on attachment 8422047 [details] [diff] [review]
bug-1006876-too-much-recursion-crash.patch

Clearing review until I get a green try push.
Attachment #8422047 - Flags: review?(ejpbruel)
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
Attachment #8422047 - Attachment is obsolete: true
Attachment #8423465 - Flags: review?(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!
Attachment #8423465 - Flags: review?(ejpbruel) → review+
Added a comment describing why the recursion check is in insertFrames instead of saveCurrentStack.
Attachment #8423465 - Attachment is obsolete: true
Attachment #8424078 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/7db77c1072cb
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.