Closed Bug 1422575 Opened 2 years ago Closed Last year

Crash in OOM | unknown | js::AutoEnterOOMUnsafeRegion::crash | js::gc::StoreBuffer::put<T>

Categories

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

Unspecified
Windows 10
defect

Tracking

()

RESOLVED WORKSFORME
mozilla59
Tracking Status
firefox-esr52 --- wontfix
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: jseward, Assigned: jonco)

References

Details

(Keywords: crash)

Crash Data

Attachments

(2 files, 1 obsolete file)

This bug was filed from the Socorro interface and is
report bp-ac1b9ecf-861a-4edb-9abf-487340171201.
=============================================================

This is topcrash #8 in the windows nightly 20171130220131.

Top 8 frames of crashing thread:

0 xul.dll js::AutoEnterOOMUnsafeRegion::crash js/src/jscntxt.cpp:1651
1 xul.dll js::gc::StoreBuffer::put&lt;js::gc::StoreBuffer::MonoTypeBuffer&lt;js::gc::StoreBuffer::CellPtrEdge&gt;, js::gc::StoreBuffer::CellPtrEdge&gt; js/src/gc/StoreBuffer.h:372
2 xul.dll js::jit::ICCall_Scripted::ICCall_Scripted js/src/jit/BaselineIC.cpp:4672
3 xul.dll js::jit::ICStubCompiler::newStub&lt;js::jit::ICCall_Scripted, js::jit::ICStubSpace*&amp;, js::jit::JitCode*, js::jit::ICStub*&amp;, JS::Rooted&lt;JSFunction*&gt;&amp;, JS::Rooted&lt;JSObject*&gt;&amp;, unsigned int&amp;&gt; js/src/jit/SharedIC.h:1131
4 xul.dll js::jit::TryAttachCallStub js/src/jit/BaselineIC.cpp:2306
5 xul.dll js::jit::DoCallFallback js/src/jit/BaselineIC.cpp:2529
6  @0x144f5f77621 
7 xul.dll xul.dll@0xb0112b 

=============================================================
Flags: needinfo?(jcoppeard)
Priority: -- → P1
This is OOM, but there may be some things we can do to mitigate this.
Attached patch bug1422575-storebuffer-oom (obsolete) — Splinter Review
Here's a patch to pre-allocate the hash sets used by the store buffer.  We  request a minor GC before we reach the actual capacity is reached since the size of the hash table is actually a multiple of the initial size requested.

However it's kind of nasty adding a special removeWithoutResize() API just for this purpose.  I considered adding another template parameter for minimum hash table size, but that's not great either.  What do you think?

(Also this patch makes the size of the hash set the same on 32 and 64 bit platforms which it wasn't before.  The value I picked was about half way between the original sizes for the different platforms.)
Assignee: nobody → jcoppeard
Flags: needinfo?(jcoppeard)
Attachment #8935004 - Flags: review?(sphink)
Comment on attachment 8935004 [details] [diff] [review]
bug1422575-storebuffer-oom

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

How is this showing up as the #8 crasher?!

The API addition is unfortunate. Especially since if you need a barriered table, there's yet another entry point to consider.

Also, it seems a little weird to have a storebuffer use a hashtable in a different way in order to get the nonshrinking behavior. I guess it seems better to me to have a nonshrinking hashtable. This seems like it wants to add a new invariant, and that's never going to work (though it'll probably work well enough) without the hashtable implementation maintaining it. For example, if someone calls clearAndShrink(), it'll happily shrink (which is probably fine, since you explicitly told it to.) But more problematic, if you ever remove something using an Enum, currently it could also shrink. You'd need a NonShrinkingEnum or something to plug that hole. Which isn't a fatal flaw, you're just saying that it's up to the user to indirectly manage the table's minimum capacity by not calling things that would shrink it, but it's nonobvious what that means.

What do you think of adding a minCapacity field, initialized to sMinCapacity but controllable with a setMinCapacity() or something? Hopefully you could mostly then just s/sMinCapacity/minCapacity/, though it does look like there's some funky logic around picking the actual capacity in init(). But maybe using it in wouldBeUnderloaded() is enough.

We really need to switch hashtable implementations, I think. froydnj is doing this for the Gecko one, switching to a Robin Hood hashing variant. Oh well; different bug.
Attachment #8935004 - Flags: review?(sphink)
Patch to give hash tables a minimum capacity based on the value passed to init().

This makes the generation field debug-only; it was never used outside debug builds anyway.  The space is repurposed for the minCapacity field.
Attachment #8935004 - Attachment is obsolete: true
Attachment #8935410 - Flags: review?(sphink)
As before, pre-allocate the store buffer hash sets.
Attachment #8935412 - Flags: review?(sphink)
Comment on attachment 8935410 [details] [diff] [review]
bug1422575-hashtable-min-capacity

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

lgtm, thanks!

::: js/public/HashTable.h
@@ +1366,5 @@
>                 capacity() * sMaxAlphaNumerator / sAlphaDenominator;
>      }
>  
>      // Would the table be underloaded if it had the given capacity and entryCount?
> +    bool wouldBeUnderloaded(uint32_t capacity)

The comment probably ought to be updated. Maybe

// Considering its current entryCount, would the table be underloaded if it had the given capacity?
Attachment #8935410 - Flags: review?(sphink) → review+
Attachment #8935412 - Flags: review?(sphink) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/115d70e6c818
Give hash tables a minimum capacity based on the parameter passed to init() r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/1938afc34193
Give store buffer hash sets a minimum capacity to reduce the chance of OOM while adding entries r=sfink
https://hg.mozilla.org/mozilla-central/rev/115d70e6c818
https://hg.mozilla.org/mozilla-central/rev/1938afc34193
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Depends on: 1424324
Crash volume doesn't look worth backport consideration, but feel free to set the status for 58 back to affected and request approval if you feel strongly otherwise.
Backed out for perf and memory regressions:

https://hg.mozilla.org/integration/mozilla-inbound/rev/12a50b0af80f3c82a63dc408736ea17192d63f3d
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Memory use seems to have returned to previous levels.
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → WORKSFORME
Wrong bug.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
This crash signature seems to be gone in 63.
Status: REOPENED → RESOLVED
Closed: 2 years agoLast year
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.