The default bug view has changed. See this FAQ.

Make Error instances use SavedFrame for stacks

RESOLVED FIXED in Firefox 39

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: bz, Assigned: fitzgen)

Tracking

(Depends on: 1 bug, Blocks: 4 bugs)

unspecified
mozilla39
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: [MemShrink])

Attachments

(7 attachments, 18 obsolete attachments)

18.86 KB, patch
Details | Diff | Splinter Review
16.77 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
73.17 KB, patch
decoder
: feedback+
Details | Diff | Splinter Review
4.13 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
45.10 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
5.42 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
11.10 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
See some of the discussion in bug 1036527.  Spinning this out, since we can do this without actually fixing bug 1036527.
Created attachment 8455431 [details] [diff] [review]
Somewhat of a start


This is some hackery to try this out.  It has at least the following issues:

1)  I just disabled the wrap() call in js_CopyErrorObject to make this
compile.  That's clearly wrong.

2)  This is storing the SavedFrame in the slot.  That's bogus; we want to
either store a string or nothing at all.  We definitely shouldn't expose the
SavedFrame to script.

To make the wrap() thing really work right, we probably need to allow
creation of Error objects with a HandleObject and then just assert that it's
either a SavedFrame or a security wrapper for one.

For the other, we need to decide whether we want to use a getter or a resolve
hook (with Jason preferring the latter) to do this, and then rejigger the
insides of ErrorObject accordingly.
Blocks: 1083849
No longer blocks: 1036527
Depends on: 1036527
Blocks: 1125259
Blocks: 1036527
No longer depends on: 1036527
OK.  So can we decide whether we want an accessor or a resolve hook here?  I'm happy to try hooking up either one...
Flags: needinfo?(jorendorff)
Though if resolve hook, I'm not sure how that's supposed to interact with Xrays, exactly...
Blocks: 1127694
(In reply to Boris Zbarsky [:bz] from comment #2)
> OK.  So can we decide whether we want an accessor or a resolve hook here? 
> I'm happy to try hooking up either one...

Slight preference for an accessor, but feel free to do whatever's expedient.
Flags: needinfo?(jorendorff)
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Blocks: 1142577
Created attachment 8576972 [details] [diff] [review]
Make Error instances use SavedFrame objects for their stacks
Created attachment 8576990 [details] [diff] [review]
Make Error instances use SavedFrame objects for their stacks
Attachment #8576972 - Attachment is obsolete: true
(In reply to Nick Fitzgerald [:fitzgen] from comment #6)
> Created attachment 8576990 [details] [diff] [review]
> Make Error instances use SavedFrame objects for their stacks

So it seems to work locally, for the most part, but I'm getting this strange bug where the error name doesn't prefix the error message in jit-test (seems to work fine for me in the shell):

WITHOUT PATCH

/Users/fitzgen/src/mozilla-central/js/src/jit-test/tests/basic/testBug1126754.js:9:0 SyntaxError: missing ( before formal parameters:
/Users/fitzgen/src/mozilla-central/js/src/jit-test/tests/basic/testBug1126754.js:9:0 })()
/Users/fitzgen/src/mozilla-central/js/src/jit-test/tests/basic/testBug1126754.js:9:0 ^
Exit code: 3
[1|0|0|0] 100% ==========================================================>|   0.2s
PASSED ALL


WITH PATCH

/Users/fitzgen/src/mozilla-central/js/src/jit-test/tests/basic/testBug1126754.js:9:0 missing ( before formal parameters:
/Users/fitzgen/src/mozilla-central/js/src/jit-test/tests/basic/testBug1126754.js:9:0 })()
/Users/fitzgen/src/mozilla-central/js/src/jit-test/tests/basic/testBug1126754.js:9:0 ^
Exit code: 3
FAIL - basic/testBug1126754.js
[0|1|0|0] 100% ==========================================================>|   0.2s
FAILURES:
    /Users/fitzgen/src/mozilla-central/js/src/jit-test/tests/basic/testBug1126754.js
TIMEOUTS:
Created attachment 8577265 [details] [diff] [review]
Make Error instances use SavedFrame objects for their stacks
Attachment #8576990 - Attachment is obsolete: true
(In reply to Nick Fitzgerald [:fitzgen] from comment #8)
> Created attachment 8577265 [details] [diff] [review]
> Make Error instances use SavedFrame objects for their stacks

Alright got it down to two jit-test failures. Getting closer.
Created attachment 8577572 [details] [diff] [review]
Part 0: Make js/src/vm/SavedStacks.h use 1-based column numbers, like js::ComputeStackString and other browsers do.
Attachment #8577265 - Attachment is obsolete: true
Attachment #8577572 - Flags: review?(jorendorff)
Created attachment 8577573 [details] [diff] [review]
Part 1: Make Error instances use SavedFrame objects for their stacks
Attachment #8577573 - Flags: review?(jorendorff)
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=65aee61f2ea2
:gkw, do you think you could fuzz these patches a bit? I'd like to double check that this isn't introducing any weird behavior before landing.
Flags: needinfo?(gary)
(In reply to Nick Fitzgerald [:fitzgen] from comment #12)
> Try push:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=65aee61f2ea2

Oof. That's not a happy try push.
A bunch of those look like the fact that the new getter and setter don't handle "this" being an Xray to an Error object, right?
(In reply to Not doing reviews right now from comment #15)
> A bunch of those look like the fact that the new getter and setter don't
> handle "this" being an Xray to an Error object, right?

Yup
Created attachment 8577666 [details] [diff] [review]
Part 1: Make Error instances use SavedFrame objects for their stacks
Attachment #8577573 - Attachment is obsolete: true
Attachment #8577573 - Flags: review?(jorendorff)
Attachment #8577666 - Flags: review?(jorendorff)
Ok, that should take care of the issue with wrappers, and here is another try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c325d39aeae1
Which patches should be tested? (Part 0 or 1 or both?)

It is preferred to have a rollup patch and let us know which m-c rev it is based on, again preferably rebased to tip.
Flags: needinfo?(gary)
Comment on attachment 8577572 [details] [diff] [review]
Part 0: Make js/src/vm/SavedStacks.h use 1-based column numbers, like js::ComputeStackString and other browsers do.

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

::: js/src/vm/SavedStacks.cpp
@@ +1094,5 @@
>              return false;
>  
>          locationp->line = iter.computeLine(&locationp->column);
> +        // Make the column 1-based instead of 0-based as in other browsers.
> +        locationp->column++;

As discussed on IRC, please push this ++ down into FrameIter::computeLine and from there down into the functions it calls, and so on until we're 1-based throughout the engine. Reflect.parse can specially -1 things if that's 

I don't think it's necessary to change source notes, since they purportedly only record column *spans*, i.e. the length of character sequences...? I dunno, your call.

Reflect.parse may unfortunately be stuck with 0-indexed column numbers. If so, we can specially subtract 1 in that code.

Btw, this comment is ambiguous; it sounds like you're saying "0-based as in other browsers" which isn't what you mean...
Attachment #8577572 - Flags: review?(jorendorff) → review+
Comment on attachment 8577666 [details] [diff] [review]
Part 1: Make Error instances use SavedFrame objects for their stacks

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

f+ although I'm a little concerned about the amount of unwrapping that seems to surround SavedFrameObjects in the existing code.

::: js/src/vm/ErrorObject.cpp
@@ +166,5 @@
> +                             InformalValueTypeName(thisValue));
> +        return false;
> +    }
> +
> +    RootedObject thisObj(cx, CheckedUnwrap(&thisValue.toObject()));

See if you can use CallNonGenericMethod instead of adding a CheckedUnwrap call here. (Search in js/src/builtins and you'll see plenty of examples of how it's used.)

@@ +214,5 @@
> +        return false;
> +    }
> +
> +    JSAutoCompartment ac(cx, error);
> +    error->setReservedSlot(STACK_OVERRIDDEN_SLOT, BooleanValue(true));

I went back and forth about whether this should define a data property on `error` or what you wrote. I guess what you wrote is better.

::: js/src/vm/SavedStacks-inl.h
@@ +13,5 @@
> +js::AssertObjectIsSavedFrame(JSContext *cx, HandleObject stack)
> +{
> +#ifdef DEBUG
> +    if (stack) {
> +        RootedObject savedFrameObj(cx, CheckedUnwrap(stack));

Please add a comment explaining why it is *necessary* to allow wrappers, and what we have to do to make sure it is *safe* to allow wrappers.

I mean "safe" in the sense of avoiding accidentally accessing SavedStackObject slots on a wrapper object.

@@ +15,5 @@
> +#ifdef DEBUG
> +    if (stack) {
> +        RootedObject savedFrameObj(cx, CheckedUnwrap(stack));
> +        MOZ_ASSERT(savedFrameObj);
> +        MOZ_ASSERT(js::SavedFrame::isSavedFrameAndNotProto(*savedFrameObj));

If you can figure out a way to make the proto a PlainObject, please do it and then callers can just say something like

    MOZ_ASSERT_IF(frameObj, frameObj->is<SavedFrameObject>());
Attachment #8577666 - Flags: review?(jorendorff) → feedback+
Created attachment 8578305 [details] [diff] [review]
Part 2: Add module level documentation for js/src/vm/SavedStacks.h
Attachment #8578305 - Flags: review?(jorendorff)
It seems that the Error.prototype.stack accessor is failing on BackstagePass objects (and perhaps others too, this is the first failure that reproduces).

Do we need to implement xrays for error objects now? Or something else?
Flags: needinfo?(jorendorff)
Created attachment 8578427 [details] [diff] [review]
Part 0: Make js/src/vm/SavedStacks.h use 1-based column numbers, like js::ComputeStackString and other browsers do.
Attachment #8577572 - Attachment is obsolete: true
Attachment #8578427 - Flags: review?(jorendorff)
Created attachment 8578429 [details] [diff] [review]
Part 1: Make Error instances use SavedFrame objects for their stacks
Attachment #8577666 - Attachment is obsolete: true
Created attachment 8578430 [details] [diff] [review]
Part 3: js::ComputeStackString should use SavedFrame
We have Xrays for Error objects.  See bug 987669.

When you say "failing on BackstagePass objects", do you mean that "this" is a BackstagePass?  Or the compartment global is a BackstagePass?  Something else?
Flags: needinfo?(nfitzgerald)
Looking at the try push from comment 18, the M1 failure is triggered by this bit:

 Assert.AssertionError = function(options) {
...
  this.stack = stack;
 }

 Assert.AssertionError.prototype = Object.create(Error.prototype, {
...

in Assert.jsm.  As in, we have people poor-man's-subclassing Error.  At some point we'll make Error actually subclassable and they can stop doing that, but since I bet people do this on the web too, it doesn't seem to me like we can have the stack setter assume it's got an Error object.

The simplest thing to do would be to have the stack setter define a property on "this" (as suggested in comment 21) which has "stack" as the name and the provided value as a value (akin to how [Replaceable] webidl attributes work).  This would shadow the proto setter and it seems like things would just work.

And we should probably add an explicit test for this sort of thing, presumably in SpiderMonkey, since it's not Gecko-specific.

Looking more at the try run, the M5 orange is the same issue.  So is "B".

The "bc" failures are something different, since they involve the "stack" getter.  Would have to actually check what's throwing in TaskImpl_run and what it's throwing, exactly.
Argh, I misread the proto/parent while debugging. I think that this is likely the same issue as the AssertionErrors.

    frame #0: 0x000000010768a716 XUL`js::ErrorObject::checkAndUnwrapThis(cx=0x0000000100433c40, args=0x00007fff5fbf06a8, fnName=0x0000000108569654, error=MutableHandle<js::ErrorObject *> at 0x00007fff5fbf0540) + 262 at ErrorObject.cpp:172
   169 	
   170 	    RootedObject thisObj(cx, CheckedUnwrap(&thisValue.toObject()));
   171 	    if (!thisObj || !thisObj->is<ErrorObject>()) {
-> 172 	        JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_INCOMPATIBLE_PROTO,
   173 	                             js_Error_str, fnName,
   174 	                             thisObj ? thisObj->getClass()->name : "object");
   175 	        return false;
(lldb) p thisObj->dump()
object 0x122ad0920
class 0x10acce730 Object
flags:
proto <Object at 0x11b8f2440>
parent <BackstagePass object at 0x1196599c0>
properties:
    ((js::Shape *) 0x11eb9b8d0) enumerate JSString* (0x1196cc418) = Latin1Char * (0x1196cc420) = "operation"
: slot 0 = JSString* (0x13064ef70) = Latin1Char * (0x13064ef78) = "open"

    ((js::Shape *) 0x11eb9b8f8) enumerate JSString* (0x11964cdd8) = Latin1Char * (0x11964cde0) = "path"
: slot 1 = JSString* (0x121c999a0) = char16_t * (0x12ecdf9c0) = "/var/folders/14/l4lqpmvs5sncftp1c0ym243w0000gp/T/tmpVPgIbI.mozrunner/signedInUser.json"

    ((js::Shape *) 0x11eb9b920) enumerate JSString* (0x11b21a5b0) = Latin1Char * (0x11b21a5b8) = "unixErrno"
: slot 2 = 2

(lldb) p thisObj.ptr->getTaggedProto().toObjectOrNull()->dump()
object 0x11b8f2440
class 0x10acce730 Object
flags: delegate
proto <Proxy object at 0x11b226560>
parent <BackstagePass object at 0x1196599c0>
properties:
    ((js::Shape *) 0x11b22f6a0) enumerate JSString* (0x11961c9a0) = Latin1Char * (0x11961c9a8) = "toString"
: slot 0 = <function toString (resource://gre/modules/osfile/osfile_unix_allthreads.jsm:88) at 0x11b8ef180>
    ((js::Shape *) 0x11b22f6c8) enumerate JSString* (0x1196be838) = Latin1Char * (0x1196be840) = "toMsg"
: slot 1 = <function toMsg (resource://gre/modules/osfile/osfile_unix_allthreads.jsm:94) at 0x11b8f1f80>
    ((js::Shape *) 0x11b2169a8) permanent shared getterValue=0x11b8fa580 JSString* (0x11b21a5f8) = Latin1Char * (0x11b21a600) = "becauseExists"
: slot 16777215
    ((js::Shape *) 0x11b2169e0) permanent shared getterValue=0x11b8f6c80 JSString* (0x1196e55e0) = Latin1Char * (0x1196e55e8) = "becauseNoSuchFile"
: slot 16777215
    ((js::Shape *) 0x11b216a18) permanent shared getterValue=0x11c105bc0 JSString* (0x11b21a610) = Latin1Char * (0x11b21a618) = "becauseNotEmpty"
: slot 16777215
    ((js::Shape *) 0x11b216a50) permanent shared getterValue=0x11b8f4f00 JSString* (0x11b21a640) = Latin1Char * (0x11b21a648) = "becauseClosed"
: slot 16777215
    ((js::Shape *) 0x11b216a88) permanent shared getterValue=0x11b8f85c0 JSString* (0x1196e5600) = Latin1Char * (0x1196e5608) = "becauseAccessDenied"
: slot 16777215
    ((js::Shape *) 0x11eba0548) permanent shared getterValue=0x11b8f8040 JSString* (0x1196e5620) = Latin1Char * (0x1196e5628) = "becauseInvalidArgument"
: slot 16777215
Flags: needinfo?(nfitzgerald)
Created attachment 8578785 [details] [diff] [review]
Part 1: Make Error instances use SavedFrame objects for their stacks
Attachment #8578429 - Attachment is obsolete: true
Ok, with this patch version I was able to successfully run some devtools tests that were previously failing.

Fingers crossed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=19134fcc7d15
Flags: needinfo?(jorendorff)
Blocks: 1144340

Updated

2 years ago
Depends on: 1144353
Created attachment 8578909 [details] [diff] [review]
Part 0: Make js/src/vm/SavedStacks.h use 1-based column numbers, like js::ComputeStackString and other browsers do
Attachment #8578427 - Attachment is obsolete: true
Attachment #8578427 - Flags: review?(jorendorff)
Attachment #8578909 - Flags: review+
(In reply to Nick Fitzgerald [:fitzgen] from comment #32)
> Created attachment 8578909 [details] [diff] [review]
> Part 0: Make js/src/vm/SavedStacks.h use 1-based column numbers, like
> js::ComputeStackString and other browsers do

As discussed on IRC, pushing the 1-based column numbers down into SpiderMonkey turns out to be a bit of a mess, so we are back to the old version of the patch that had r+ already. See bug 1144340 for future work here.
Ok, here is a new try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=faf34dd7458a
Depends on: 1144973
Created attachment 8579713 [details] [diff] [review]
Part 0: Make js/src/vm/SavedStacks.h use 1-based column numbers, like js::ComputeStackString and other browsers do
Attachment #8578909 - Attachment is obsolete: true
Created attachment 8579714 [details] [diff] [review]
Part 1: Make Error instances use SavedFrame objects for their stacks
Attachment #8578785 - Attachment is obsolete: true
And another try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1483c51d4a32
Created attachment 8580155 [details] [diff] [review]
Part 0: Make js/src/vm/SavedStacks.h use 1-based column numbers, like js::ComputeStackString and other browsers do
Attachment #8579713 - Attachment is obsolete: true
Attachment #8580155 - Flags: review+
Created attachment 8580156 [details] [diff] [review]
Part 1: Make Error instances use SavedFrame objects for their stacks
Attachment #8580156 - Flags: review?(jorendorff)
Created attachment 8580157 [details] [diff] [review]
Part 2: Add module level documentation for js/src/vm/SavedStacks.h
Attachment #8578305 - Attachment is obsolete: true
Attachment #8578305 - Flags: review?(jorendorff)
Attachment #8580157 - Flags: review?(jorendorff)
Created attachment 8580158 [details] [diff] [review]
Part 3: js::ComputeStackString should use SavedFrame
Attachment #8578430 - Attachment is obsolete: true
Attachment #8580158 - Flags: review?(jorendorff)
Ok, those last SM(ggc) failures were most likely fixed by bug 1144108. I've rebased onto the latest m-c and I think everything is good to go now!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=19b55498f36f
Attachment #8579714 - Attachment is obsolete: true
Created attachment 8580161 [details] [diff] [review]
combined.patch

This is the combined series (plus bug 1144973) which applies cleanly on the latest m-c, which for posterity is:

    234403[tip]:234392,234402   cbd0efcd976c   2015-03-19 14:00 +0100   cbook
      merge fx-team to mozilla-central a=merge

Gary, can you fuzz this a little bit? Thanks!
Attachment #8580161 - Flags: feedback?(gary)
Attachment #8580161 - Flags: feedback?(choller)
Depends on: 1145441

Updated

2 years ago
Blocks: 1144353
No longer depends on: 1144353
Depends on: 1145643
Comment on attachment 8580161 [details] [diff] [review]
combined.patch

Nothing found after spinning up some extra EC2 instances for testing this.
Attachment #8580161 - Flags: feedback?(gary) → feedback+
Comment on attachment 8580156 [details] [diff] [review]
Part 1: Make Error instances use SavedFrame objects for their stacks

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

This is a very nice patch. Only nits and a few test requests.

I don't see where StringifySavedFrameStack is defined, presumably in some previous patch, but if you can change it to BuildStackString() or something, I'd appreciate it. The only other thing using this word is "JS_Stringify()", which is about JSON; it'd be weird to have just those two unrelated things using the same unusual word.

::: js/src/jit-test/tests/basic/shell-principals.js
@@ +29,5 @@
>       eval('function a() { check("a",        Error().stack); b(); }');
>  low .eval('function b() { check("b",        Error().stack); c(); }');
>  mid .eval('function c() { check("cba",      Error().stack); d(); }');
>  high.eval('function d() { check("dcba",     Error().stack); e(); }');
> +     eval('function e() { check("ecba",     Error().stack); f(); }');

Nice!

I think this test could use a one-line comment somewhere noting that the original global has no principal, and that's treated the same as 0xffff.

::: js/src/jit-test/tests/debug/Memory-drainAllocationsLog-08.js
@@ +20,5 @@
>  let allocs1 = dbg.memory.drainAllocationsLog();
>  root.doSecondAlloc();
>  let allocs2 = dbg.memory.drainAllocationsLog();
>  
> +let allocs1Lines = allocs1.filter(x => !!x.frame).map(x => x.frame.line);

What changed here? The implicit allocation of a bunch of SavedFrame objects?

Could that ever be actually bad in combination with this API?

Btw, if you want to change that root.eval() call to just use a multiline string (`...`) rather than an array and .join() and function.toString(), please feel free...

::: js/src/jsexn.cpp
@@ +369,5 @@
> +{
> +    SuppressErrorsGuard seg(cx);
> +    if (!CaptureCurrentStack(cx, stack, MAX_REPORTED_STACK_DEPTH))
> +        return false;
> +    return true;

return CaptureCurrentStack(...);

::: js/src/vm/ErrorObject.cpp
@@ +168,5 @@
> +
> +    // Walk up the prototype chain until we find the first ErrorObject that has
> +    // the slots we need. This allows us to support the poor-man's subclassing
> +    // of error: Object.create(Error.prototype).
> +    RootedObject target(cx, CheckedUnwrap(&thisValue.toObject()));

It's not a good idea to elide error reporting, because errors are stateful in the JS engine. To avoid reporting errors twice, with the second error clobbering the first, please check after this CheckedUnwrap succeeded:

    if (!target)
        return false;

@@ +173,5 @@
> +    RootedObject proto(cx);
> +    while (target && !target->is<ErrorObject>()) {
> +        if (!GetPrototype(cx, target, &proto))
> +            return false;
> +        target = CheckedUnwrap(proto);

And here.

@@ +178,5 @@
> +    }
> +
> +    if (!target) {
> +        JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_INCOMPATIBLE_PROTO,
> +                             js_Error_str, fnName, thisValue.toObject().getClass()->name);

Make sure there's a test for this error case. It would have to be something like

var obj = {};
var desc = Object.getOwnPropertyDescriptor(Error.prototype, "stack");
Object.defineProperty(obj, "stack", desc);
assertThrowsInstanceOf(() => obj.stack, TypeError);

@@ +194,5 @@
> +    Rooted<ErrorObject*> error(cx);
> +    if (!checkAndUnwrapThis(cx, args, "(get stack)", &error))
> +        return false;
> +
> +    RootedObject savedFrameObj(cx, error->stack());

Please make sure there's a test for when this is null. We should return an empty string, right?

(Presumably we already have a test for Error.prototype.stack etc. somewhere, but make sure...)

@@ +197,5 @@
> +
> +    RootedObject savedFrameObj(cx, error->stack());
> +    RootedString stackString(cx);
> +    if (!StringifySavedFrameStack(cx, savedFrameObj, &stackString))
> +        return false;

Please change the comment on StringifySavedFrameStack in jsapi.h to note two unusual things about it:

1.   The 'stack' argument must be a SavedFrame object, a wrapper around a SavedFrame, or null.

2.   The 'stack' argument does *not* have to be in the same compartment as cx.

@@ +213,5 @@
> +js::ErrorObject::setStack(JSContext *cx, unsigned argc, Value *vp)
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    // We accept any object here, because of poor-man's subclassing of Error.
> +    return CallNonGenericMethod<IsObject, setStack_impl>(cx, args);

Please also test this with non-object receivers:

    assertThrowsInstanceOf(desc.set, TypeError);
    assertThrowsInstanceOf(() => desc.set.call("string"), TypeError);

@@ +229,5 @@
> +    RootedValue val(cx, args[0]);
> +
> +    if (!DefineProperty(cx, thisObj, cx->names().stack, val))
> +        return false;
> +    return true;

Style nit: instead of the if-statement, write a tail call:
    return DefineProperty(...);

::: js/src/vm/SavedStacks-inl.h
@@ +16,5 @@
> +// We allow wrappers here because the JSAPI functions for working with
> +// SavedFrame objects and the SavedFrame accessors themselves handle wrappers
> +// and use the original caller's compartment's principals to determine what
> +// level of data to present. Unwrapping and entering the referent's compartment
> +// would mess that up. See the module level documentation in

Good comment.

::: js/src/vm/SavedStacks.h
@@ +132,1 @@
>  class SavedStacks {

If it's not too much trouble, please add a comment on this class.
Attachment #8580156 - Flags: review?(jorendorff) → review+
Created attachment 8582695 [details] [diff] [review]
Part 1: Make Error instances use SavedFrame objects for their stacks
Attachment #8580156 - Attachment is obsolete: true
Attachment #8582695 - Flags: review+
Created attachment 8582696 [details] [diff] [review]
Part 3: js::ComputeStackString should use SavedFrame
Attachment #8580158 - Attachment is obsolete: true
Attachment #8580158 - Flags: review?(jorendorff)
Attachment #8582696 - Flags: review?(jorendorff)
Comment on attachment 8580161 [details] [diff] [review]
combined.patch

feedback+ from me as well, no bugs found.
Attachment #8580161 - Flags: feedback?(choller) → feedback+
100% green try push with these latest versions: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2bc353a3eca1

This fixes the errors from this try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=19b55498f36f

* Remove SuppressErrorGuard in CaptureStack in jsexn.cpp -- this was causing a test to fail in SM(arm) on basic/bug1118996.js and making the syntax error propogate past the catch!

* Set the stack depth limit from 1 << 20 to 1 << 7 -- this fixes the timeouts in SM(compacting nee ggc) for the too-much-recursion + saved-stacks error.
(In reply to Jason Orendorff [:jorendorff] from comment #45)
> ::: js/src/jit-test/tests/debug/Memory-drainAllocationsLog-08.js
> @@ +20,5 @@
> >  let allocs1 = dbg.memory.drainAllocationsLog();
> >  root.doSecondAlloc();
> >  let allocs2 = dbg.memory.drainAllocationsLog();
> >  
> > +let allocs1Lines = allocs1.filter(x => !!x.frame).map(x => x.frame.line);
> 
> What changed here? The implicit allocation of a bunch of SavedFrame objects?

We were saving a SavedFrame stack for each SavedFrame object we allocated, resulting in accidental O(n^2) behavior. Now, SavedFrame objects just get a null allocation site, but they still show up in the allocations log.

> Could that ever be actually bad in combination with this API?

We already have to handle null stacks in general because Objects can be allocated with no JS on the stack (for example, DOM Events are often allocated before any JS gets run).

> ::: js/src/vm/ErrorObject.cpp
> @@ +168,5 @@
> > +
> > +    // Walk up the prototype chain until we find the first ErrorObject that has
> > +    // the slots we need. This allows us to support the poor-man's subclassing
> > +    // of error: Object.create(Error.prototype).
> > +    RootedObject target(cx, CheckedUnwrap(&thisValue.toObject()));
> 
> It's not a good idea to elide error reporting, because errors are stateful
> in the JS engine. To avoid reporting errors twice, with the second error
> clobbering the first, please check after this CheckedUnwrap succeeded:
> 
>     if (!target)
>         return false;

CheckedUnwrap doesn't take a cx, so it can't set an exception on the cx can it? Am I misunderstanding?

> @@ +194,5 @@
> > +    Rooted<ErrorObject*> error(cx);
> > +    if (!checkAndUnwrapThis(cx, args, "(get stack)", &error))
> > +        return false;
> > +
> > +    RootedObject savedFrameObj(cx, error->stack());
> 
> Please make sure there's a test for when this is null. We should return an
> empty string, right?

Yes, we have tests: https://dxr.mozilla.org/mozilla-central/source/js/src/jsapi-tests/testSavedStacks.cpp

(Although, I see the claiming to test for stack string is actually a copy-paste fail and tests the source property... Will fix that here.)

> @@ +197,5 @@
> > +
> > +    RootedObject savedFrameObj(cx, error->stack());
> > +    RootedString stackString(cx);
> > +    if (!StringifySavedFrameStack(cx, savedFrameObj, &stackString))
> > +        return false;
> 
> Please change the comment on StringifySavedFrameStack in jsapi.h to note two
> unusual things about it:
> 
> 1.   The 'stack' argument must be a SavedFrame object, a wrapper around a
> SavedFrame, or null.
> 
> 2.   The 'stack' argument does *not* have to be in the same compartment as
> cx.

Both are already noted in jsapi.h just above: https://dxr.mozilla.org/mozilla-central/source/js/src/jsapi.h#5174-5196

> ::: js/src/vm/SavedStacks.h
> @@ +132,1 @@
> >  class SavedStacks {
> 
> If it's not too much trouble, please add a comment on this class.

Did you see part 2 yet? Does that cover what you want here?
Created attachment 8582797 [details] [diff] [review]
Part 1: Make Error instances use SavedFrame objects for their stacks
Attachment #8582695 - Attachment is obsolete: true
Attachment #8582797 - Flags: review+
Created attachment 8582802 [details] [diff] [review]
Part 4: Rename JS::StringifySavedFrameStack to JS::BuildStackString
Attachment #8582802 - Flags: review+
(In reply to Nick Fitzgerald [:fitzgen] from comment #50)
> > ::: js/src/vm/ErrorObject.cpp
> > @@ +168,5 @@
> > > +
> > > +    // Walk up the prototype chain until we find the first ErrorObject that has
> > > +    // the slots we need. This allows us to support the poor-man's subclassing
> > > +    // of error: Object.create(Error.prototype).
> > > +    RootedObject target(cx, CheckedUnwrap(&thisValue.toObject()));
> > 
> > It's not a good idea to elide error reporting, because errors are stateful
> > in the JS engine. To avoid reporting errors twice, with the second error
> > clobbering the first, please check after this CheckedUnwrap succeeded:
> > 
> >     if (!target)
> >         return false;
> 
> CheckedUnwrap doesn't take a cx, so it can't set an exception on the cx can
> it? Am I misunderstanding?

New version does:

    if (!target) {
        JS_ReportError(cx, "Permission denied to access object");
        return false;
    }
(In reply to Nick Fitzgerald [:fitzgen] from comment #50)
> CheckedUnwrap doesn't take a cx, so it can't set an exception on the cx can
> it? Am I misunderstanding?

My error. Carry on.

> Did you see part 2 yet? Does that cover what you want here?

It does, and I need to finish that review. On it now...
Comment on attachment 8580157 [details] [diff] [review]
Part 2: Add module level documentation for js/src/vm/SavedStacks.h

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

Thank you so much for this.

::: js/src/vm/SavedStacks.h
@@ +17,5 @@
> +//
> +// The `SavedStacks` class provides a compact way to capture and save JS stacks
> +// as `SavedFrame` `JSObject` subclasses. A single `SavedFrame` object
> +// represents one frame that was on the stack, and has a strong reference to its
> +// parent `SavedFrame` (the next oldest frame). This reference is null when the

I think "next oldest" means the opposite of what you intend here. "Apart from Chris, Izzy is the next tallest person in the room" means Izzy is *shorter* than Chris.

I don't have a good alternative, though. "(the next older frame)"? Maybe this isn't a problem. Change it if you want.
Attachment #8580157 - Flags: review?(jorendorff) → review+
Created attachment 8583901 [details] [diff] [review]
Part 2: Add module level documentation for js/src/vm/SavedStacks.h
Attachment #8580157 - Attachment is obsolete: true
Attachment #8583901 - Flags: review+
Attachment #8582696 - Flags: review?(jorendorff) → review+
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fadf551562b8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/550a5c9e8800
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ba50784c810d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7304e101be54
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b336dc0af92c
Blocks: 981514
https://hg.mozilla.org/mozilla-central/rev/fadf551562b8
https://hg.mozilla.org/mozilla-central/rev/550a5c9e8800
https://hg.mozilla.org/mozilla-central/rev/ba50784c810d
https://hg.mozilla.org/mozilla-central/rev/7304e101be54
https://hg.mozilla.org/mozilla-central/rev/b336dc0af92c
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
We're not caching the stack string in any way anymore, right?
(In reply to Not doing reviews right now from comment #59)
> We're not caching the stack string in any way anymore, right?

No, we aren't. I imagine if repeatedly stringifying the SavedFrame stack becomes a problem, we can make a per-principals cache of some sort.
Whiteboard: [MemShrink]
You need to log in before you can comment on or make changes to this bug.