1131348 - Large OOMs in content serialize code

Status: RESOLVED FIXED

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

Description

[(Away)]

This bug was filed from the Socorro interface and is 
report bp-008f38d1-704c-4c08-946d-ee8442150202.
=============================================================

In total about 0.72% of beta crashes. Usually 500k or 1M allocations. Socorro puts some garbage on the stacks but generally the real stacks go like:

> nsAString_internal::Replace(unsigned int, unsigned int, nsAString_internal const&)
> nsDocumentEncoder::SerializeNodeStart(nsINode*, int, int, nsAString_internal&, nsINode*)
> nsDocumentEncoder::SerializeToStringRecursive(nsINode*, nsAString_internal&, bool, unsigned int)

> nsAString_internal::Replace(unsigned int, unsigned int, nsAString_internal const&)
> nsXMLContentSerializer::AppendToString(nsAString_internal const&, nsAString_internal&)
> nsXMLContentSerializer::AppendToStringConvertLF(nsAString_internal const&, nsAString_internal&)
> nsXMLContentSerializer::SerializeAttr(nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, nsAString_internal&, bool)

Comment 1

[(Away)]

How invasive would it be to make these fallible?

Comment 2

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

Hmm, do we have proper stacks for these crashes? Like showing where the calls are coming?
But yeah, I think we could make this stuff fallible.
htmlelement.innerHTML uses different, faster code paths these days anyway.

Comment 3

[(Away)]

They come from JS calls to XMLSerializerBinding::serializeToString.

More stacks available here: https://crash-stats.mozilla.com/report/list?product=Firefox&signature=OOM+|+large+|+NS_ABORT_OOM%28unsigned+int%29+|+nsAString_internal%3A%3AReplace%28unsigned+int%2C+unsigned+int%2C+nsAString_internal+const%26%29#tab-reports

(Just ignore the frames that don't make any sense)

Comment 4

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

Oh, it is XMLSerializer. Not at all what I thought it would be.

Anyhow, better to throw some sane exception to the web pages using XMLSerializer and causing OOM.


Henri, would you have time to look at this?

Comment 5

[Henri Sivonen (:hsivonen)]

(In reply to Olli Pettay [:smaug] (no new review requests before Feb 22, please) from comment #4)
> Henri, would you have time to look at this?

I don't, unfortunately.

Comment 6

[Robert Kaiser]

[Tracking Requested - why for this release]:
The crash signature of this bug is #8 with ~1.4% of all 37.0b3 crashes, I think we should track and put more emphasis on this.

Comment 7

[Lawrence Mandel [:lmandel] (use needinfo)]

Tracking topcrash.

Andrew - Can you please help find an owner?

Comment 8

[Andrew Overholt [:overholt]]

William has offered to investigate.

Comment 9

[Lawrence Mandel [:lmandel] (use needinfo)]

William - We have very little time left in the 37 cycle. Last Beta goes to build on this Thu, Mar 19. Do you have an update on this top crash?

Comment 10

[William Chen [:wchen]]

Attachment: Use fallible append in content serializers.

Here is a patch that converts a lot of the Append calls (which looks like is the caller of Replace that OOMs) to the fallible version.

Unfortunately this patch makes the code ugly, and it's quite invasive. I fixed a few instances manually before I gave up and used VIM commands to mass replace the rest (I did look over the changes and made adjustments before uploading the patch).

Almost all the crash reports are missing frames that I want to see. From the few that actually show frames from the serializer, it seems like nsXMLContentSerializer::AppendToString is eventually calling nsAString_internal::ReplaceReplace that causes the OOM. Even changing only nsXMLContentSerializer::AppendToString to be infallible is very invasive.

Alternatively, we can try to be more precise and only use the fallible version of AppendToString in nsXMLContentSerializer::AppendElementStart because that's the only function I have found in the crash reports, see https://crash-stats.mozilla.com/report/index/1ec97c5b-fea9-432c-a940-0b2002150310 for an example of a more complete stack. But these stacks are fairly rare and the OOM can easily be happening in other places. Let me know if you rather take the more precise approach and I can get a new patch up pretty quickly.

Comment 11

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

Comment on attachment 8578517 [details] [diff] [review]
Use fallible append in content serializers.

String API changes should be reviewed by someone else.

Comment 12

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

The main issue the patch has that some methods return bool and some nsresult.
So whenever NS_ENSURE_TRUE(somemethod(),) is used, one needs to check whether the method return bool
or nsresult.

Comment 13

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

Comment on attachment 8578517 [details] [diff] [review]
Use fallible append in content serializers.

>
> nsXMLContentSerializer::AppendElementStart(Element* aElement,
>                                            Element* aOriginalElement,
>                                            nsAString& aStr)
> {
>   NS_ENSURE_ARG(aElement);
> 
>   nsIContent* content = aElement;
> 
>   bool forceFormat = false;
>-  if (!CheckElementStart(content, forceFormat, aStr)) {
>-    return NS_OK;
>+  nsresult rv = NS_OK;
>+  if (!CheckElementStart(content, forceFormat, aStr, rv)) {
>+    return rv;
>   }
> 
>+  NS_ENSURE_SUCCESS(rv, NS_ERROR_OUT_OF_MEMORY);
Nit, NS_ENSURE_SUCCESS(rv, rv); would make more sense

Comment 14

[Nathan Froyd [:froydnj]]

Comment on attachment 8578517 [details] [diff] [review]
Use fallible append in content serializers.

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

String parts look good.  r=me

Comment 15

[Nathan Froyd [:froydnj]]

Comment on attachment 8578517 [details] [diff] [review]
Use fallible append in content serializers.

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

Sigh, bugzilla.  Re-adding Olli's r+.

Comment 16

[William Chen [:wchen]]

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

Comment 17

[Carsten Book [:Tomcat]]

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

Comment 18

[Lawrence Mandel [:lmandel] (use needinfo)]

This but affects 37 but the William and I agree that the fix is too risky to take right at the end of the Beta cycle. If the fix is shown to be good in Nightly, let's look at uplifting to 38.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

William, please let me know when you know if you want to uplift that to 38. Thanks

Comment 20

[William Chen [:wchen]]

Comment on attachment 8578517 [details] [diff] [review]
Use fallible append in content serializers.

Approval Request Comment
[Feature/regressing bug #]: Feature has been around since hg@1
[User impact if declined]: Browser crashing when trying to serialize a large DOM tree.
[Describe test coverage new/current, TreeHerder]: Passes all tests on TreeHerder. No new serializer bugs since landing on m-c.
[Risks and why]: Low, each change causes functions to return early instead of crashing in OOM situations. The biggest risk I can think of is that returning early in some functions may leave the serializer with an incorrect internal state and cause it to return incorrect output.
[String/UUID change made/needed]: None

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d6d3d4e918eb