SavedStacks / JS::CaptureCurrentStack should take an optional depth limit

RESOLVED FIXED in mozilla33

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: fitzgen, Assigned: jimb)

Tracking

unspecified
mozilla33
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

So that you can limit the number of frames from the stack you want to capture. If a limit n was provided, we would only capture the n youngest frames.
(Assignee)

Comment 1

4 years ago
Created attachment 8448895 [details] [diff] [review]
Make CaptureCurrentStack accept an optional limit on the number of frames captured.
Assignee: nobody → jimb
Status: NEW → ASSIGNED
Attachment #8448895 - Flags: review?(nfitzgerald)
Comment on attachment 8448895 [details] [diff] [review]
Make CaptureCurrentStack accept an optional limit on the number of frames captured.

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

Looks good to me, but I'm not a SpiderMonkey peer.
Attachment #8448895 - Flags: review?(nfitzgerald) → feedback+
(Assignee)

Updated

4 years ago
Attachment #8448895 - Flags: review?(evilpies)
(Assignee)

Comment 4

4 years ago
Comment on attachment 8448895 [details] [diff] [review]
Make CaptureCurrentStack accept an optional limit on the number of frames captured.

I think Tom is on vacation.
Attachment #8448895 - Flags: review?(evilpies) → review?(ejpbruel)
(Assignee)

Updated

4 years ago
Attachment #8448895 - Flags: review?(ejpbruel) → review?(shu)

Comment 5

4 years ago
Comment on attachment 8448895 [details] [diff] [review]
Make CaptureCurrentStack accept an optional limit on the number of frames captured.

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

::: js/src/builtin/TestingFunctions.cpp
@@ +885,5 @@
> +    if (args.length() >= 1) {
> +        double d;
> +        if (!ToNumber(cx, args[0], &d))
> +            return false;
> +        if (d < 0) {

This is just a testing function so it doesn't really matter, but a better API would be having Infinity meaning "no limit" than 0 meaning "no limit"

::: js/src/jit-test/tests/saved-stacks/max-frame-count.js
@@ +17,5 @@
> +assertEq(recur(0, 0).parent !== null, true);
> +assertEq(recur(0, 0).parent.parent, null);
> +assertEq(recur(1, 0).parent.parent.parent, null);
> +assertEq(recur(2, 0).parent.parent.parent.parent, null);
> +assertEq(recur(3, 0).parent.parent.parent.parent.parent, null);

Why are there so many parents here? What are the top 2? One of them I imagine is the toplevel, what's the one above that?
Attachment #8448895 - Flags: review?(shu) → review+
(Assignee)

Comment 6

4 years ago
(In reply to Shu-yu Guo [:shu] from comment #5)
> Comment on attachment 8448895 [details] [diff] [review]
> Make CaptureCurrentStack accept an optional limit on the number of frames
> captured.
> 
> Review of attachment 8448895 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/builtin/TestingFunctions.cpp
> @@ +885,5 @@
> > +    if (args.length() >= 1) {
> > +        double d;
> > +        if (!ToNumber(cx, args[0], &d))
> > +            return false;
> > +        if (d < 0) {
> 
> This is just a testing function so it doesn't really matter, but a better
> API would be having Infinity meaning "no limit" than 0 meaning "no limit"

I agree. However, I'm nervous about the portability of <math.h>'s INFINITY constant. I want to land this promptly, so I'm going to leave it as-is.


> ::: js/src/jit-test/tests/saved-stacks/max-frame-count.js
> @@ +17,5 @@
> > +assertEq(recur(0, 0).parent !== null, true);
> > +assertEq(recur(0, 0).parent.parent, null);
> > +assertEq(recur(1, 0).parent.parent.parent, null);
> > +assertEq(recur(2, 0).parent.parent.parent.parent, null);
> > +assertEq(recur(3, 0).parent.parent.parent.parent.parent, null);
> 
> Why are there so many parents here? What are the top 2? One of them I
> imagine is the toplevel, what's the one above that?

In the 'recur(0, 0)' case, the two frames are the single call to recur, and the top-level script.

In the 'recur(3, 0)' case, we've got four frames for recur, and one frame for the top-level script.
(Assignee)

Comment 7

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b018836f449
https://hg.mozilla.org/integration/mozilla-inbound/rev/111548aad0c5
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla33
https://hg.mozilla.org/mozilla-central/rev/111548aad0c5
https://hg.mozilla.org/mozilla-central/rev/2b018836f449
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.