Crash [@ IntersectDominators]

VERIFIED FIXED in Firefox 24

Status

()

--
critical
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: decoder, Assigned: nmatsakis)

Tracking

(Blocks: 1 bug, {crash, testcase})

Trunk
mozilla25
x86
Linux
crash, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox22 wontfix, firefox23- wontfix, firefox24- verified, firefox25- verified)

Details

(Whiteboard: [jsbugmon:update], crash signature)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
The following testcase crashes on mozilla-central revision 204de5b7e0a6 (run with --ion-eager):


var actual = '';
var obj0 = {}
obj2 = {};
obj2['b'+1] = 1;
if (actual[0] != 0[0])
  throw new i();
for (var k in obj2) {
  for (var apply in obj0)
    ++count;
}
(Reporter)

Comment 1

6 years ago
Created attachment 759318 [details]
[crash-signature] Machine-readable crash signature
(Reporter)

Comment 2

6 years ago
Looks like a null-deref:

Program received signal SIGSEGV, Segmentation fault.
0x0845762e in IntersectDominators (block2=<optimized out>, block1=0x92fdd50) at js/src/ion/IonAnalysis.cpp:668
668                 if (idom == finger2)
#0  0x0845762e in IntersectDominators (block2=<optimized out>, block1=0x92fdd50) at js/src/ion/IonAnalysis.cpp:668
#1  ComputeImmediateDominators (graph=...) at js/src/ion/IonAnalysis.cpp:708
#2  js::ion::BuildDominatorTree (graph=...) at js/src/ion/IonAnalysis.cpp:736
#3  0x08559a6e in js::ion::UnreachableCodeElimination::removeUnmarkedBlocksAndCleanup (this=0xffffc554) at js/src/ion/UnreachableCodeElimination.cpp:70
#4  0x0844a154 in js::ion::OptimizeMIR (mir=0x9288800) at js/src/ion/Ion.cpp:1031
#5  0x0844faf2 in CompileBackEnd (mir=0x9288800, maybeMasm=<optimized out>) at js/src/ion/Ion.cpp:1246
[...]
eax     0x0     0
=> 0x845762e <js::ion::BuildDominatorTree(js::ion::MIRGraph&)+1054>:    mov    0x64(%eax),%esi
Whiteboard: [jsbugmon:update,bisect]
(Reporter)

Updated

6 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
(Reporter)

Comment 3

6 years ago
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/ee14945b452c
parent:      128511:d989eab66df4
user:        Brian Hackett
date:        Thu Apr 11 18:39:32 2013 -0600
summary:     Bug 804676 - Remove dependence of Ion compilation on ScriptAnalysis::analyzeTypes.

This iteration took 11.485 seconds to run.
(Reporter)

Comment 4

6 years ago
Needinfo from Brian based on comment 3.
Flags: needinfo?(bhackett1024)
This looks like a problem with unreachable code elimination.  The different type information caused UCE to decide to eliminate the preheader for some loop, despite there being an OSR block inside it.  This causes later graph analysis to break.
Flags: needinfo?(bhackett1024) → needinfo?(nmatsakis)
(Assignee)

Comment 6

6 years ago
All the blocks which UCE removes are indeed unreachable, but in this case it has the effect of severing the graph into two and removing loop headers. This is not clearly wrong to me, but it does seem to be incompatible with what later passes expect. I have a partial fix which tries to avoid optimizing away tests if they will create this scenario, but I haven't quite found the right way to summarize the set of blocks that should not be deleted.
Flags: needinfo?(nmatsakis)
(Assignee)

Comment 7

6 years ago
Created attachment 774066 [details] [diff] [review]
Do not attempt UCE if there are blocks only reachable from OSR
Attachment #774066 - Flags: review?(bhackett1024)
(Assignee)

Updated

6 years ago
Duplicate of this bug: 887772
Comment on attachment 774066 [details] [diff] [review]
Do not attempt UCE if there are blocks only reachable from OSR

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

::: js/src/ion/UnreachableCodeElimination.cpp
@@ +105,5 @@
> +    return list.append(block);
> +}
> +
> +MBasicBlock *
> +UnreachableCodeElimination::branchSuccessor(MBasicBlock *block) {

This function should have a comment, or a better name.  Also, nit: { on the next line.

@@ +125,3 @@
>  UnreachableCodeElimination::prunePointlessBranchesAndMarkReachableBlocks()
>  {
>      Vector<MBasicBlock *, 16, SystemAllocPolicy> worklist;

This variable's type should be 'Worklist'

@@ +125,4 @@
>  UnreachableCodeElimination::prunePointlessBranchesAndMarkReachableBlocks()
>  {
>      Vector<MBasicBlock *, 16, SystemAllocPolicy> worklist;
> +    Vector<MBasicBlock *, 16, SystemAllocPolicy> neuter;

Maybe 'blocksWithKnownSuccessors' instead.

@@ +157,5 @@
> +    // Now, if there is an OSR block, check that all of its successors
> +    // were reachable (bug 880377). If not, we are in danger of
> +    // creating a CFG with two disjoint parts, so simply mark all
> +    // blocks as reachable. This generally occurs when the TI info is
> +    // incomplete.

The last sentence could use elaboration.  'This generally occurs when TI info for stack types is incomplete, due to operations that have only executed in the interpreter'
Attachment #774066 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 10

6 years ago
Created attachment 774658 [details] [diff] [review]
Do not attempt UCE if there are blocks only reachable from OSR

Carrying over r+ from bhackett
Assignee: general → nmatsakis
Attachment #774066 - Attachment is obsolete: true
Attachment #774658 - Flags: review+
We should backport this to aurora and beta, it's causing browser crashes.
status-firefox23: --- → affected
status-firefox24: --- → affected
status-firefox25: --- → affected
tracking-firefox23: --- → ?
tracking-firefox24: --- → ?
tracking-firefox25: --- → ?

Updated

6 years ago
Blocks: 878413
(Assignee)

Comment 15

6 years ago
Comment on attachment 774658 [details] [diff] [review]
Do not attempt UCE if there are blocks only reachable from OSR

[Approval Request Comment]
Regression caused by (bug #): 820676
User impact if declined: Crashes
Testing completed (on m-c, etc.): Added regression test
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch:
Attachment #774658 - Flags: approval-mozilla-release?
Attachment #774658 - Flags: approval-mozilla-beta?
Attachment #774658 - Flags: approval-mozilla-aurora?
Not a top crash, so the highest we'll take this is on Aurora. No need to track, but we'll approve for Aurora once it's had a little bake time.
tracking-firefox23: ? → -
tracking-firefox24: ? → -
tracking-firefox25: ? → -
Attachment #774658 - Flags: approval-mozilla-release?
Attachment #774658 - Flags: approval-mozilla-release-
Attachment #774658 - Flags: approval-mozilla-beta?
Attachment #774658 - Flags: approval-mozilla-beta-
https://hg.mozilla.org/mozilla-central/rev/42a9972a1667
https://hg.mozilla.org/mozilla-central/rev/7b576d83f89f
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25

Updated

6 years ago
status-firefox25: affected → fixed
Attachment #774658 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/7255bae7b635
https://hg.mozilla.org/releases/mozilla-aurora/rev/218c4145c066
status-firefox22: --- → wontfix
status-firefox23: affected → wontfix
status-firefox24: affected → fixed

Updated

5 years ago
Duplicate of this bug: 878413
Reproduced in nightly 2013-06-06.
Verified fixed jshell-win32 FF 24b6.
status-firefox24: fixed → verified
Keywords: verifyme
Verified fixed jshell-win32 FF 25b2.
Status: RESOLVED → VERIFIED
status-firefox25: fixed → verified
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.