Closed Bug 582084 Opened 11 years ago Closed 11 years ago

JM: jsreftest browser crash on e4x/decompilation/regress-352013.js


(Core :: JavaScript Engine, defect)

Not set





(Reporter: dmandelin, Assigned: dmandelin)




(2 files)

The crash is in stubs::Add, where the rval is tagged as a string but has payload 0x1.
What commit was this tested with? fixed a syncing bug in the Add slowpath that sounds like this.
It is in today's tip.

Cause: the fast path for JSOP_(INC|DEC)GLOBAL doesn't store a type tag. So it stores an int payload but leaves a stale type tag.
Attached patch PatchSplinter Review
I'm not really sure exactly what the conditions on pushNumber are supposed to be, but it seems like something different is wanted by globalinc, so I just made a new function to push an int32.

It looks like this may cost us 1 ms on SS, but it's hard to tell with such a small change. I can file a followup bug on trying out holding the tag in a register after this lands.
Attachment #460370 - Flags: review?(dvander)
(In reply to comment #3)
> Created attachment 460370 [details] [diff] [review]

FrameState::pushInt32() is the same as FrameState::pushNumber(foo, true). The problem is just that the true flag was not passed -- causing pushNumber() to mark the typed as synced in memory, when it was never actually written.
Patch implementing above suggestion. The pushInt32() approach might be cleaner -- but then the purpose of pushNumber() is dubious.
There is also the option of pushUntypedPayload(reg, JSVAL_INT32_TAG), which I think used to be used in jsop_globalinc.
Comment on attachment 460370 [details] [diff] [review]

>     /*
>+     * Pushes an int32 onto the operation stack.
>+     */
>+    inline void pushInt32(RegisterID payload);

This should probably say that it's related to pushNumber, and that int32 is a soft guarantee. i.e. "The caller guarantees that an int32 has been pushed on the inline path, and that if a double was written on the OOL path, it has been synced to memory."
Attachment #460370 - Flags: review?(dvander) → review+
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.