781022 - IonMonkey: Crash [@ js::gc::MarkGCThingRoot]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack from opt crash

a = Uint8ClampedArray;
for each(c in [0]) {
    gczeal(10)
}
try {
    (function () {
        Object.defineProperty(this, "b", {
            get: function () {
                a.subarray()
            }
        })
    })()
(b)
} catch (e) {}
try {
    (function () {
        Object.defineProperty(this, "a", {
            get: function () {
                return Uint8ClampedArray()
            }
        })
    })()
(2 instanceof b)
} catch (e) {}
(b instanceof 2)

crashes js opt shell on IonMonkey changeset b2361e15b665 with --ion-eager at js::gc::MarkGCThingRoot when the testcase is passed in as a CLI argument, does not seem to cause anything on debug shell.

s-s because gc is on the stack, and also because a weird memory address 0x7fff000001f5 is being accessed.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   103056:9f3dc298e25b
user:        David Anderson
date:        Thu Aug 02 14:17:13 2012 -0700
summary:     MIR nodes should use CompilerRoot, not HeapPtr (bug 779812, r=sstangl).

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack from opt crash

This is the correct stack.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Valgrind stack

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

Whoops, when I changed this to use CompilerRootNode I didn't notice that Entries were created on the stack.

Comment 4

[Kannan Vijayan [:djvj]]

Comment on attachment 649941 [details] [diff] [review]
fix

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

::: js/src/ion/MIR.h
@@ +3935,5 @@
>  
>    public:
>      InlinePropertyTable(jsbytecode *pc)
> +        : pc_(pc), priorResumePoint_(NULL)
> +    {

Not sure if style guides include this, but I think it's good to refer to all members in constructor initializers (i.e. have an empty initialier for entries_ in this case).  When reading code it makes it easier to figure out the initial state of the object without having to look at the member declarations.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

I don't have a particular opinion on style there, but I re-added the initialization. Pushed as:

https://hg.mozilla.org/projects/ionmonkey/rev/826f67c39bdc

Comment 6

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.