930378 - Improve rooting in Debugger

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

Attachment: Debugger rooting

I just don't like AutoValueArray.

Comment 1

[Steve Fink [:sfink] [:s:]]

Comment on attachment 821467 [details] [diff] [review]
Debugger rooting

This fixes one actual bug (an AutoValueArray address was passed into something that GC'd before the element was initialized, so a bogus Value was marked, which failed an assertion.) It also switches to more straightforward rooting. The renaming from argv -> specificName is arguable, since its address ends up being passed into Invoke as a 1-element array. Tell me if you no like.

Comment 2

[Steve Fink [:sfink] [:s:]]

(In reply to Steve Fink [:sfink] from comment #1)
> Comment on attachment 821467 [details] [diff] [review]
> Debugger rooting
> 
> This fixes one actual bug (an AutoValueArray address was passed into
> something that GC'd before the element was initialized, so a bogus Value was
> marked, which failed an assertion.)

Er... actually, the part about the address being passed in is irrelevant. The problem was really just GCing with an uninitialized AutoValueArray on the stack.

Comment 3

[Steve Fink [:sfink] [:s:]]

Attachment: bug-930378-using-base

I don't know whether this is better or worse, so I'll put it up for review and let you decide. (And yes, I'm abusing an unrelated bug to hang this off of. Sue me. bzexport is busted due to a bmo change. Bug/patch manipulation is painful.)

Comment 4

[Jim Blandy :jimb]

Comment on attachment 821467 [details] [diff] [review]
Debugger rooting

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

Thanks very much for looking at these. All these changes look like good cleanups to me, except for the one I note below.

There are a few cases (ApplyOrCall's argument list wrapping, for example) that seem to go out of their way to not do the obvious thing. I wish I knew why; but let's go with your changes.

::: js/src/vm/Debugger.cpp
@@ +964,5 @@
>  
>      Maybe<AutoCompartment> ac;
>      ac.construct(cx, object);
>  
> +    AutoValueVector argv(cx);

Can't we just use AutoValueArray, and initialize the slot? It seems excessive to use a dynamically allocated type when we need exactly two slots.

Comment 5

[Steve Fink [:sfink] [:s:]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/12eb8c273f9b

https://tbpl.mozilla.org/?tree=Try&rev=7e1af21fc01a
https://hg.mozilla.org/integration/mozilla-inbound/rev/0de56f72315e

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/12eb8c273f9b
https://hg.mozilla.org/mozilla-central/rev/0de56f72315e