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

Status: RESOLVED FIXED

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

Description

[(Away)]

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.

Comment 1

[(Away)]

Attachment: fallible allocation

Comment 2

[(Away)]

Attachment: fallible allocation

Sorry, had some unrelated files in the previous patch

Comment 3

[(Away)]

[Tracking Requested - why for this release]: Moderate topcrash on 38.0.1; easy fix

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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?

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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++)

Comment 7

[(Away)]

FWIW, I consider the stability people who constantly watch crash-stats, to be a kind of test :)

Comment 8

[Pulsebot]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Backed out for Werror bustage.
https://treeherder.mozilla.org/logviewer.html#?job_id=9981697&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/39f481e86829

Comment 10

[(Away)]

Attachment: fallible allocation

I thought about just landing this to fix the bustage, but given that this may be rushed up to beta, another review seemed prudent.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9dfe96e082e6

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Backed out for Werror bustage.
https://treeherder.mozilla.org/logviewer.html#?job_id=10003610&repo=mozilla-inbound

Comment 13

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1757ff5613db

Comment 14

[(Away)]

Ok, if this fails, I give up on this bug >:(

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a80bba91d119

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/529142f49290

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/529142f49290

Comment 17

[(Away)]

Take that, Werror!

Comment 18

[(Away)]

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

Comment 19

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/055f05c95d1a

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/139bb92cdcc3

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8607876 [details] [diff] [review]
fallible allocation

This will ride the train from 39.