Closed Bug 1571682 Opened 6 months ago Closed 5 months ago

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

Categories

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

x86
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- fixed
firefox68 --- wontfix
firefox69 --- fixed
firefox70 --- fixed

People

(Reporter: marcia, Assigned: jonco)

References

(Regression)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

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

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.

Assignee: nobody → jcoppeard
Priority: -- → P1

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.

Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d8d687ca13f8
Make allocationSiteGroup fail cleanly on OOM r=tcampbell,jandem?
Backout by malexandru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/17ef275f8a79
Backed out changeset d8d687ca13f8 for causing spidermonkey bustages. CLOSED TREE
Attachment #9084052 - Attachment description: Bug 1571682 - Make allocationSiteGroup fail cleanly on OOM r=tcampbell? → Bug 1571682 - Make allocationSiteGroup fail cleanly on OOM r=jandem
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/36da938c1552
Make allocationSiteGroup fail cleanly on OOM r=tcampbell,jandem
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

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!

Duplicate of this bug: 1574796

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:
Flags: needinfo?(jcoppeard)
Attachment #9084052 - Flags: approval-mozilla-beta?
Crash Signature: [@ OOM | unknown | js::AutoEnterOOMUnsafeRegion::crash | js::MovableCellHasher<T>::hash] → [@ OOM | unknown | js::AutoEnterOOMUnsafeRegion::crash | js::MovableCellHasher<T>::hash] [@ OOM | unknown | js::AutoEnterOOMUnsafeRegion::crash | JS::WeakCache<T>::lookupForAdd]

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

JS stability fix. Approved for 69.0b15.

Attachment #9084052 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Crash Signature: [@ OOM | unknown | js::AutoEnterOOMUnsafeRegion::crash | js::MovableCellHasher<T>::hash] [@ OOM | unknown | js::AutoEnterOOMUnsafeRegion::crash | JS::WeakCache<T>::lookupForAdd] → [@ OOM | unknown | js::AutoEnterOOMUnsafeRegion::crash | js::MovableCellHasher<T>::hash] [@ OOM | unknown | js::AutoEnterOOMUnsafeRegion::crash | JS::WeakCache<T>::lookupForAdd]

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

Flags: needinfo?(jcoppeard)

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:
Flags: needinfo?(jcoppeard)
Attachment #9084052 - Flags: approval-mozilla-esr68?

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

Attachment #9084052 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
You need to log in before you can comment on or make changes to this bug.