1005458 - Crash [@ GetElement] or Assertion failure: isObject(), at dist/include/js/Value.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

Attachment: debug and opt stacks

(function(x) {
    for (var y = 0; y < 1; y++) {
        Array.prototype.shift.call(arguments.callee.arguments);
    }
})(0)

asserts js debug shell on m-c changeset 8d591a3f6fea with --ion-eager --ion-parallel-compile=off at Assertion failure: isObject(), at dist/include/js/Value.h and crashes js opt shell at GetElement

My configure flags (debug) are:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --with-ccache --enable-threadsafe <other NSPR options>

Opt:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-optimize --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --with-ccache --enable-threadsafe <other NSPR options>


autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/d34458e80bcb
user:        Shu-yu Guo
date:        Thu Apr 24 01:59:36 2014 -0700
summary:     Bug 716647 - Part 1: Introduce JS_OPTIMIZED_OUT magic for optimized out slots and teach Debugger about them. (r=jandem)


Shu-yu, is bug 716647 a likely regressor?

Comment 1

[Shu-yu Guo [:shu]]

Attachment: Argument slot phis are always observable in non-strict scripts due to Function.arguments.

We've been optimizing out argument slots incorrectly. Argument slots are
*always* observable in non-strict slots due to the travesty that is
Function.arguments.

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8416948 [details] [diff] [review]
Argument slot phis are always observable in non-strict scripts due to Function.arguments.

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

::: js/src/jit/CompileInfo.h
@@ +397,5 @@
> +            return true;
> +
> +        // Function.arguments can be used to access all arguments in
> +        // non-strict scripts, so we can't optimize out any arguments.
> +        return !(firstArgSlot() <= i && i - firstArgSlot() < nargs());

nice!

::: js/src/jit/shared/CodeGenerator-shared.cpp
@@ +148,5 @@
>          mir->isUnused() ? MIRType_MagicOptimizedOut :
>          mir->type();
>  
> +    MOZ_ASSERT_IF(type == MIRType_MagicOptimizedOut,
> +                  mir->block()->info().canOptimizeOutSlot(*allocIndex));

I'll suggest moving this assertion to either LIRGeneratorShared::buildSnapshot or LRecoverInfo::appendResumePoint, at least we have a clear overview of each resume points as opposed to the code generator.

Also allocIndex mirrors the full snapshot which might contain multiple resume points (in case of inlined frames).

Comment 3

[Shu-yu Guo [:shu]]

Attachment: Argument slot phis are always observable in non-strict scripts due to Function.arguments.

Addressed review; also needed to fix up a debugger test since this patch
changes what's optimized.

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8416960 [details] [diff] [review]
Argument slot phis are always observable in non-strict scripts due to Function.arguments.

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

::: js/src/jit/shared/Lowering-shared.cpp
@@ +74,5 @@
> +static bool
> +CanOptimizeOutSlot(MDefinition *mir, size_t slot)
> +{
> +    MOZ_ASSERT(mir->isUnused());
> +    if (!mir->isRecoveredOnBailout() || mir->type() == MIRType_MagicOptimizedOut)

nit: remove the "!", as recover instructions are only used during bailouts, and not during the recovery of arguments.

@@ +75,5 @@
> +CanOptimizeOutSlot(MDefinition *mir, size_t slot)
> +{
> +    MOZ_ASSERT(mir->isUnused());
> +    if (!mir->isRecoveredOnBailout() || mir->type() == MIRType_MagicOptimizedOut)
> +        return mir->block()->info().canOptimizeOutSlot(slot);

The block should be the block of the resume point holding the MIR and not the block of the MIR instruction.  An inline function could use a MIR instruction from the outer script as argument, in which case we want to prevent the optimization of the inline frame arguments.

@@ +111,5 @@
>          // Guards should never be eliminated.
> +        MOZ_ASSERT_IF(ins->isUnused(), !ins->isGuard());
> +
> +        // Unused, non-recoverable operands must be in eliminable slots.
> +        MOZ_ASSERT_IF(ins->isUnused(), CanOptimizeOutSlot(ins, index));

This needs to be done:
 - before the check of isRecoveredOnBailout.
 - only if the holder is a ResumePoint ( add LRecoverInfo::OperandIter::holderIsResumePoint() )

Comment 5

[Shu-yu Guo [:shu]]

Attachment: Argument slot phis are always observable in non-strict scripts due to Function.arguments.

Applied review comments.

Comment 6

[Shu-yu Guo [:shu]]

Attachment: Argument slot phis are always observable in non-strict scripts due to Function.arguments.

Changed |this->operator*()| to |**this|

Comment 7

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87d402b80185

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/87d402b80185

Comment 9

[javascriptjedi]

Can someone please merge this fix into the FF 31.2 ESR codestream?  The crash still happens (often for me) with the released version and the patch fixes the problem.  (I didn't test the debug stuff, just the change to CompileInfo.h and IonAnalysis.cpp.)

ESR is supposed to be more stable than the latest branch, not less, so I'm not sure why this wasn't done long ago.

Comment 11

[Matthias Versen [:Matti]]

[Tracking Requested - why for this release]:
Requested by the user in comment#9/bug1097551 + asking the developer about his opinion about a backport

Comment 12

[Shu-yu Guo [:shu]]

Yeah backport it, sure why not.

Comment 13

[Shu-yu Guo [:shu]]

Attachment: Backport for ESR31.

Comment 14

[Shu-yu Guo [:shu]]

Comment on attachment 8524267 [details] [diff] [review]
Backport for ESR31.

Posting backport for ESR31 per bug 1096290. Tests were not backported.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: See bug 1096290
Fix Landed on Version: 32.
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 15

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/251dfa40560f

Comment 16

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Backed out for jit-test failures.
https://hg.mozilla.org/releases/mozilla-esr31/rev/809a30c15b76

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=26844&repo=mozilla-esr31

Comment 17

[Shu-yu Guo [:shu]]

Attachment: Backport patch for ESR31 + test fix

Comment 18

[Shu-yu Guo [:shu]]

Comment on attachment 8528088 [details] [diff] [review]
Backport patch for ESR31 + test fix

Sorry about that, forgot to update test.

Comment 19

[Shu-yu Guo [:shu]]

Could you repush, Ryan?

Comment 20

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/0270179cfab5

FWIW, it looks like this missed the cutoff for ESR 31.3, unless there's a respin.

Comment 21

[Jan Swaelens]

Hello All,

Do you have an indication of the release date of ESR 31.4 please?

tx

Comment 22

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

ESR 31.4 is due to ship with Firefox 35 on 2015-01-13.

Comment 23

[Jan Swaelens]

Thanks!