1002737 - General Cleanups to PropDesc

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[Eric Faust [:efaust]]

PropDesc has come about very organically. People have just added things as they were needed. Now that we want to expose it, we should probably add a bit more polish to the idea.

Comment 1

[Eric Faust [:efaust]]

Attachment: Cleanup: Find a new home for PropDesc::unwrapDebuggerObjectsInto

This also adds setters for a few internal fields, since this is no longer a function that can reach private fields.

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 8414072 [details] [diff] [review]
Cleanup: Find a new home for PropDesc::unwrapDebuggerObjectsInto

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

Stealing review. Looks great.

::: js/src/vm/Debugger.cpp
@@ +5357,5 @@
>              return false;
> +        Handle<PropDesc> wrapped = Handle<PropDesc>::fromMarkedLocation(&descs[i]);
> +        MutableHandle<PropDesc> unwrapped =
> +            MutableHandle<PropDesc>::fromMarkedLocation(&unwrappedDescs[i]);
> +        if (!dbg->unwrapPropDescInto(cx, obj, wrapped, unwrapped))

Can we make AutoPropDescArrayRooter::operator[] return a MutableHandle? Then the fromMarkedLocation call is (a) hidden (b) located in a place where it's obviously-correct. And this code ends up looking even nicer than it was before:

    if (!dbg->unwrapPropDescInto(cx, obj, descs[i], unwrappedDescs[i]))

Comment 3

[Eric Faust [:efaust]]

Attachment: Make AutoPropDescArrayRooter into AutoPropDescVector

> Can we make AutoPropDescArrayRooter::operator[] return a MutableHandle? Then
> the fromMarkedLocation call is (a) hidden (b) located in a place where it's
> obviously-correct. And this code ends up looking even nicer than it was
> before:

Ask and you shall receive ;)

Comment 4

[Eric Faust [:efaust]]

Attachment: Make PropDesc::setUndefined() imply more about the descriptor state

When you clear it, clear it. It's much easier to start from scratch every time.

Comment 5

[Eric Faust [:efaust]]

Attachment: Convert PropDesc::pd_ to a JSObject *

It's only ever either UndefinedValue() or ObjectValue(). We can do that better. And it simplifies projecting the object back out for later use.

Comment 6

[Eric Faust [:efaust]]

Attachment: Sprinkle asserts about definedness around

Comment 7

[Eric Faust [:efaust]]

Attachment: Try to better honor the concept of [[Origin]] by converting makeObject() to getOrCreateDescriptorObject()

Comment 8

[Eric Faust [:efaust]]

Attachment: move PropDesc::wrapInto to JSCompartment::wrap()

Comment 9

[Eric Faust [:efaust]]

Attachment: Random Cleanup: Fix DebugObject_defineProperties use of AutoPropDescVector(s)

Feel free to pass this to Jimb, if it's more fitting.

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 8414930 [details] [diff] [review]
Make AutoPropDescArrayRooter into AutoPropDescVector

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

Nice.

::: js/src/jsobjinlines.h
@@ +791,4 @@
>  {
>    public:
> +    explicit AutoPropDescVector(JSContext *cx
> +                    MOZ_GUARD_OBJECT_NOTIFIER_PARAM)

This doesn't seem to be indented properly. "MOZ_" should line up wtih "JSContext".

Comment 11

[Jason Orendorff [:jorendorff]]

Comment on attachment 8414935 [details] [diff] [review]
Try to better honor the concept of [[Origin]] by converting makeObject() to getOrCreateDescriptorObject()

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

Is this still a good idea if [[Origin]] gets backed out of ES6?

I think this is a behavior change and we should probably skip it.

Comment 12

[Jason Orendorff [:jorendorff]]

Comment on attachment 8414936 [details] [diff] [review]
move PropDesc::wrapInto to JSCompartment::wrap()

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

::: js/src/vm/PropDesc.h
@@ -280,5 @@
>      JSStrictPropertyOp setter() const { return desc()->setter(); }
> -
> -    // We choose not to expose the debugger-specific parts of PropDesc, both
> -    // because they are not really general use, but also because they are a
> -    // pain to expose.

Glad to see this go!

Comment 13

[Jason Orendorff [:jorendorff]]

Comment on attachment 8414937 [details] [diff] [review]
Random Cleanup: Fix DebugObject_defineProperties use of AutoPropDescVector(s)

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

::: js/src/vm/Debugger.cpp
@@ +5372,4 @@
>                  return false;
>          }
>  
>          ErrorCopier ec(ac, dbg->toJSObject());

Side note: ErrorCopier's constructor has a weird signature because when it was written, entering a compartment could fail, and ~ErrorCopier needed an infallible way to switch compartments. ac.destroy() was the solution.

Now entering a compartment is infallible and I think ErrorCopier could be revised to be less crazy.

@@ +5375,5 @@
>          ErrorCopier ec(ac, dbg->toJSObject());
>          for (size_t i = 0; i < n; i++) {
>              bool dummy;
> +            if (!DefineProperty(cx, obj, ids.handleAt(i),
> +                                descs[i], true, &dummy))

See if it'll fit on one line now.

Comment 14

[Eric Faust [:efaust]]

(In reply to Jason Orendorff [:jorendorff] from comment #13)
> Comment on attachment 8414937 [details] [diff] [review]

> Side note: ErrorCopier's constructor has a weird signature because when it
> was written, entering a compartment could fail, and ~ErrorCopier needed an
> infallible way to switch compartments. ac.destroy() was the solution.
> 
> Now entering a compartment is infallible and I think ErrorCopier could be
> revised to be less crazy.

Filed bug 1004088.

Comment 15

[Eric Faust [:efaust]]

Comment on attachment 8414937 [details] [diff] [review]
Random Cleanup: Fix DebugObject_defineProperties use of AutoPropDescVector(s)

Hey Jim, I keep missing you on IRC, but I just want to confirm that the three rooted arrays were just out of a habit of defensive programming, and not a major issue to unify.

Comment 16

[Jim Blandy :jimb]

Comment on attachment 8414937 [details] [diff] [review]
Random Cleanup: Fix DebugObject_defineProperties use of AutoPropDescVector(s)

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

As long as unwrapPropDescInto behaves properly when both its arguments point to the same memory, I don't see any problem with this. There shouldn't be any subtle magic going on in that code.

Comment 17

[Eric Faust [:efaust]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b82adb960c54
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c0dd6b9cc07a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ad09630ae9a3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6d4043272a0a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5afce70dad1f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8a63bad8faed
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ec411f0ce167

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Backed out for SM rootanalysis test failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b922ed24938f

https://tbpl.mozilla.org/php/getParsedLog.php?id=40980241&tree=Mozilla-Inbound

Comment 19

[Ryan VanderMeulen [:RyanVM]]

And Windows GC crashes a la:

https://tbpl.mozilla.org/php/getParsedLog.php?id=40983675&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=40983326&tree=Mozilla-Inbound

Comment 20

[Eric Faust [:efaust]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/98b5ebd7a3d2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/00481a443cf0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d1a3b8ee7c7e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c14b461b3c24
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a98a39b58a54
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1374cc1b03c0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4db799c3cee9

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/98b5ebd7a3d2
https://hg.mozilla.org/mozilla-central/rev/00481a443cf0
https://hg.mozilla.org/mozilla-central/rev/d1a3b8ee7c7e
https://hg.mozilla.org/mozilla-central/rev/c14b461b3c24
https://hg.mozilla.org/mozilla-central/rev/a98a39b58a54
https://hg.mozilla.org/mozilla-central/rev/1374cc1b03c0
https://hg.mozilla.org/mozilla-central/rev/4db799c3cee9