1027163 - Large OOM in mozilla::dom::DOMStorage::SetItem

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Robert Kaiser]

This bug was filed from the Socorro interface and is 
report bp-a713f0ac-de7d-4d5e-99c3-2aa692140616.
=============================================================

Top frames:
0 	xul.dll 	NS_ABORT_OOM(unsigned int) 	xpcom/base/nsDebugImpl.cpp
1 	xul.dll 	nsAString_internal::Assign(nsAString_internal const &) 	xpcom/string/src/nsTSubstring.cpp
2 	xul.dll 	nsIDOMStorage_SetItem 	obj-firefox/js/xpconnect/src/dom_quickstubs.cpp
3 	mozjs.dll 	js::Invoke(JSContext *,JS::CallArgs,js::MaybeConstruct) 	js/src/vm/Interpreter.cpp
4 	mozjs.dll 	Interpret 	js/src/vm/Interpreter.cpp

This seems to be common for all the reports I checked in this list: https://crash-stats.mozilla.com/report/list?signature=OOM%20%7C%20large%20%7C%20NS_ABORT_OOM%28unsigned%20int%29%20%7C%20nsAString_internal%3A%3AAssign%28nsAString_internal%20const%26%29

The OOM crashes happen at least on Windows and Android, across different Firefox versions, with different large allocation sizes, the reports linked at the top of this comment for example is doing a >50M allocation, and bp-6b114c26-4960-4209-ace3-0765e2140616 on Android is doing a 2M allocation.

We should do fallible instead of infallible allocations here.

Comment 1

[(not currently active) Ted Mielczarek]

Yeah, wow, this is directly callable from content JS, I'm amazed we haven't noticed this before.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Well string allocation was fallible by default until relatively recently ...

Comment 3

[Robert Kaiser]

Hmm, actually, the second signature I added here is mostly mozilla::storage::Statement::GetString, let's leave that low-volume one off the bug for now, it may be other code paths that need to be fixed there.

Comment 4

[Robert Kaiser]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #2)
> Well string allocation was fallible by default until relatively recently ...

Not sure if Firefox 14, which apparently was the last before this patch, can still be considered "relatively recent" ;-)

That said, I wonder if nsAString_internal::Assign ought to go on the Socorro signature prefix list, i.e. if we should append the next frame on those signatures.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8443548 [details] [diff] [review]
Patch

r=me

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/c19185460878

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c19185460878

Comment 9

[alex_mayorga]

FWIW my SO's Firefox beta 31.0 crashed like this:

bp-b7675b1d-aa1d-4919-991b-647742140721	21/07/2014	12:19 p.m.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This landed in 33.

Comment 11

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Looking in Socorro [1], there still seem to be some 143 crashes over the past two weeks on Beta and Aurora: 
- 139 crashes on Firefox 33 Beta 
- 4 crashes on Firefox 34 Aurora

Given this number of crashes I'm not sure whether this has improved and is enough to call this fixed or not. Any thoughts?

[1] - https://crash-stats.mozilla.com/report/list?signature=OOM+%7C+large+%7C+NS_ABORT_OOM%28unsigned+int%29+%7C+nsAString_internal%3A%3AAssign%28nsAString_internal+const%26%29&product=Firefox&query_type=contains&range_unit=weeks&process_type=any&version=Firefox%3A35.0a1&version=Firefox%3A34.0a2&version=Firefox%3A33.0b&hang_type=any&date=2014-09-22+15%3A00%3A00&range_value=2#tab-reports

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

You're looking at the signatures for all crashes in nsAString_internal::Assign, not just the ones that are caused by this bug.  I picked 5 at random and didn't see any with DOM storage in the stack.  Most seem to go through mozilla::css::ErrorReporter::ReportUnexpected.

Comment 13

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #12)
> You're looking at the signatures for all crashes in
> nsAString_internal::Assign, not just the ones that are caused by this bug. 
> I picked 5 at random and didn't see any with DOM storage in the stack.  Most
> seem to go through mozilla::css::ErrorReporter::ReportUnexpected.

Thanks Kyle... it seems you are 100% right, as none of the crashes from the past 4 weeks go through DOMStorage, and most of them go through ErrorReporter. Since this was about the DOM Storage crashes, I'm marking it as Verified.

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

ni dbaron to see if he wants to look at the ones that go through CSS e.g.

https://crash-stats.mozilla.com/report/index/9037bde0-9c60-4824-beaa-ece562140910
https://crash-stats.mozilla.com/report/index/df79c6ea-d9f9-4055-b68e-60ef82140910