1092032 - [jsdbg2] ArgumentsObjects with JS_OPTIMIZED_OUT magic slots misbehave

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Shu-yu Guo [:shu]]

Turns out that ArugmentsObject uses magic uint32 values to signify that a missing value should be forwarded from a CallObject. The check for whether a slot is such a magic uint32 value is v.isMagic(). The problem with this is that sometimes when creating ArgumentsObjects, we copy values from a frame up front. If this frame happens to be an Ion frame or a just bailed out Ion frame, some of those slots could be JS_OPTIMIZED_OUT magic. That's fine, as we only do that for values that are proven to be unobservable.

Unless, of course, you reflect the ArgumentsObject in the Debugger and try to get the slot.

Comment 1

[Shu-yu Guo [:shu]]

Attachment: Fix ArgumentsObject interaction with Debugger reflection on JIT frames with optimized out arguments.

Comment 2

[Shu-yu Guo [:shu]]

I don't have a reduced test case, but STR as found by nemo on IRC:

1. Go to http://peterjensen.github.io/mandelbrot/js/mandelbrot-xdk.html
2. Turn SIMD on.
3. Click start.
4. Wait till first Slow Script Dialog, press continue.
5. Wait till second Slow Script Dialog, press continue.
6. Wait till third Slow Script Dialog, press Debug.

The browser crashes.

Comment 3

[nemo]

Nice. That was fast.
https://crash-stats.mozilla.com/report/index/ef4d191d-2f14-411a-8f1f-ea3432141031

was my crash in case that matters at this point.

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 8514819 [details] [diff] [review]
Fix ArgumentsObject interaction with Debugger reflection on JIT frames with optimized out arguments.

Uh oh, the forward-to-call-object magic value stores an integer as the magic value's payload, so we can't distinguish between v.isMagic(JS_OPTIMIZED_OUT) and a forward-to-call-object magic value with an integer payload == JS_OPTIMIZED_OUT.

Two options I can see:
 - Can we instead filter JS_OPTIMIZED_OUT magic values before storing into the argsobj array?  Also, these are arguments; how do they get the TDZ value?
 - Find some other sentinel value type (I think we'd need a whole new value tag)

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Luke Wagner [:luke] from comment #4)
> Uh oh, the forward-to-call-object magic value stores an integer as the magic
> value's payload, so we can't distinguish between v.isMagic(JS_OPTIMIZED_OUT)
> and a forward-to-call-object magic value with an integer payload ==
> JS_OPTIMIZED_OUT.

Could we add the max JSWhyMagic value to the integers we store and subtract this value when we read the integer?

Comment 6

[Luke Wagner [:luke]]

lol, nice.  For what started out as a singleton value type, how we've fallen :)

But still, can't we filter out JS_OPTIMIZED_OUT values when bailing?  (Or is the whole point of JS_OPTIMIZED_OUT magic values to catch accidental misuse?)

Comment 7

[Shu-yu Guo [:shu]]

(In reply to Luke Wagner [:luke] from comment #6)
> lol, nice.  For what started out as a singleton value type, how we've fallen
> :)
> 
> But still, can't we filter out JS_OPTIMIZED_OUT values when bailing?  (Or is
> the whole point of JS_OPTIMIZED_OUT magic values to catch accidental misuse?)

That, and to be able to reflect optimized out values to the Debugger, as I've added the ability to turn on Debugger with JIT frames on the stack.

Comment 8

[Shu-yu Guo [:shu]]

A third option: can we check both v.isMagic() && !getFixedSlot(MAYBE_CALL_SLOT).isMagic() to mean "forward to call object"? ISTM that slot isn't used normally, and we could initialize MAYBE_CALL_SLOT to JS_OPTIMIZED_OUT (or another magic value) for the cases when we are not forwarding.

Doesn't seem to me that we could both be wanting to forward to a call object and have an JS_OPTIMIZED_OUT slot value at the same time.

Comment 9

[Luke Wagner [:luke]]

Maybe I'm misunderstanding your idea, but it seems like you could have a situation where one formal is forwarded to the call object and one formal is JS_OPTIMIZED_OUT.

Comment 10

[Shu-yu Guo [:shu]]

(In reply to Luke Wagner [:luke] from comment #9)
> Maybe I'm misunderstanding your idea, but it seems like you could have a
> situation where one formal is forwarded to the call object and one formal is
> JS_OPTIMIZED_OUT.

Yeah, you're right. I thought forwarding to a call object was mutually exclusive with having any other slot on the arguments object be a magic value. Going to take Jan's idea.

Comment 11

[Shu-yu Guo [:shu]]

Attachment: Bias magic uint32s in ArgumentObject forwarded slots by the maximum JSWhyMagic value to distinguish them from the JSWhyMagic-based magic values.

Comment 12

[Luke Wagner [:luke]]

Comment on attachment 8515312 [details] [diff] [review]
Bias magic uint32s in ArgumentObject forwarded slots by the maximum JSWhyMagic value to distinguish them from the JSWhyMagic-based magic values.

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

::: js/src/vm/ArgumentsObject-inl.h
@@ +34,5 @@
>  {
>      MOZ_ASSERT(!isElementDeleted(i));
>      HeapValue &lhs = data()->args[i];
> +    if (IsMagicScopeSlotValue(lhs)) {
> +        uint32_t slot = lhs.magicUint32() - JS_WHY_MAGIC_COUNT;

SlotFromMagicScopeSlotValue?

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/dc0e16585318