1276724 - Crash in OOM | large | mozalloc_abort | mozalloc_handle_oom | moz_xmalloc | nsTArray_base<T>::EnsureCapacity<T> | mozilla::safebrowsing::HashStore::WriteAddPrefixes and elsewhere in url-classifier

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P1, critical)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-9237bb75-6fef-48e3-a659-42cc82160523.
=============================================================
Crashing Thread (48)
Frame 	Module 	Signature 	Source
0 	mozglue.dll 	mozalloc_abort(char const* const) 	memory/mozalloc/mozalloc_abort.cpp:33
1 	mozglue.dll 	mozalloc_handle_oom(unsigned int) 	memory/mozalloc/mozalloc_oom.cpp:46
2 	mozglue.dll 	moz_xmalloc 	memory/mozalloc/mozalloc.cpp:85
3 	xul.dll 	nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>::EnsureCapacity<nsTArrayInfallibleAllocator>(unsigned int, unsigned int) 	xpcom/glue/nsTArray-inl.h:136
4 	xul.dll 	mozilla::safebrowsing::HashStore::WriteAddPrefixes(nsIOutputStream*) 	toolkit/components/url-classifier/HashStore.cpp:777
5 	xul.dll 	mozilla::safebrowsing::Classifier::ApplyTableUpdates(nsTArray<mozilla::safebrowsing::TableUpdate*>*, nsACString_internal const&) 	toolkit/components/url-classifier/Classifier.cpp:663
6 	xul.dll 	nsUrlClassifierDBServiceWorker::CacheCompletions(nsTArray<mozilla::safebrowsing::CacheResult>*) 	toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:700
7 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:995

this oom crash seems to occur on various windows systems and on android as well. on 47.0b9 this signature is currently around #40 by volume.

Comment 1

[Calixte Denizet (:calixte)]

Crash volume for signature 'OOM | large | mozalloc_abort | mozalloc_handle_oom | moz_xmalloc | nsTArray_base<T>::EnsureCapacity<T> | mozilla::safebrowsing::HashStore::WriteAddPrefixes':
  - esr (45): 678

Affected platform: Windows

Comment 2

[Nicholas Nethercote [:njn]]

These crashes have occurred about 2300 times in the past 7 days across all versions of Firefox. Fixing them is easy.

Comment 3

[Nicholas Nethercote [:njn]]

Attachment: Make two url-classifier allocations fallible

This addresses two crashes in the top #75 on 47.0.1.

Comment 4

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8775848 [details] [diff] [review]
Make two url-classifier allocations fallible

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

LGTM. The first failure is correctly detected upwards as OOM and won't result in a database reset, and the second one is before the old PrefixSet is cleared, so it won't silently disable SafeBrowsing.

Comment 5

[Nicholas Nethercote [:njn]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/19c9fa346278d7fabd7a4dfebd4cee0c263ec551
Bug 1276724 - Make two url-classifier allocations fallible. r=gcp.

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/19c9fa346278

Comment 7

[[:philipp]]

the signature is currently causing 0.25% of browser crashes on 49.0b.
could we uplift the patch to 49 in terms of the scope/risk?

Comment 8

[Nicholas Nethercote [:njn]]

Comment on attachment 8775848 [details] [diff] [review]
Make two url-classifier allocations fallible

Approval Request Comment

[Feature/regressing bug #]: safebrowsing

[User impact if declined]: OOM crashes, currently 0.25% of crashes on beta.

[Describe test coverage new/current, TreeHerder]: landed on m-c 13 days ago. No specific tests.

[Risks and why]: Very low. Patch is trivial, it just makes two allocations fallible. All callers to the two functions containing those allocations appropriately check for failure, so any OOM should be handled well. 

[String/UUID change made/needed]: none.

Comment 9

[Liz Henry (:lizzard)]

Comment on attachment 8775848 [details] [diff] [review]
Make two url-classifier allocations fallible

Crash fix, let's give it a try on beta.

Comment 11

[Wes Kocher (:KWierso)]

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

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Too late for 48

Comment 13

[Trevor Skywalker]

[Tracking Requested - why for this release]:

Signature report for OOM | large | mozalloc_abort | mozalloc_handle_oom | moz_xmalloc | je_free | mozilla::safebrowsing::LookupCache::ConstructPrefixSet

Showing results from 6 months ago

Operating System
Windows 7 	778 	81.9%
Windows 8.1 	67 	7.1%
Windows 10 	62 	6.5%
Windows Vista 	19 	2.0%
Windows XP 	17 	1.8%
Windows 8 	6 	0.6%
WindowsServer03	1 	0.1%

Product
Firefox	45.9.0esr 	20 	57.1% 	28
Firefox	45.3.0esr 	2 	5.7% 	2
Firefox	45.4.0esr 	2 	5.7% 	2
Firefox	45.5.1esr 	2 	5.7% 	2
Firefox	45.8.0esr 	2 	5.7% 	2
Firefox 	46.0.1 	2 	5.7% 	2
Firefox 	43.0.1 	1 	2.9% 	1
Firefox	45.6.0esr 	1 	2.9% 	1
Firefox 	47.0.1 	1 	2.9% 	1
Firefox 	47.0.2 	1 	2.9% 	1
Firefox	48.0b99 	1 	2.9% 	1

Architecture
x86 	950 	100.0%