Closed Bug 1005458 Opened 6 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
firefox-esr31 34+ fixed

People

(Reporter: gkw, Assigned: shu)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:update])

Crash Data

Attachments

(3 files, 4 obsolete files)

Attached file 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?
Flags: needinfo?(shu)
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.
Attachment #8416948 - Flags: review?(nicolas.b.pierron)
Assignee: nobody → shu
Flags: needinfo?(shu)
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).
Attachment #8416948 - Flags: review?(nicolas.b.pierron) → feedback+
Addressed review; also needed to fix up a debugger test since this patch
changes what's optimized.
Attachment #8416948 - Attachment is obsolete: true
Attachment #8416960 - Flags: review?(nicolas.b.pierron)
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() )
Attachment #8416960 - Flags: review?(nicolas.b.pierron)
Applied review comments.
Attachment #8416960 - Attachment is obsolete: true
Attachment #8417635 - Flags: review?(nicolas.b.pierron)
Changed |this->operator*()| to |**this|
Attachment #8417635 - Attachment is obsolete: true
Attachment #8417635 - Flags: review?(nicolas.b.pierron)
Attachment #8417758 - Flags: review?(nicolas.b.pierron)
Attachment #8417758 - Flags: review?(nicolas.b.pierron) → review+
Status: NEW → ASSIGNED
Depends on: 1007027
https://hg.mozilla.org/mozilla-central/rev/87d402b80185
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
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.
Duplicate of this bug: 1097551
[Tracking Requested - why for this release]:
Requested by the user in comment#9/bug1097551 + asking the developer about his opinion about a backport
Flags: needinfo?(shu)
Yeah backport it, sure why not.
Flags: needinfo?(shu)
Attached patch Backport for ESR31. (obsolete) — Splinter Review
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.
Attachment #8524267 - Flags: approval-mozilla-esr31?
Attachment #8524267 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
Attachment #8524267 - Attachment is obsolete: true
Comment on attachment 8528088 [details] [diff] [review]
Backport patch for ESR31 + test fix

Sorry about that, forgot to update test.
Attachment #8528088 - Attachment description: Argument slot phis are always observable in non-strict scripts due to Function.arguments. ( → Backport patch for ESR31 + test fix
Flags: needinfo?(shu)
Could you repush, Ryan?
Flags: needinfo?(ryanvm)
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.
Flags: needinfo?(ryanvm) → in-testsuite+
Hello All,

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

tx
ESR 31.4 is due to ship with Firefox 35 on 2015-01-13.
Thanks!
You need to log in before you can comment on or make changes to this bug.