1571682 - Crash in [@ OOM | unknown | js::AutoEnterOOMUnsafeRegion::crash | js::MovableCellHasher<T>::hash]

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect, P1)

Description

[Marcia Knous [:marcia]]

This bug is for crash report bp-0faef739-7a79-4bca-928c-1d7970190806.

Similar signature was found and fixed in the 48 timeframe (bug 1260785), but this crash is very visible in 69 beta: https://mzl.la/2MGREgD. Crash volume also seems higher in 68 (#14 overall top crash) release than it was in 67.

Top 10 frames of crashing thread:

0 xul.dll js::AutoEnterOOMUnsafeRegion::crash js/src/vm/JSContext.cpp:1477
1 xul.dll js::MovableCellHasher<JSScript*>::hash js/src/gc/Barrier.cpp:132
2 xul.dll JS::WeakCache<JS::GCHashMap<js::ObjectGroupRealm::AllocationSiteKey, js::WeakHeapPtr<js::ObjectGroup*>, js::ObjectGroupRealm::AllocationSiteKey, js::SystemAllocPolicy, JS::DefaultMapSweepPolicy<js::ObjectGroupRealm::AllocationSiteKey, js::WeakHeapPtr<js::ObjectGroup*> > > >::lookupForAdd js/public/GCHashTable.h:451
3 xul.dll js::ObjectGroup::allocationSiteGroup js/src/vm/ObjectGroup.cpp:1434
4 xul.dll js::NewObjectOperation js/src/vm/Interpreter.cpp:5146
5 xul.dll static bool Interpret js/src/vm/Interpreter.cpp
6 xul.dll js::RunScript js/src/vm/Interpreter.cpp:423
7 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:563
8 xul.dll static bool InternalCall js/src/vm/Interpreter.cpp:590
9 xul.dll js::Call js/src/vm/Interpreter.cpp:606

Comment 1

[Jon Coppeard (:jonco)]

Probably caused by bug 1543055. We need to extract a hasher class for AllocationSiteKey and make it a specialization of MovableCellHasher<>, implementing hasHash() and ensureHash() methods. This will make it fail cleanly on OOM. Then we could also give JSScripts a unique ID as suggested in bug 1543055 comment 2 which could decrease memory use, depending how often scripts are used in tables like this.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Bug 1571682 - Make allocationSiteGroup fail cleanly on OOM r=jandem

This should make this fail cleanly on OOM rather than crashing, which should make this crash go away (without reducing memory usage obviously). The problem was the lack of hasHash/ensureHash methods that we use to handle OOM when generating unique IDs for GC things. I also tidied the equivalent code for ObjectGroupRealm::NewEntry (FallibleHashMethods is already implemented for MovableCellHasher).

We could further improve this by giving each script an immutable hash code on creation if you think it's worth the tradeoff of storing this for every script.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d8d687ca13f8
Make allocationSiteGroup fail cleanly on OOM r=tcampbell,jandem?

Comment 4

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17ef275f8a79
Backed out changeset d8d687ca13f8 for causing spidermonkey bustages. CLOSED TREE

Comment 5

[Alexandru Michis [:malexandru]]

Backed out changeset d8d687ca13f8 for causing spidermonkey bustages.

Backout link: https://hg.mozilla.org/integration/autoland/rev/17ef275f8a79eb2c79b618b62587873fe0acd781

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&fromchange=d8d687ca13f8a8f4b5820e31aa6a6c02413f608a&tochange=17ef275f8a79eb2c79b618b62587873fe0acd781

Failure logs:

• https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=261802973&repo=autoland&lineNumber=5898
• https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=261802988&repo=autoland&lineNumber=89537

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/36da938c1552
Make allocationSiteGroup fail cleanly on OOM r=tcampbell,jandem

Comment 7

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/36da938c1552

Comment 8

[[:philipp]]

do you think we should get this fix to beta/68esr - if you deem fit to do so, can you please request uplifts?
(the crashes on nightly already seem to have stopped with build 20190809215748 which was before the patch here has landed though)

thank you!

Comment 10

[Jon Coppeard (:jonco)]

Comment on attachment 9084052 [details]
Bug 1571682 - Make allocationSiteGroup fail cleanly on OOM r=jandem

Beta/Release Uplift Approval Request

• User impact if declined: Possible crashes on OOM. I don't know how much difference this actually makes as without this fix we might just crash more elsewhere.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This brings the hashing code for AllocationSiteTable into line with all the other uses of MovableCellHasher. It's not a trivial change but it's the same pattern we use everywhere else, so I'd say it's low risk.
• String changes made/needed:

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9084052 [details]
Bug 1571682 - Make allocationSiteGroup fail cleanly on OOM r=jandem

JS stability fix. Approved for 69.0b15.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e1341fad0145

Comment 13

[Ryan VanderMeulen [:RyanVM]]

This is looking good on Nightly and Beta - should we nominate this for ESR68 also?

Comment 14

[Jon Coppeard (:jonco)]

Comment on attachment 9084052 [details]
Bug 1571682 - Make allocationSiteGroup fail cleanly on OOM r=jandem

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This improves our OOM handling.
• User impact if declined: More likely to crash in low memory situations.
• Fix Landed on Version: 70
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This brings the hashing code for AllocationSiteTable into line with all the other uses of MovableCellHasher. It's not a trivial change but it's the same pattern we use everywhere else, so I'd say it's low risk.
• String or UUID changes made by this patch:

Comment 15

[Julien Cristau [:jcristau]]

Comment on attachment 9084052 [details]
Bug 1571682 - Make allocationSiteGroup fail cleanly on OOM r=jandem

avoid some OOM crashes, verified in beta; approved for 68.1

Comment 16

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

https://hg.mozilla.org/releases/mozilla-esr68/rev/a975570ee8f1