875611 - Move Debugger objects reflections from private fields to reserved fields.

Status: RESOLVED DUPLICATE of bug 1722872

(Core :: JavaScript Engine, defect)

Description

[Matt Stults]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0 (Beta/Release)
Build ID: 20130326150557

Comment 1

[Matt Stults]

(This bug was suggested by ejpbruel)

Move Debugger objects reflections from private fields to reserved fields.  We want to move the reflections to reserved fields from private fields because:
1) There are up to 16 reserved slots and only one private slot available.
2) Normal (non-magic and non-private) are automatically handled by GC.
3) Because reserved fields are typed, they make debugging easier.

The objects that need to be refactored are:
1) Debugger.Environment
2) Debugger.DebuggeeValue

The Debugger.Script object does not need its "referent" moved out of its private field.  This is because the Debugger.Script referent is a JSScript, and JSScripts do not extend Value, and so cannot be placed directly in reserved fields.

Comment 2

[Matt Stults]

Attachment: First Pass

Let me know if this is what you're looking for or if I've missed any debug objects.  I've tested using:
jit-test/jit_test.py build-release/js --tbpl
tests/jstests.py build-release/js --args="-m -n"

Thanks for your time.

Comment 3

[Eddy Bruel [:ejpbruel]]

Hey Matt.

First of all, thank you very much for taking time for this. I'm trying to finish a project before the end of this week, so I'll take a look at your patch this friday.

For future reference, it normally takes a reviewer a couple of days at most to take a look at your patch, though sometimes it can take longer (it depends on how busy that person is and how high priority your patch).

If you haven't gotten any response to your patch, its perfectly acceptable to ping the reviewer and ask for an update :-)

Comment 4

[Eddy Bruel [:ejpbruel]]

Comment on attachment 754331 [details] [diff] [review]
First Pass

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

Sorry this took so long. This looks very good! r+ from me, provided you fix that single comment I had.

Normally, you'd be done now, and you could either check in the patch, or, if you don't have commit rights yet, request somebody else to check it in for you (works the same way as requesting a review, except you use the checkin flag).

However, you need a r+ from the module owner before you can check something in, and unfortuntately, that's not me in this case. I've asked for an additional review from Jim Blandy. He's the module owner for the debugger. Once you get a review from him, you're good to go.

Just so you know, I will be on PTO for the next 3 weeks. During that time, you can direct your questions to Jim (jimb on irc). Feel free to e-mail me if you're having trouble with anything, but I can't promise I'll be able to respond.

::: js/src/vm/Debugger.cpp
@@ +739,5 @@
>                                   : JSMSG_DEBUG_OBJECT_WRONG_OWNER);
>              return false;
>          }
>  
> +        vp.setObject(*static_cast<JSObject*>(dobj->getReservedSlot(JSSLOT_DEBUGOBJECT_REFERENT).toObjectOrNull()));

You don't need the static_cast here anymore, since .toObjectOrNull() already returns as JSObject *

Comment 5

[Matt Stults]

Attachment: Removing unnecessary static cast.

Thanks for the feedback!  I've made the suggested change and included Jim as the reviewer.  Please let me know if there's anything else I can do for the patch.

Comment 6

[Jim Blandy :jimb]

Comment on attachment 757068 [details] [diff] [review]
Removing unnecessary static cast.

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

It's great to see this getting done.

I think this patch has pieces missing; see the comments. I've cleared the review flag, waiting for an updated, complete patch.

::: js/src/vm/Debugger.cpp
@@ +1569,5 @@
>       * frames.)
>       */
>      for (FrameMap::Range r = frames.all(); !r.empty(); r.popFront()) {
>          RelocatablePtrObject &frameobj = r.front().value;
> +        JS_ASSERT(frameobj->getReservedSlot(JSSLOT_DEBUGFRAME_REFERENT));

I don't see any definition for JSSLOT_DEBUGFRAME_REFERENT; is this the whole patch?

@@ +4086,5 @@
>      NULL,                 /* checkAccess */
>      NULL,                 /* call        */
>      NULL,                 /* construct   */
>      NULL,                 /* hasInstance */
> +    NULL                  /* trace       */

You can just delete all the trailing NULL initialization values.

@@ +4092,5 @@
>  
>  static JSObject *
> +DebuggerObject_getReferent(JSObject *obj)
> +{
> +    return obj->getReservedSlot(JSSLOT_DEBUGOBJECT_REFERENT).toObjectOrNull();

These accessors are great! But they should have:

JS_ASSERT(obj->getClass() == &DebuggerObject_class);

@@ +4870,5 @@
>      NULL,                 /* checkAccess */
>      NULL,                 /* call        */
>      NULL,                 /* construct   */
>      NULL,                 /* hasInstance */
> +    NULL,                 /* trace       */

Same here.

@@ +4876,5 @@
>  
> +static Env *
> +DebuggerEnv_getReferent(JSObject *obj)
> +{
> +    return static_cast<Env *>(obj->getReservedSlot(JSSLOT_DEBUGENV_REFERENT).toObjectOrNull());

Analogous assertion needed here, too.

Comment 7

[Matt Stults]

Attachment: Updating with the correct patch.

This should have the suggested changes--let me know if I missed anything.

JSSLOT_DEBUGFRAME_REFERENT was left from a previous version of the patch--looks like I grabbed the wrong one.

I've tested this with:
tests/jstests.py build-release/js --args="-m -n"
jit-test/jit_test.py build-release/js
and it looks good.  I've also compiled into Firefox and run the debugger to make sure there are no blatant issues, but this is about as far as my verification has gone.  Let me know if there's anything else I can do.

Comment 8

[Jim Blandy :jimb]

Comment on attachment 758374 [details] [diff] [review]
Updating with the correct patch.

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

Looks great. Just one suggestion, but I can take care of that myself when I land it.

Thanks for the patch!

::: js/src/vm/Debugger.cpp
@@ +739,5 @@
>                                   : JSMSG_DEBUG_OBJECT_WRONG_OWNER);
>              return false;
>          }
>  
> +        vp.setObject(*dobj->getReservedSlot(JSSLOT_DEBUGOBJECT_REFERENT).toObjectOrNull());

Since vp is a MutableValueHandle, and getReservedSlot returns a Value, I think this can be, simply:

vp.set(dobj->getReservedSlot(JSSLOT_DEBUGOBJECT_REFERENT));

Comment 9

[Matt Stults]

Hey Jimb, thanks again for reviewing this!  Let me know if I should make the change and get someone to check this in or if--what I gather from your previous comment--you're going to submit it with the change yourself.  Just making sure I tie up any remaining loose ends on this before moving on to something else.

Comment 10

[Chris Peterson [:cpeterson]]

jimb: did you forget to land Matt's r+'d patch from last year? Is it still relevant?

Comment 11

[Jim Blandy :jimb]

Oh, wow, I didn't realize he wasn't able to land it himself. This may be bit-rotted by now. Let me try to refresh it.

Comment 12

[Jim Blandy :jimb]

Actually, now that I've updated this patch and tried it out: How in the world did this patch manage to store cross-compartment edges directly in object slots?

The patch needs to use private values (i.e. PrivateValue) to store these edges, with all the appropriate casts, because a Debugger.Object's referent is usually in another compartment, and direct cross-compartment edges are forbidden.

If Matt still has any interest in this patch, I'd be happy to help him bring it forward. But this isn't something I should prioritize at the moment.

Comment 13

[Matt Stults]

I think it has been almost a year from when I worked on this and I no longer have the context to work on this efficiently.  I'm happy with dropping this patch altogether and letting someone else work on the problem.  Just let me know if there's anything I need to do to clean up and close this patch out.

I'm sorry it didn't work out for you out of the box.

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:sdetar, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 15

[Jan de Mooij [:jandem]]

This was fixed in bug 1722872.