1029933 - Give Error Objects a ClassSpec

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

Same story as everywhere else - we need a ClassSpec in order to support them over Xrays.

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Bobby Holley (:bholley)]

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

Comment 3

[Bobby Holley (:bholley)]

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

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 3 - Introduce the concept of "dependent" standard classes and handle them in the ClassSpec infrastructure. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 4 - Introduce and use ParentKeyForStandardClass. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 5 - Tweak GenericCreateConstructor to make it usable with Error. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 6 - Make Error use a ClassSpec. v1

Comment 8

[Jeff Walden [:Waldo]]

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.

Comment 9

[Jeff Walden [:Waldo]]

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.

Comment 10

[Jeff Walden [:Waldo]]

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

Comment 11

[Jeff Walden [:Waldo]]

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.

Comment 12

[Jeff Walden [:Waldo]]

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.

Comment 13

[Bobby Holley (:bholley)]

Thanks for the quick reviews Waldo!

Final all-platform try push:

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

Comment 14

[Bobby Holley (:bholley)]

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

Comment 15

[Bobby Holley (:bholley)]

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

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 17

[Bobby Holley (:bholley)]

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

Comment 18

[Bobby Holley (:bholley)]

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

Comment 19

[Bobby Holley (:bholley)]

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

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 21

[Benjamin Bouvier [:bbouvier] (inactive)]

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

Comment 22

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2ec6c5b1d1d2