Assertion failure: type == MIRType_Object, at jit/IonTypes.h

RESOLVED WONTFIX

Status

()

Core
JavaScript Engine: JIT
--
critical
RESOLVED WONTFIX
3 years ago
2 years ago

People

(Reporter: gkw, Assigned: nbp)

Tracking

(Blocks: 1 bug, {assertion, regression, testcase})

Trunk
x86_64
Mac OS X
assertion, regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41- unaffected, firefox42 unaffected)

Details

(Whiteboard: [jsbugmon:update,ignore])

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
f = (function() {
         "use asm"
         function f() {
             var i = {};
             (_ ? 2() : (0 ? [] : w)) | 0
         }
         return f
 })()
 for (var j = 0; j < 999; j++) {
     try {
         f()
     } catch (e) {}
 }

asserts js debug shell on m-c changeset 203e1025a826 with --fuzzing-safe --no-threads --baseline-eager at Assertion failure: type == MIRType_Object, at jit/IonTypes.h.

Configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/fuzzing/js/compileShell.py -b "--enable-debug --enable-more-deterministic --enable-nspr-build" -r 203e1025a826

=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20150611043342" and the hash "fbb2e7d00e46".
The "bad" changeset has the timestamp "20150611053142" and the hash "46acf3627306".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=fbb2e7d00e46&tochange=46acf3627306

Setting s-s because MIR type assertion failures can be bad.

Nicolas, is bug 1166711 a likely regressor?
Flags: needinfo?(nicolas.b.pierron)
(Reporter)

Comment 1

3 years ago
Created attachment 8622101 [details]
stack

(lldb) bt 5
* thread #1: tid = 0x1d1518, 0x00000001006f2435 js-dbg-64-dm-nsprBuild-darwin-203e1025a826`js::jit::CodeGeneratorX64::visitBox(js::jit::LBox*) + 52 at IonTypes.h:450, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x00000001006f2435 js-dbg-64-dm-nsprBuild-darwin-203e1025a826`js::jit::CodeGeneratorX64::visitBox(js::jit::LBox*) + 52 at IonTypes.h:450
    frame #1: 0x00000001006f2401 js-dbg-64-dm-nsprBuild-darwin-203e1025a826`js::jit::CodeGeneratorX64::visitBox(this=<unavailable>, box=<unavailable>) + 593 at CodeGenerator-x64.cpp:81
    frame #2: 0x00000001004c2b79 js-dbg-64-dm-nsprBuild-darwin-203e1025a826`js::jit::CodeGenerator::generateBody(this=0x0000000101fe4000) + 985 at CodeGenerator.cpp:4111
    frame #3: 0x00000001004db9da js-dbg-64-dm-nsprBuild-darwin-203e1025a826`js::jit::CodeGenerator::generate(this=0x0000000101fe4000) + 458 at CodeGenerator.cpp:7787
    frame #4: 0x0000000100532c6e js-dbg-64-dm-nsprBuild-darwin-203e1025a826`js::jit::GenerateCode(mir=0x000000010391a258, lir=0x000000010391f188) + 302 at Ion.cpp:1729
(lldb)

Updated

3 years ago
QA Whiteboard: https://bugzilla.mozilla.org/show_bug.cgi?id=1174163

Updated

3 years ago
QA Whiteboard: https://bugzilla.mozilla.org/show_bug.cgi?id=1174163
See Also: → bug 1174163
Keywords: sec-high
This issue is caused by the fact that we re-specialized a Phi after removing a branch due to GVN / UCE optimizations.  The problem is that the first time we specialized the Phi to be an Object, and the type analysis inserted an MBox of that Phi.  The second iteration, as none of the result type sets are known, the phi is specialized to a Value, but the MBox is kept in place.

I see 2 options:

The first solution would be to flag any instructions added by the ApplyType phase, and the second time we run the ApplyType phase, we can either get rid of all of them, or only the one which are working against the Phi specialization.

The second solution would be to not specialize the Phi the second time.


I like the first solution as an optimization, but it sounds that it might be more likely introduce additional issues.  I will attempt the second solution, and see if it works well.
Created attachment 8623618 [details] [diff] [review]
ApplyType: Prevent re-entry of specialization phases.
Attachment #8623618 - Flags: review?(jdemooij)
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #0)
> Likely regression window:
> https://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=fbb2e7d00e46&tochange=46acf3627306
> 
> Nicolas, is bug 1166711 a likely regressor?

No, this is Bug 1165348, as Bug 1166711 only extend Scalar Replacement, and this issue is about re-executing the Phi specialization after running UCE.
Blocks: 1165348
No longer blocks: 1166711
Flags: needinfo?(nicolas.b.pierron)
Duplicate of this bug: 1175350
Duplicate of this bug: 1175339
Duplicate of this bug: 1174163
Do you have any other test case with the same signature?

Apparently the current fix does not fix all web-site, so I suspect there is another one with a similar signature.
Flags: needinfo?(gary)
(Reporter)

Comment 9

3 years ago
(In reply to Nicolas B. Pierron [:nbp] from comment #8)
> Do you have any other test case with the same signature?

Nope I don't, yet.
(Reporter)

Updated

3 years ago
Flags: needinfo?(gary)
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision be81b8d6fae9).
Comment on attachment 8623618 [details] [diff] [review]
ApplyType: Prevent re-entry of specialization phases.

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

::: js/src/jit/IonAnalysis.h
@@ +46,5 @@
>  
>  bool
>  EliminateDeadCode(MIRGenerator* mir, MIRGraph& graph);
>  
> +// This state variable is used to prevent optimization which might not be safe

Nit: optimizations

@@ +49,5 @@
>  
> +// This state variable is used to prevent optimization which might not be safe
> +// to re-enter.
> +//
> +// Fox example, UCE can remove branches which cause the Phi specialization to

Nit: causes

@@ +51,5 @@
> +// to re-enter.
> +//
> +// Fox example, UCE can remove branches which cause the Phi specialization to
> +// change from a more specific MIRType_Object to a less specific
> +// MIRType_Value. The last type specialization might have add a MBox after the

Nit: added

@@ +54,5 @@
> +// change from a more specific MIRType_Object to a less specific
> +// MIRType_Value. The last type specialization might have add a MBox after the
> +// Phi, but this MBox is not removed during the second iteration, and after the
> +// re-specialization.
> +enum ApplyTypeState {

You can use an enum class to avoid the ApplyType_ prefix:

enum class ApplyTypeState {
    FirstPass,
    FixupPass
};
Attachment #8623618 - Flags: review?(jdemooij) → review+
Nicolas, I noticed that this patch was r+'d on 6/24 but it hasn't been checked in anywhere. Could you please follow up?
Flags: needinfo?(nicolas.b.pierron)
Adding a tracking flag for FF41 so we can make sure the fix for this sec-high bugs gets landed in Aurora.
tracking-firefox41: --- → +
(In reply to Ritu Kothari (:ritu) from comment #12)
> Nicolas, I noticed that this patch was r+'d on 6/24 but it hasn't been
> checked in anywhere. Could you please follow up?

Bug 1165348 got backed out, so we should no longer be able to reproduce this issue (see comment 2).
I kept this bug open as I still need to land this fix when I reland Bug 1165348.
Flags: needinfo?(nicolas.b.pierron)
(Reporter)

Updated

3 years ago
Component: JavaScript Engine → JavaScript Engine: JIT
Duplicate of this bug: 1174556
Setting FF41 status to fixed based on comment 14 and removing the tracking flag for 41. Adding a tracking flag for FF42 as Nicolas wants this bug to remain open.
status-firefox42: --- → ?
tracking-firefox41: + → -
tracking-firefox42: --- → +

Updated

3 years ago
Group: core-security → javascript-core-security
(In reply to Ritu Kothari (:ritu) from comment #13)
> Adding a tracking flag for FF41 so we can make sure the fix for this
> sec-high bugs gets landed in Aurora.

The faulty patch got backed out (Bug 1165348 comment 14), so this bug no longer need to be a s-s bug, and we do not have to track any version anymore.
Group: javascript-core-security
status-firefox41: affected → unaffected
status-firefox42: ? → unaffected
tracking-firefox42: + → ---
Keywords: sec-high
(Reporter)

Comment 18

2 years ago
(In reply to Nicolas B. Pierron [:nbp] from comment #14)
> I kept this bug open as I still need to land this fix when I reland Bug
> 1165348.

If bug 1165348 is now WONTFIX, should this fix also be WONTFIX'ed?
Flags: needinfo?(nicolas.b.pierron)
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #18)
> (In reply to Nicolas B. Pierron [:nbp] from comment #14)
> > I kept this bug open as I still need to land this fix when I reland Bug
> > 1165348.
> 
> If bug 1165348 is now WONTFIX, should this fix also be WONTFIX'ed?

Yes, indeed.  Branch Pruning is the new alternative to moving Scalar Replacement later in the pipeline, which does not require any ApplyType fixes.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Flags: needinfo?(nicolas.b.pierron)
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.