Jason Orendorff [:jorendorff] Reporter
	Description • 12 years ago

var x = new Error;
assertEq(Object.getOwnPropertyNames(x).indexOf("stack") >= 0, true);  // FAIL


This is because js::ErrorClass has resolve hook but no enumerate hook.

js> var x = new Error; 
js> Object.getOwnPropertyNames(x)
[]
js> x.stack;
"@typein:1\n"
js> Object.getOwnPropertyNames(x)
["stack"]
js> x.lineNumber;
1
js> Object.getOwnPropertyNames(x)
["stack", "lineNumber"]

	Diego Vilar
	Comment 3 • 12 years ago

I came across this problem and found out that, interestingly, if you have firebug installed and add a "debugger" instruction (not a breakpoint) before the getOwnPropertyNames call and step over it, stack (and all properties) will be returned in the array.

If you use the console to type your code, it will work as well.

Notice that the problem described happens from Firefox 6 and beyond. It does not happen in Firefox 5 and 4.

	Jeff Walden [:Waldo] Assignee
	Comment 4 • 12 years ago

We should just make Error objects include all these properties ab initio, and do away with the resolve/enumerate hooks.  All the properties are trivially computable, so it wouldn't really lose any time -- indeed it'd sort of win time, because having these be present from the start means initializing them is just setting reserved slots.

The only confounding property is "stack", which might be slower to compute.  Maybe.  But then again, the work to create a JSExnPrivate is already pretty big.  If we just created the relevant properties directly, that could easily be faster by nature of eliminating the middleman.  We'd still need a private, or something like it, to store JSExnPrivate::errorReport, but other than that JSExnPrivate is just overhead.

	Jason Orendorff [:jorendorff] Reporter
	Comment 5 • 12 years ago

Agreed. Remove the hooks on Error and just eagerly initialize.

(Separately, it'd be great to capture the stack on throwing, but that is orthogonal to Error objects. It's on file somewhere.)

	Mark S. Miller
	Comment 6 • 12 years ago

Please make these non-standard properties configurable, and actually deletable, as per http://wiki.ecmascript.org/doku.php?id=conventions:make_non-standard_properties_configurable and the recent tc39 meeting discussion.

	Jeff Walden [:Waldo] Assignee
	Comment 7 • 12 years ago

Oh, right, resolve hooks can't do configurable properties correctly, at least not without pain to make sure they can't be re-resolved after deletion.  Yet another reason to do this.

I started on a patch this weekend; we'll see if I can finish it off soonish.

	Mark S. Miller
	Comment 8 • 11 years ago

What is the status of this? What other properties are detectably present if you know their names, but whose names are undiscoverable by getOwnPropertyNames? Iff we're confident we have a complete list, then Caja can repair getOwnPropertyNames to probe for these detectable but undiscoverable names. Otherwise, Caja treats this as an unknowable security hole, forcing us to translate rather than use SES.

	Jeff Walden [:Waldo] Assignee
	Comment 9 • 11 years ago

The status right now, for me, is internationalization API work is more important.  And, realistically, I'm pretty sure Intl functionality ranks rather higher than SES on the things-to-care-about scale for JS authors in general.  My partial patch for this bug is still sitting in my queue, unfinished and not yet compiling, waiting for when I have time to finish it.

For "what other", it's a class-by-class thing -- the hook to enumerate properties, and the hook to add lazily defined properties, have to be in semantic agreement about these things.  If they are, all good.  If not, you have various issues.  Probably this question of semantic agreement is reducible to the halting problem, although heuristic shortcuts might get you close to a correct answer for every hook pair used, without requiring manual inspection of all of them.  I'd be somewhat surprised if the same situation didn't exist in other engines in one form or another, excepting perhaps that they anticipated the issue when assigning hooks, rather than having to somewhat retrofit like we've done to an existing system.

	Mark S. Miller
	Comment 10 • 11 years ago

With bug 674195 apparently fixed (thanks!), this is now the only outstanding issue causing Caja to classify FF as needing the expensive ES5-to-ES3 translator rather than the efficient SES-on-ES5 system. Any news?

	Mark S. Miller
	Comment 11 • 11 years ago

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #9)
> For "what other", it's a class-by-class thing -- the hook to enumerate
> properties, and the hook to add lazily defined properties, have to be in
> semantic agreement about these things.  If they are, all good.  If not, you
> have various issues.  Probably this question of semantic agreement is
> reducible to the halting problem, although heuristic shortcuts might get you
> close to a correct answer for every hook pair used, without requiring manual
> inspection of all of them.

Hi Jeff, this is only an urgent issue for us regarding non-host objects, i.e., the kinds of objects which appear in the ES5 & ES6 specs directly. SES generally denies direct access to host objects, so surprising and undiscoverable properties on host objects are not fatal to SES's security.

Do you expect that there may be any undiscoverable properties in FF non-host objects, other than the Error properties that this issue is about? Could we get a complete list of the property names we need to check for on Error objects, so that we detect all that might be present but not enumerated by getOwnPropertyNames? Thanks.

	Jeff Walden [:Waldo] Assignee
	Comment 12 • 11 years ago

(In reply to Mark S. Miller from comment #11)
> Do you expect that there may be any undiscoverable properties in FF non-host
> objects, other than the Error properties that this issue is about? Could we
> get a complete list of the property names we need to check for on Error
> objects, so that we detect all that might be present but not enumerated by
> getOwnPropertyNames? Thanks.

I don't think there are any others.  The lazy-property kinds of objects I can think of (most aren't, there's no reason for them to be), built into SpiderMonkey, are Error objects, Function objects, and String objects.  String objects' hooks are self-consistent.  Same for Function objects.

The set of Error object properties that get lazily added are "message", "fileName", "lineNumber", "columnNumber", and "stack".

	Jeff Walden [:Waldo] Assignee
	Comment 13 • 11 years ago

This is what I'd finished up, the last time I looked at this, several months ago.  This also includes adding BaseErrorObject and other Error object subclasses.  It still needs work to be correct -- some stuff here's done, other parts need tweaking, other parts haven't yet been touched.  Plus I've been rebasing it for several months, and it's quite possible my rebasing has introduced additional small errors -- likely nothing too difficult, but still things to work through.  Anyway, just putting it out there as a minor status update.

	Jeff Walden [:Waldo] Assignee
	Comment 16 • 11 years ago

There's particular overlap with stuff needed/used by the debugger (like error copying), so let's try jimb for review.  A few-hours-old iteration of this passed debug jstests/jit-tests/jsapi-tests, and the obvious build errors pointed out by a try run are hopefully fixed, so this is probably good to go now.

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

	Jeff Walden [:Waldo] Assignee
	Comment 17 • 11 years ago

Once more, with feeling!

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

	Jeff Walden [:Waldo] Assignee
	Comment 18 • 11 years ago

And an extra bit (it's short!) to hopefully fix a debugger test that expects errors' properties to be present in a certain order, and to remove debugger hackarounds for essentially this bug:

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

Will post a folded patch in the morning when that's gone green.

	Jeff Walden [:Waldo] Assignee
	Comment 19 • 11 years ago

Seeing green Xs, so this is legit.

	Brian Hackett [Laid off!]
	Comment 20 • 11 years ago

Comment on attachment 8335932 [details] [diff] [review]
Bug-free-for-reals patch

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

r+ for the AddTypeProperty stuff.  Is there more you wanted me to look at?

	Brian Hackett [Laid off!]
	Comment 21 • 11 years ago

Here's a push to add a comment to that section fwiw, to make future modifications less cargo culty.

https://hg.mozilla.org/integration/mozilla-inbound/rev/69f7f35a53f2

	Ryan VanderMeulen [:RyanVM]
	Comment 22 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/69f7f35a53f2

	Jeff Walden [:Waldo] Assignee
	Comment 23 • 11 years ago

After poking on IRC about whether "AddTypeProperty" covered only the jsinfer.cpp changes or also the bit on ErrorObject.cpp, we determined this was a sufficient tweak to cover that bit as well.

There was also this:

<bhackett> Waldo: looks better, you could make it an initReservedSlotWithType if you wanted, it's just better if it's internal to the object api
<Waldo> bhackett: this would be adding that method, with an obvious definition? no such method right now; if we added that, tho, couldn't we get rid of the hacks in jsinfer.cpp totally, and have String/RegExp/errors all use initReservedSlotWithType? (followup territory of course)

No response on that yet, but as noted we can do that later, and if this delta's adequate to address concerns, slightly more niceness (and less fugly, maybe) can happen later.

Obviously I'll fold the two patches together when pushing.

	Jim Blandy :jimb
	Comment 24 • 11 years ago

Comment on attachment 8335932 [details] [diff] [review]
Bug-free-for-reals patch

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

The devtools and object metadata test patches look fine. Just one bit that wants a bit more explanation.

::: js/src/jit-test/tests/basic/metadata-hook.js
@@ +13,5 @@
> +    var res =
> +      {
> +        count: ++count,
> +        location: Error().stack,
> +        message: Error().message // no .message => Error.prototype.message => ""

I found this comment and property cryptic; there's no indication of what is really being tested here.

Please add a brief comment to the effect that TI was unhappy with the Error().stack evaluation, in a way that wasn't easily reproducible in other contexts; and that TI had similar problems with Error().message; and that checking them here seemed like the quickest way to cover both cases.

::: toolkit/devtools/server/actors/script.js
@@ -2906,5 @@
>  
>    /**
> -   * Force the magic Error properties to appear.
> -   */
> -  _forceMagicProperties: function () {

So glad to see this go!

(ahem)

	Jim Blandy :jimb
	Comment 25 • 11 years ago

Comment on attachment 8335932 [details] [diff] [review]
Bug-free-for-reals patch

Realized I'm not done with this review yet...

	Jim Blandy :jimb
	Comment 26 • 11 years ago

Comment on attachment 8335932 [details] [diff] [review]
Bug-free-for-reals patch

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

Some more review-in-progress comments:

::: js/src/jsexn.cpp
@@ +46,3 @@
>  const Class ErrorObject::class_ = {
>      js_Error_str,
> +    JSCLASS_IMPLEMENTS_BARRIERS | JSCLASS_NEW_RESOLVE |

We should remove JSCLASS_NEW_RESOLVE from here, right?

::: js/src/vm/ErrorObject.cpp
@@ +34,5 @@
> +{
> +    // Null out early in case of error, for exn_finalize's sake.
> +    obj->initReservedSlot(ERROR_REPORT_SLOT, PrivateValue(nullptr));
> +
> +    if (obj->nativeEmpty()) {

Could we have a one-line comment here saying, roughly, "This means we need to construct the initial Error shape for this compartment"?

@@ +38,5 @@
> +    if (obj->nativeEmpty()) {
> +        RootedShape shape(cx, ErrorObject::assignInitialShapeNoMessage(cx, obj));
> +        if (!shape)
> +            return false;
> +        if (!obj->isDelegate()) {

What's the isDelegate check there for? Wouldn't the Error prototype object always be the first instance of the Class created in a given compartment?

@@ +78,5 @@
> +    if (message)
> +        obj->initReservedSlot(MESSAGE_SLOT, StringValue(message));
> +
> +    if (errorReport && errorReport->originPrincipals)
> +        JS_HoldPrincipals(errorReport->originPrincipals);

It seems like the ErrorReport constructor / initializer ought to take care of holding and dropping the principals.

::: js/src/vm/ErrorObject.h
@@ +21,5 @@
>  
>  class ErrorObject : public JSObject
>  {
> +    static ErrorObject *
> +    createProto(JSContext *cx, JS::Handle<GlobalObject*> global, int type, JS::HandleObject proto);

Can we use JSExnType instead of int for type? Or would that just create a castfest everywhere?

(If it touches a lot of code that this patch doesn't touch already, I'd prefer to review that as a separate patch.)

@@ +23,5 @@
>  {
> +    static ErrorObject *
> +    createProto(JSContext *cx, JS::Handle<GlobalObject*> global, int type, JS::HandleObject proto);
> +
> +    /* For access to createNativeProto. */

I think you mean 'createProto', right?

	Jeff Walden [:Waldo] Assignee
	Comment 27 • 11 years ago

(In reply to Jim Blandy :jimb from comment #26)
> We should remove JSCLASS_NEW_RESOLVE from here, right?

Yeah, done.

> Could we have a one-line comment here saying, roughly, "This means we need
> to construct the initial Error shape for this compartment"?

Sure.

> What's the isDelegate check there for? Wouldn't the Error prototype object
> always be the first instance of the Class created in a given compartment?

This might be vestigial from the original RegExp/String code I used as template.  For consistency I'd like to keep this as-is, then fix/simplify in a followup.  jstests and jit-tests both pass with that condition asserted true, so it's probably doable.  But I don't know for certain, and best not to hold this up given people need this to work atop.  They won't be affected by this tweak, I'm sure.

> It seems like the ErrorReport constructor / initializer ought to take care
> of holding and dropping the principals.

Looks fair.  But also followup territory, I suspect it'd go afield of what's in this patch, and I don't know enough about the principals bits here to be confident we could just make this change and nothing would break.

> Can we use JSExnType instead of int for type? Or would that just create a
> castfest everywhere?

Nope, just stuff in this patch, simply changed.

> I think you mean 'createProto', right?

Ah, yes.  Naming vacillations...

	Jeff Walden [:Waldo] Assignee
	Comment 28 • 11 years ago

Actually, no -- the proto/insertInitialShape stuff has since been changed in the String/RegExp.  I'll make that tweak here.

	Jeff Walden [:Waldo] Assignee
	Comment 29 • 11 years ago

Er, no, I'm blind, and the proto stuff is still present in current String/RegExp code.  Followup.

	Jeff Walden [:Waldo] Assignee
	Comment 30 • 11 years ago

So, I think the current caching setup is fairly confused.

It's trying to not cache if it's creating the class prototype objects -- by checking isDelegate.  But the delegate bit isn't set at the time the object is created, it's only set when that object's used as a prototype.  So no nativeEmpty() object will have isDelegate() ever be true, so both String.prototype and new String() will both be nativeEmpty() (the first time through, for the latter) and will cache an initial shape.

The fix is to change the central location where all but the most primordial prototype objects are created, to mark them as delegates from birth.  Then we really can do it the way the current code thinks things are happening.

This is mostly fixing the current code to do it the slightly-shorter way the Error code does it in the patch here, in the String and RegExp cases, plus adding beefier comments explaining exactly how the code here works.  I stepped through in a debugger for a few different cases, and initial shape caches were being hit, and not-hit, and so on when I expected, including after manual GC that would empty out the reusable initial shapes.

	Brian Hackett [Laid off!]
	Comment 31 • 11 years ago

Comment on attachment 8342199 [details] [diff] [review]
Fix initial-shape-caching

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

::: js/src/vm/ErrorObject.cpp
@@ +53,3 @@
>              RootedObject proto(cx, obj->getProto());
>              EmptyShape::insertInitialShape(cx, shape, proto);
>          }

Can you common out the logic shared between here and the regexp/string methods, maybe with a template?  While the similar logic is ok it'd be nice to avoid the copy-pasted comments.  If the logic is too annoying to common out these comments could go in one place I guess (maybe around insertInitialShape?) with pointers to that spot from these blocks.

	Brian Hackett [Laid off!]
	Comment 33 • 11 years ago

Comment on attachment 8342678 [details] [diff] [review]
Take two

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

Thanks!

	Jim Blandy :jimb
	Comment 34 • 11 years ago

Comment on attachment 8335932 [details] [diff] [review]
Bug-free-for-reals patch

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

Looks good! Some minor comments.

::: js/src/jsexn.cpp
@@ +204,5 @@
>      }
>  };
>  
> +/* XXXbe Consolidate the ugly truth that we don't treat filename as UTF-8
> +         with these two functions. */

... except it doesn't, because we still handle filenames directly when producing the stack trace... Is FilenameToString worth keeping?

@@ +239,2 @@
>                  if (atom == nullptr)
> +                    atom = cx->names().empty;

Couldn't this simply become:

if (i.isNonEvalFunctionFrame() && i.callee()->displayAtom())
  atom = i.callee()->displayAtom()

? The 'empty' bit is redundant now.

@@ +264,5 @@
> +
> +            /*
> +             * Cut off the stack if it gets too deep (most commonly for
> +             * infinite recursion errors).
> +             */

The original code tried to pre-reserve string space, and truncate the stack if it failed, rather than OOM-ing. I took that to be an acknowledgement that filename length and function name length are under the content's control. Not worth keeping?

@@ +892,5 @@
> +
> +    // We really want a unique_ptr-style class for copyReport, but this will
> +    // suffice for now.
> +    if (obj)
> +        copyReport.forget();

Would it be nicer for ErrorObject::create and ErrorObject::init to take the report as a ScopedJSFreePtr<JSErrorReport> &, and do the 'forget' themselves, at the exact moment that the error object takes ownership of the report?

::: js/src/jsexn.h
@@ +14,4 @@
>  #include "jsapi.h"
>  #include "NamespaceImports.h"
>  
> +#include "vm/ErrorObject.h"

Do we need to #include this, actually?  We want to keep inter-header dependencies to a minimum, as I understand it. It seems like there aren't too many places that need js_InitExceptionClasses' declaration, and those could include vm/ErrorObject.h themselves.

::: js/src/jswrapper.cpp
@@ -149,5 @@
>      JSContext *cx = ac.ref().context()->asJSContext();
>      if (ac.ref().origin() != cx->compartment() && cx->isExceptionPending()) {
>          RootedValue exc(cx, cx->getPendingException());
> -        if (exc.isObject() && exc.toObject().is<ErrorObject>() &&
> -            exc.toObject().as<ErrorObject>().getExnPrivate())

So this code used to avoid copying prototypes, but now there's no longer any need to do so? That's nice.

::: js/src/vm/ErrorObject.h
@@ +44,5 @@
> +    static const uint32_t COLUMNNUMBER_SLOT = LINENUMBER_SLOT + 1;
> +    static const uint32_t STACK_SLOT        = COLUMNNUMBER_SLOT + 1;
> +    static const uint32_t MESSAGE_SLOT      = STACK_SLOT + 1;
> +
> +    static const uint32_t RESERVED_SLOTS = MESSAGE_SLOT + 1;

Let's use an enum here!

@@ +76,5 @@
> +        return getReservedSlot(LINENUMBER_SLOT).toInt32();
> +    }
> +
> +    uint32_t columnNumber() const {
> +        return getReservedSlot(LINENUMBER_SLOT).toInt32();

COLUMNNUMBER_SLOT

(EARNING MY PATCH REVIEW PAY TODAY)

::: js/src/vm/GlobalObject.h
@@ +413,2 @@
>          JSProtoKey key = GetExceptionProtoKey(exnType);
> +        return &getPrototype(key).toObject();

The call to getPrototype(key) uses 'this', doesn't it? Isn't it a GC hazard, if js_InitExceptionClasses relocates 'this'? I gather that's why the old code was written the way it was.

	Jeff Walden [:Waldo] Assignee
	Comment 35 • 11 years ago

After:

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

We now have:

https://hg.mozilla.org/integration/mozilla-inbound/rev/54eac2d5c039
https://hg.mozilla.org/integration/mozilla-inbound/rev/479975fcd736

(In reply to Jim Blandy :jimb from comment #34)
> ... except it doesn't, because we still handle filenames directly when
> producing the stack trace... Is FilenameToString worth keeping?

Agreed.  I just didn't feel like potentially rocking the boat to get rid of it, happy to do so.  :-)

> Couldn't this simply become:

Hm, yes.

> @@ +264,5 @@
> > +
> > +            /*
> > +             * Cut off the stack if it gets too deep (most commonly for
> > +             * infinite recursion errors).
> > +             */
> 
> The original code tried to pre-reserve string space, and truncate the stack
> if it failed, rather than OOM-ing. I took that to be an acknowledgement that
> filename length and function name length are under the content's control.

Well, sort of.  The previous |!sb.reserve(length)| check is wrong in two ways.  First, if that happens, an exception was thrown -- you can't just silently break and continue on in this case any more.  Second, it misunderstands the |reserve| API -- which takes the overall amount to reserve, not an additional increment.  So these reserve calls, after the first one, did basically nothing.

OOM is exceptional.  Super-long names of files or functions are exceptional.  I don't think we should care about less-than-perfect grace here.

> Would it be nicer for ErrorObject::create and ErrorObject::init to take the
> report as a ScopedJSFreePtr<JSErrorReport> &, and do the 'forget'
> themselves, at the exact moment that the error object takes ownership of the
> report?

Ideally with rvalue references, but makes sense.  In the absence of those for the moment, I'm having it pass by pointer so that there's no scattering of not-clearly-correct call sites.  (Which is basically what happens under the hood with pass-by-&&, really, isn't it -- except for the ultimate transfer being spelled differently.)

> > +#include "vm/ErrorObject.h"
> 
> Do we need to #include this, actually?  We want to keep inter-header
> dependencies to a minimum, as I understand it. It seems like there aren't
> too many places that need js_InitExceptionClasses' declaration, and those
> could include vm/ErrorObject.h themselves.

jsexn.h is a legacy header, lots of problems with what it includes, and with who includes it, in my book.  So when I ran into the problem requiring that, I didn't think too much about just adding it.  But you're right, it's easy to make those few users include vm/ErrorObject.h themselves -- done.

> So this code used to avoid copying prototypes, but now there's no longer any
> need to do so? That's nice.

I didn't read the semantics too closely, but I think so, yeah.

> ::: js/src/vm/ErrorObject.h
> @@ +44,5 @@
> > +    static const uint32_t COLUMNNUMBER_SLOT = LINENUMBER_SLOT + 1;
> > +    static const uint32_t STACK_SLOT        = COLUMNNUMBER_SLOT + 1;
> > +    static const uint32_t MESSAGE_SLOT      = STACK_SLOT + 1;
> > +
> > +    static const uint32_t RESERVED_SLOTS = MESSAGE_SLOT + 1;
> 
> Let's use an enum here!

Sequential static const uint32_t are ubiquitous for this, sensible because historically you couldn't control an enum's underlying type, and because slot numbers are by definition uint32_t.  Let's be consistent with that here, an enum doesn't buy enough.

> > +    uint32_t columnNumber() const {
> > +        return getReservedSlot(LINENUMBER_SLOT).toInt32();
> 
> COLUMNNUMBER_SLOT

Ahem.  It's rather disconcerting that apparently no tests fail because of this.  I wrote a test that fails because of this, passes with.  (Although, it appears our column number information is just generally all wrong, so that test checks for a columnNumber 0 and a lineNumber non-zero, with a comment by it saying the column number's all wrong, but at least the test does what it intended to do, and the zero can be changed whenever the column numbering code is fixed.)

> The call to getPrototype(key) uses 'this', doesn't it? Isn't it a GC hazard,
> if js_InitExceptionClasses relocates 'this'? I gather that's why the old
> code was written the way it was.

Hm, maybe, reverted that bit to not trouble trouble.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 36 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/54eac2d5c039
https://hg.mozilla.org/mozilla-central/rev/479975fcd736

	Florian Scholz (Open Web Docs)
	Comment 37 • 10 years ago

https://developer.mozilla.org/en-US/Firefox/Releases/28#JavaScript
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/getOwnPropertyNames#SpiderMonkey-specific_notes