1448089 - Crash in OOM | unknown | js::AutoEnterOOMUnsafeRegion::crash | js::jit::MBasicBlock::addPredecessorSameInputsAs

Status: RESOLVED FIXED

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

Description

[Mingzheng Ji]

This bug was filed from the Socorro interface and is
report bp-41a47bd8-d567-4535-a6bc-65f841180322.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll js::AutoEnterOOMUnsafeRegion::crash js/src/jscntxt.cpp:1651
1 xul.dll js::jit::MBasicBlock::addPredecessorSameInputsAs js/src/jit/MIRGraph.cpp:1169
2 xul.dll UpdateTestSuccessors js/src/jit/IonAnalysis.cpp:773
3 xul.dll MaybeFoldConditionBlock js/src/jit/IonAnalysis.cpp:893
4 xul.dll js::jit::OptimizeMIR js/src/jit/Ion.cpp:1480
5 xul.dll js::jit::CompileBackEnd js/src/jit/Ion.cpp:1980
6 xul.dll js::HelperThread::handleIonWorkload js/src/vm/HelperThreads.cpp:1908
7 xul.dll js::HelperThread::threadLoop js/src/vm/HelperThreads.cpp:2297
8 xul.dll js::detail::ThreadTrampoline<void  js/src/threading/Thread.h:235
9 ucrtbase.dll thread_start<unsigned int > 

=============================================================

Comment 1

[Nicolas B. Pierron [:nbp]]

This bug sounds easily fixable by making addPredecessorSameInputAs fallible by using the Result type.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: Make MBasicBlock::addPredecessorSameInputsAs fallible.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/131166cff47d
Make MBasicBlock::addPredecessorSameInputsAs fallible. r=tcampbell

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9f508d0e46e
followup, remove now-unused variable to fix a CLOSED TREE.

Comment 5

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/131166cff47d
https://hg.mozilla.org/mozilla-central/rev/d9f508d0e46e

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Whether we ship this in Fx60 or not, I'd like to keep this on the radar for possible ESR60 uplift eventually since the crash volume looks non-trivial.

Comment 7

[Nicolas B. Pierron [:nbp]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #6)
> Whether we ship this in Fx60 or not, I'd like to keep this on the radar for
> possible ESR60 uplift eventually since the crash volume looks non-trivial.

Yes, these patches should be safe to uplift, and remove ~175 crashes/week when it moves to release.

(In reply to Pulsebot from comment #4)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/d9f508d0e46e
> followup, remove now-unused variable to fix a CLOSED TREE.

Thanks.

Comment 8

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8969301 [details] [diff] [review]
Make MBasicBlock::addPredecessorSameInputsAs fallible.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1028580
[User impact if declined]: Unlikely crashes on OOM.
[Is this code covered by automated tests?]: No, not easy to cover error handling code.
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]:  No
[List of other uplifts needed for the feature/fix]: (both patches from this bug)
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Usage of MOZ_MUST_USE and simple patch.
[String changes made/needed]: None.

Comment 9

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Comment on attachment 8969301 [details] [diff] [review]
Make MBasicBlock::addPredecessorSameInputsAs fallible.

Fix for OOM crash, let's uplift for beta 15.

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/dc7d484e8d3d
https://hg.mozilla.org/releases/mozilla-beta/rev/e378f069dee4