Closed
Bug 880377
Opened 11 years ago
Closed 11 years ago
Crash [@ IntersectDominators]
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
mozilla25
People
(Reporter: decoder, Assigned: nmatsakis)
References
Details
(Keywords: crash, testcase, Whiteboard: [jsbugmon:update])
Crash Data
Attachments
(2 files, 1 obsolete file)
586 bytes,
text/plain
|
Details | |
6.52 KB,
patch
|
nmatsakis
:
review+
bajaj
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta-
akeybl
:
approval-mozilla-release-
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
Reporter | ||
Comment 2•11 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•11 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Reporter | ||
Comment 3•11 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•11 years ago
|
||
Needinfo from Brian based on comment 3.
Flags: needinfo?(bhackett1024)
Comment 5•11 years ago
|
||
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•11 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)
Comment 9•11 years ago
|
||
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•11 years ago
|
||
Carrying over r+ from bhackett
Assignee: general → nmatsakis
Attachment #774066 -
Attachment is obsolete: true
Attachment #774658 -
Flags: review+
Assignee | ||
Comment 12•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/42a9972a1667
Comment 13•11 years ago
|
||
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:
--- → ?
Assignee | ||
Comment 14•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b576d83f89f
Assignee | ||
Comment 15•11 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?
Comment 16•11 years ago
|
||
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.
Updated•11 years ago
|
Attachment #774658 -
Flags: approval-mozilla-release?
Attachment #774658 -
Flags: approval-mozilla-release-
Attachment #774658 -
Flags: approval-mozilla-beta?
Attachment #774658 -
Flags: approval-mozilla-beta-
Comment 17•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/42a9972a1667 https://hg.mozilla.org/mozilla-central/rev/7b576d83f89f
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Updated•11 years ago
|
Updated•11 years ago
|
Attachment #774658 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 18•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/7255bae7b635 https://hg.mozilla.org/releases/mozilla-aurora/rev/218c4145c066
You need to log in
before you can comment on or make changes to this bug.
Description
•