Boris Zbarsky [:bzbarsky] Reporter
	Description • 10 years ago

See some of the discussion in bug 1036527.  Spinning this out, since we can do this without actually fixing bug 1036527.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 1 • 10 years ago

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.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 2 • 10 years ago

OK.  So can we decide whether we want an accessor or a resolve hook here?  I'm happy to try hooking up either one...

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 3 • 10 years ago

Though if resolve hook, I'm not sure how that's supposed to interact with Xrays, exactly...

	Jason Orendorff [:jorendorff]
	Comment 4 • 10 years ago

(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.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 7 • 10 years ago

(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:

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 9 • 10 years ago

(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.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 12 • 10 years ago

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=65aee61f2ea2

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 13 • 10 years ago

: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.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 14 • 10 years ago

(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.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 15 • 10 years ago

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?

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 16 • 10 years ago

(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

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 18 • 10 years ago

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

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)
	Comment 19 • 10 years ago

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.

	Jason Orendorff [:jorendorff]
	Comment 20 • 10 years ago

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...

	Jason Orendorff [:jorendorff]
	Comment 21 • 10 years ago

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>());

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 23 • 10 years ago

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?

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 27 • 10 years ago

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?

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 28 • 10 years ago

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.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 29 • 10 years ago

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

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 31 • 10 years ago

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

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 33 • 10 years ago

(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.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 34 • 10 years ago

Ok, here is a new try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=faf34dd7458a

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 37 • 10 years ago

And another try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1483c51d4a32

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 42 • 10 years ago

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

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 43 • 10 years ago

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!

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)
	Comment 44 • 10 years ago

Comment on attachment 8580161 [details] [diff] [review]
combined.patch

Nothing found after spinning up some extra EC2 instances for testing this.

	Jason Orendorff [:jorendorff]
	Comment 45 • 10 years ago

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.

	Christian Holler (:decoder)
	Comment 48 • 10 years ago

Comment on attachment 8580161 [details] [diff] [review]
combined.patch

feedback+ from me as well, no bugs found.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 49 • 10 years ago

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.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 50 • 10 years ago

(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?

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 53 • 10 years ago

(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;
    }

	Jason Orendorff [:jorendorff]
	Comment 54 • 10 years ago

(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...

	Jason Orendorff [:jorendorff]
	Comment 55 • 10 years ago

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.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 57 • 10 years ago

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

	Phil Ringnalda (:philor)
	Comment 58 • 10 years ago

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

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 59 • 10 years ago

We're not caching the stack string in any way anymore, right?

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 60 • 10 years ago

(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.