Closed Bug 1322900 Opened 7 years ago Closed 7 years ago

Firefox Nightly 53.0a1 (2016-12-10) crashes in [@ js::jit:: ... ] & [@ ComputeImmediateDominators ]

Categories

(Core :: JavaScript Engine: JIT, defect)

53 Branch
x86_64
Windows 7
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla53
Tracking Status
firefox-esr45 --- unaffected
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- unaffected
firefox53 + verified

People

(Reporter: Virtual, Assigned: h4writer)

References

()

Details

(Keywords: crash, nightly-community, regression)

Crash Data

Attachments

(1 file)

STR:
1. Open this URL - http://www.pcgamer.com/watch-as-over-5000-eve-online-pilots-destroy-a-fully-operational-death-star/
and enjoy crash

Probably caused by:
bug #1310155 - Split IonBuilder into cfg and the rest



[Tracking Requested - why for this release]: Regression
Flags: needinfo?(hv1989)
From what I can see another good example of a video that will reproduce this is any twitch video.

https://www.twitch.tv/direwolf20/v/106794909 was an example I found, again reproducible.
Do we need bugs for all the new JIT crash signatures? In addition to Bug 1322943 which I filed, I also see these new signatures:

http://bit.ly/2hrxmpP
http://bit.ly/2gTydhO
http://bit.ly/2hGVlA9
http://bit.ly/2gukjSB
(In reply to Marcia Knous [:marcia - use ni] from comment #6)
> Do we need bugs for all the new JIT crash signatures? In addition to Bug
> 1322943 which I filed, I also see these new signatures:
> 
> http://bit.ly/2hrxmpP
> http://bit.ly/2gTydhO
> http://bit.ly/2hGVlA9
> http://bit.ly/2gukjSB

And this has appeared from 1210 nightly, too.
https://crash-stats.mozilla.com/report/index/6529780b-62cf-4cfe-a43c-06b332161211
Tracking 53+ - new crash with varied signatures that is reproducible.
Attached patch PatchSplinter Review
Make sure we iterate all predecessors, even if we remove one.
I think this fixes all crashes we were seeing.

I'm not sure why the fuzzers haven't complained about this yet. Looking at what is wrong I would assume this would have been a fuzzblocker with "Assertion failure: hasLastIns()". Did anybody of you have such signatures? Could be hard to reduce to a single testcase. Just asking to be sure that is the case. Else I'll need to investigate further why this is hard to detect for fuzzers.
Assignee: nobody → hv1989
Flags: needinfo?(hv1989)
Flags: needinfo?(gary)
Flags: needinfo?(choller)
Attachment #8818579 - Flags: review?(jdemooij)
Comment on attachment 8818579 [details] [diff] [review]
Patch

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

Good catch!

::: js/src/jit/MIRGraph.cpp
@@ +195,5 @@
>              for (size_t j = 0; j < block->numPredecessors(); j++) {
>                  if (!block->getPredecessor(j)->isMarked())
>                      continue;
>                  block->removePredecessor(block->getPredecessor(j));
> +                j--;

This will underflow when j is 0 and then the j++ following it will overflow to 0 again. For unsigned types like size_t this is well-defined and OK, but it might be simpler to remove the j++ in the loop head and do j++ before the continue.
Attachment #8818579 - Flags: review?(jdemooij) → review+
(In reply to Hannes Verschore [:h4writer] from comment #9)

> I'm not sure why the fuzzers haven't complained about this yet. Looking at
> what is wrong I would assume this would have been a fuzzblocker with
> "Assertion failure: hasLastIns()". Did anybody of you have such signatures?

Nope, didn't see it triggered.
Flags: needinfo?(gary)
Crash Signature: [@ js::jit::MBasicBlock::insertBefore ] → [@ js::jit::MBasicBlock::insertBefore ] [@ js::jit::MBasicBlock::numSuccessors ] [@ js::jit::IonBuilder::traverseBytecode ] [@ js::jit::BacktrackingAllocator::buildLivenessInfo] [@ js::jit::BacktrackingAllocator::moveAtEntry ] [@ js::jit::Backtrackin…
Summary: Firefox Nightly 53.0a1 (2016-12-10) crashes in [@ js::jit::MBasicBlock::insertBefore ] → Firefox Nightly 53.0a1 (2016-12-10) crashes in [@ js::jit:: ... ]
Pushed by hv1989@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7efb1c8b5dfb
IonMonkey: Make sure all predecessors are iterated during removal of successor blocks, r=jandem
Crash Signature: js::jit::BacktrackingAllocator::resolveControlFlow ] → js::jit::BacktrackingAllocator::resolveControlFlow ] [@ js::jit::BacktrackingAllocator::mergeAndQueueRegisters]
https://hg.mozilla.org/mozilla-central/rev/7efb1c8b5dfb
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
No crashes since Firefox Nightly 53.0a1 (2016-12-16) build,
so I'm marking this bug as verified.
Status: RESOLVED → VERIFIED
(In reply to Hannes Verschore [:h4writer] from comment #9)
> Created attachment 8818579 [details] [diff] [review]
> Patch
> 
> Make sure we iterate all predecessors, even if we remove one.
> I think this fixes all crashes we were seeing.
> 
> I'm not sure why the fuzzers haven't complained about this yet. Looking at
> what is wrong I would assume this would have been a fuzzblocker with
> "Assertion failure: hasLastIns()". Did anybody of you have such signatures?
> Could be hard to reduce to a single testcase. Just asking to be sure that is
> the case. Else I'll need to investigate further why this is hard to detect
> for fuzzers.

Sorry for the delayed response. I don't remember seeing this, for sure not as a fuzzblocker.
Flags: needinfo?(choller)
Crash Signature: [@ js::jit::MBasicBlock::insertBefore ] [@ js::jit::MBasicBlock::numSuccessors ] [@ js::jit::IonBuilder::traverseBytecode ] [@ js::jit::BacktrackingAllocator::buildLivenessInfo] [@ js::jit::BacktrackingAllocator::moveAtEntry ] [@ js::jit::Backtrackin… → [@ js::jit::MBasicBlock::insertBefore ] [@ js::jit::MBasicBlock::numSuccessors ] [@ js::jit::MBasicBlock::numSuccessors const ] [@ js::jit::IonBuilder::traverseBytecode ] [@ js::jit::BacktrackingAllocator::buildLivenessInfo ] [@ js::jit::Backtracking…
Summary: Firefox Nightly 53.0a1 (2016-12-10) crashes in [@ js::jit:: ... ] → Firefox Nightly 53.0a1 (2016-12-10) crashes in [@ js::jit:: ... ] & [@ ComputeImmediateDominators ]
Firefox 54.0a2 (2017-04-12) (64-bit) (aurora) just crashed with this listed as related bug. Is this a new thing or this bug also affects 54?

https://crash-stats.mozilla.com/report/index/695ca973-da00-457c-bdfe-cddd12170413
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #23)
> Firefox 54.0a2 (2017-04-12) (64-bit) (aurora) just crashed with this listed
> as related bug. Is this a new thing or this bug also affects 54?
> 
> https://crash-stats.mozilla.com/report/index/695ca973-da00-457c-bdfe-
> cddd12170413

Looks as a few of the signatures have crashes in 54/55:

js::jit::BacktrackingAllocator::buildLivenessInfo (April)
js::jit::BuildDominatorTree (Feb, March and April)

54 only: js::jit::BacktrackingAllocator::resolveControlFlow (crashes in March/April)

So it sounds like we need to file new bugs for any new issues.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: