822938 - IonMonkey: Crash [@ js::ion::GetPropertyCache] or near null or "Assertion failure: live->empty(),"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stack

(function f() {
    for (var x in []) {
        (function() {})();
    };
    f();
})();

asserts js debug shell on m-c changeset 287a7d7cf7f0 with --no-jm at Assertion failure: live->empty(), and crashes js opt shell near null.

s-s because it seems to be crashing at an unknown address, but it seems to be null, but locking just in case.

autoBisecting now.

Comment 1

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

Marking [fuzzblocker] because this opt crash does not have a signature that we can ignore.

Comment 2

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   116398:aec01763cb6b
user:        Nicolas B. Pierron
date:        Tue Dec 18 05:57:29 2012 -0800
summary:     Bug 819865 - Make the callee canonical in IonBuilder. r=jandem

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: IonGraph: Allocate Register phase result (with assertion disabled)

The problem comes from the move group which is only defining the callee on the left-hand side of the basic block and as it does not show up in the OSR block and is not captured by any Phi this assertion is raised.

Brian, do you know why we are spilling values on the argument slot, and is there a way to mark this slot as a constant slot ?

Comment 4

[Nicolas B. Pierron [:nbp]]

Flag as sec-critical as this bug is using any register returned by the register allocator instead of using the LArgument value where it is used.  This might not be a null-deref.

As jandem remark, JSOP_CALLEE is not supported in JM, and as named lambdas are not used in PdfJS this should not change the performances.  So at last we can just remove the compilation support for JSOP_CALLEE while keeping it for the scope chain.

Comment 5

[Nicolas B. Pierron [:nbp]]

jandem, dvander,  Do you mind If I replace Bug 819865 by its previous implementation namely attachment 692975 [details] [diff] [review] ?

Comment 6

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

What if we just change how it's lowered? It should work to define() LCallee as a register, and then in CG, loadPtr(<address>, output). That avoids every problem and with GVN it won't make any difference in practice.

Comment 7

[Nicolas B. Pierron [:nbp]]

(In reply to David Anderson [:dvander] from comment #6)
> What if we just change how it's lowered? It should work to define() LCallee
> as a register, and then in CG, loadPtr(<address>, output). That avoids every
> problem and with GVN it won't make any difference in practice.

This won't change the problem encountered here, because the problem seen here is that a register can be used in the normal entry point, but not for the OSR entry point.  When we hit the OSR entry point we have to provide the same allocation.  With MParameters we have no problem because the OSR block is defining then, and as formal arguments (aka MParameters) are captured in the basic blocks / resume points, we introduce MPhi at the top of each block and rely on the SSA form to do the allocation for us.

The other case similar to this MCallee is the Magic lazy-argument constant, which is only defined in the entry point only, and this does not cause any issue since the constant does not appear as part of the allocations.

As MCallee is not captured in the slots of the basic blocks, as opposed to MParameters, and as it needs an allocation which cannot be inlined, as opposed to the magic value of arguments, the register allocator complains because the OSR entry point has no definition and this cannot be solved easily except at considering the MCallee as a special slot, which would be costly for functions which are not using any callee.

Weaking the RegAlloc assertion to avoid complaining about clobber of arguments should be enough, and GVN would get rid of extra definitions of MCallee.  Without any GVN, this will lead to less optimized code (which is fine for me) which will have to load the callee everytime from the frame to a register.

Comment 8

[Christian Holler (:decoder)]

Can also crash [@ js::ion::GetPropertyCache]

Comment 9

[Nicolas B. Pierron [:nbp]]

Attachment: LCallee use a register instead of a fixed LArgument.

As discuss with dvander, this relies on GVN to remove extra MCallee, and avoid weakening the assertion by making the load in the CodeGen of the LCallee instead of using the move groups of the register allocator.

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 694827 [details] [diff] [review]
LCallee use a register instead of a fixed LArgument.

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

::: js/src/ion/IonBuilder.cpp
@@ +1035,5 @@
> +      {
> +        MCallee *callee = MCallee::New();
> +        current->add(callee);
> +        current->push(callee);
> +        return callee;

"return true;"

::: js/src/ion/IonBuilder.h
@@ +512,5 @@
>      // True if script->failedShapeGuard is set for the current script or
>      // an outer script.
>      bool failedShapeGuard_;
>  
> +    // If this script can use a lazy arguments object, it wil be pre-created

Nit: don't revert the typo fix (s/wil/will)

Comment 11

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc52b06ea2ea

Comment 12

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #11)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/bc52b06ea2ea

landed with test case as it only affects 20 and the feature can easily be disabled, by removing JSOP_CALLEE support, if there is any security issue.

Remove need-info from Brian Hackett as we no longer have any LArgument used for spilling.

Comment 13

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

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

Comment 14

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.