Closed Bug 1166349 Opened 5 years ago Closed 5 years ago

crash in OOM | large | NS_ABORT_OOM(unsigned int) | nsACString_internal::Replace(unsigned int, unsigned int, char const*, unsigned int)

Categories

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

x86
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox38.0.5 + wontfix
firefox39 --- fixed
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: dmajor, Assigned: dmajor)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 2 obsolete files)

This bug was filed from the Socorro interface and is 
report bp-ef0d5b78-31e2-4fbb-9e65-528d82150515.
=============================================================

This is a top crash on 38.0.1 (by the old metrics at least). Don't trust the stack on crash-stats. It actually goes like:

xul!nsACString_internal::Replace
xul!nsACString_internal::Append
xul!nsXMLHttpRequest::StreamReaderFunc

It's clear that the code is prepared to handle allocation failure. I guess it just needs to make a fallible call.
Attached patch fallible allocation (obsolete) — Splinter Review
Assignee: nobody → dmajor
Attachment #8607613 - Flags: review?(jonas)
Attached patch fallible allocation (obsolete) — Splinter Review
Sorry, had some unrelated files in the previous patch
Attachment #8607613 - Attachment is obsolete: true
Attachment #8607613 - Flags: review?(jonas)
Attachment #8607617 - Flags: review?(jonas)
[Tracking Requested - why for this release]: Moderate topcrash on 38.0.1; easy fix
Comment on attachment 8607617 [details] [diff] [review]
fallible allocation

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

This looks good to me, though it'd be good to add a test. But maybe such a test would take too long to run?
Attachment #8607617 - Flags: review?(jonas) → review+
Comment on attachment 8607617 [details] [diff] [review]
fallible allocation

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

Leaving it up to Olli if we need tests or not.
Attachment #8607617 - Flags: review?(bugs)
Attachment #8607617 - Flags: review?(bugs) → review+
I don't know how we could test this reliably. OOM might happen on server side, or if using
data: urls, it might happen while creating the url (either on JS side or in C++)
FWIW, I consider the stability people who constantly watch crash-stats, to be a kind of test :)
I thought about just landing this to fix the bustage, but given that this may be rushed up to beta, another review seemed prudent.
Attachment #8607617 - Attachment is obsolete: true
Attachment #8607876 - Flags: review?(bugs)
Attachment #8607876 - Flags: review?(bugs) → review+
Ok, if this fails, I give up on this bug >:(

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a80bba91d119
https://hg.mozilla.org/mozilla-central/rev/529142f49290
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Take that, Werror!
Comment on attachment 8607876 [details] [diff] [review]
fallible allocation

For some reason this has dropped in volume on 38.0.1 since I filed the bug. If I were looking at the crash lists today, I wouldn't open a bug on this. But we already have the patch and it's an easy fix... relman I leave it up to you.

Approval Request Comment
[Feature/regressing bug #]: memory exhaustion
[User impact if declined]: OOM crashes that we might have otherwise survived
[Describe test coverage new/current, TreeHerder]: green m-i
[Risks and why]: trivial change
[String/UUID change made/needed]: none
Attachment #8607876 - Flags: approval-mozilla-release?
Attachment #8607876 - Flags: approval-mozilla-beta?
Attachment #8607876 - Flags: approval-mozilla-aurora?
Comment on attachment 8607876 [details] [diff] [review]
fallible allocation

Let's take this for aurora and beta. This may help us forestall other OOM errors. And, David has defeated the mighty Werror.
Attachment #8607876 - Flags: approval-mozilla-beta?
Attachment #8607876 - Flags: approval-mozilla-beta+
Attachment #8607876 - Flags: approval-mozilla-aurora?
Attachment #8607876 - Flags: approval-mozilla-aurora+
Comment on attachment 8607876 [details] [diff] [review]
fallible allocation

This will ride the train from 39.
Attachment #8607876 - Flags: approval-mozilla-release? → approval-mozilla-release-
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.