If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Give Error Objects a ClassSpec

RESOLVED FIXED in mozilla33

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla33
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments)

Same story as everywhere else - we need a ClassSpec in order to support them over Xrays.
https://tbpl.mozilla.org/?tree=Try&rev=c8fa5a36dfdd
Created attachment 8449889 [details] [diff] [review]
Part 1 - Give BackstagePass an Enumerate hook to match its NewResolve hook. v1

This is required in order to avoid exposing resolve hook effects when
Object.freeze() is invoked on the global. The freeze() call first enumerates
the object, after which point any lazy properties need to be resolve so that
we can safely mark the object as non-extensible.
Attachment #8449889 - Flags: review?(jwalden+bmo)
Created attachment 8449890 [details] [diff] [review]
Part 2 - Tag JSStdName entries by JSProtoKey rather than a js::Class pointer. v1

The former is strictly more information, which matters in the case of Error
objects where multiple JSProtoKeys share a js::Class.
Attachment #8449890 - Flags: review?(jwalden+bmo)
Created attachment 8449891 [details] [diff] [review]
Part 3 - Introduce the concept of "dependent" standard classes and handle them in the ClassSpec infrastructure. v1
Attachment #8449891 - Flags: review?(jwalden+bmo)
Created attachment 8449892 [details] [diff] [review]
Part 4 - Introduce and use ParentKeyForStandardClass. v1
Attachment #8449892 - Flags: review?(jwalden+bmo)
Created attachment 8449893 [details] [diff] [review]
Part 5 - Tweak GenericCreateConstructor to make it usable with Error. v1
Attachment #8449893 - Flags: review?(jwalden+bmo)
Created attachment 8449894 [details] [diff] [review]
Part 6 - Make Error use a ClassSpec. v1
Attachment #8449894 - Flags: review?(jwalden+bmo)
Comment on attachment 8449889 [details] [diff] [review]
Part 1 - Give BackstagePass an Enumerate hook to match its NewResolve hook. v1

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

::: dom/workers/RuntimeService.cpp
@@ +1082,5 @@
>      }
>    }
>  
> +  // Invoking this function with JSID_VOID means "always resolve".
> +  bool shouldResolve = aId != JSID_VOID;

JSID_IS_VOID(aId)

::: dom/workers/Workers.h
@@ +171,5 @@
>  
>  // All of these are implemented in RuntimeService.cpp
> +
> +// Resolves all of the worker classes onto |aObjp| if one of them matches |aId|
> +// or if |aId| is JSID_VOID.

A little fugly, but okay.
Attachment #8449889 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8449890 [details] [diff] [review]
Part 2 - Tag JSStdName entries by JSProtoKey rather than a js::Class pointer. v1

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

I want to get rid of this error class sharing at some point, but this is fine in any event.
Attachment #8449890 - Flags: review?(jwalden+bmo) → review+
Attachment #8449891 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8449892 [details] [diff] [review]
Part 4 - Introduce and use ParentKeyForStandardClass. v1

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

::: js/src/jsfriendapi.h
@@ +633,5 @@
>      return key != keyFromClass;
>  }
>  
> +// Returns the key for the class inherited by a given standard class (that
> +// is to say, the prototype of this standard classes's prototype).

class's
Attachment #8449892 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8449893 [details] [diff] [review]
Part 5 - Tweak GenericCreateConstructor to make it usable with Error. v1

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

::: js/src/vm/GlobalObject.h
@@ +795,5 @@
>  GenericCreateConstructor(JSContext *cx, JSProtoKey key)
>  {
> +    // Note - We duplicate the trick from ClassName() so that we don't need to
> +    // include jsatominlines.h here.
> +    JSAtom *atom = (&cx->names().Null)[key];

PropertyName* please.
Attachment #8449893 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8449894 [details] [diff] [review]
Part 6 - Make Error use a ClassSpec. v1

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

::: js/src/jsexn.cpp
@@ +57,5 @@
> +static const JSFunctionSpec exception_methods[] = {
> +#if JS_HAS_TOSOURCE
> +    JS_FN(js_toSource_str,   exn_toSource,           0,0),
> +#endif
> +    JS_FN(js_toString_str,   exn_toString,           0,0),

Don't bother trying to column-align these, they get wrong easily.  Let's take the advantage to discard the alignment fetish.

@@ +546,2 @@
>  {
> +    JSExnType type = ExnTypeFromProtoKey(key);

I'd rather you deferred this til just before it's needed, as it was before, just vice versa.

@@ +548,2 @@
>      RootedObject errorProto(cx);
> +    errorProto = GenericCreatePrototype<&ErrorObject::class_>(cx, key);

This probably all fits into a single init-at-construction line.

@@ +571,5 @@
> +/* static */ JSObject *
> +ErrorObject::createConstructor(JSContext *cx, JSProtoKey key)
> +{
> +    RootedObject ctor(cx);
> +    ctor = GenericCreateConstructor<Error, 1, JSFunction::ExtendedFinalizeKind>(cx, key);

Again might fit on one line.
Attachment #8449894 - Flags: review?(jwalden+bmo) → review+
Thanks for the quick reviews Waldo!

Final all-platform try push:

https://tbpl.mozilla.org/?tree=Try&rev=5cc932b1916e
Followup for build bustage on MSVC and GCC due to the lack of support for default template parameters on functions:

https://tbpl.mozilla.org/?tree=Try&rev=6594e7988d24
https://tbpl.mozilla.org/?tree=Try&rev=c2b4a6fd521f
Comment on attachment 8449890 [details] [diff] [review]
Part 2 - Tag JSStdName entries by JSProtoKey rather than a js::Class pointer. v1

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

::: js/src/jsapi.cpp
@@ +1301,5 @@
>  
>      // If this class is anonymous, then it doesn't exist as a global
>      // property, so we won't resolve anything.
> +    JSProtoKey key = stdnm ? stdnm->key : JSProto_Null;
> +    if (key != JSProto_Null && !(ProtoKeyToClass(key)->flags & JSCLASS_IS_ANONYMOUS)) {

if (stdnm && stdnm->key != JSProto_Null && !(stdnm->clasp->flags & JSCLASS_IS_ANONYMOUS))?
(In reply to :Ms2ger from comment #16)
> if (stdnm && stdnm->key != JSProto_Null && !(stdnm->clasp->flags &
> JSCLASS_IS_ANONYMOUS))?

I removed the clasp member of stdnm entries in this patch.
https://tbpl.mozilla.org/?tree=Try&rev=3b07793a77a0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4ec31a870cb0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/650197ade3b3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3ff6b5e30818
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/30c45b56a2ef
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a7b254ac4fc4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7750f3e24883
https://hg.mozilla.org/mozilla-central/rev/4ec31a870cb0
https://hg.mozilla.org/mozilla-central/rev/650197ade3b3
https://hg.mozilla.org/mozilla-central/rev/3ff6b5e30818
https://hg.mozilla.org/mozilla-central/rev/30c45b56a2ef
https://hg.mozilla.org/mozilla-central/rev/a7b254ac4fc4
https://hg.mozilla.org/mozilla-central/rev/7750f3e24883
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Created attachment 8451500 [details] [diff] [review]
Remove unused DummyClass and SentinelClass

Warnings introduced by part2:

In file included from /home/ben/code/moz/builds/wd64/js/src/Unified_cpp_js_src7.cpp:41:0:
/home/ben/code/moz/inbound/js/src/jsapi.cpp:1198:18: warning: ‘DummyClass’ defined but not used [-Wunused-variable]
 static js::Class DummyClass;
                  ^
/home/ben/code/moz/inbound/js/src/jsapi.cpp:1199:18: warning: ‘SentinelClass’ defined but not used [-Wunused-variable]
 static js::Class SentinelClass;
                  ^

Easy to fix, and it compiles fine.
Attachment #8451500 - Flags: review?(bobbyholley)
Comment on attachment 8451500 [details] [diff] [review]
Remove unused DummyClass and SentinelClass

I had the same idea:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ec6c5b1d1d2
Attachment #8451500 - Flags: review?(bobbyholley)
https://hg.mozilla.org/mozilla-central/rev/2ec6c5b1d1d2
You need to log in before you can comment on or make changes to this bug.