1269365 - OOMs during deserialization of nsTArray should result in an OOM crash

Status: RESOLVED FIXED

(Core :: IPC, defect, P1)

Description

[Andrew McCreight [:mccr8]]

ParamTraits<InfallibleTArray<E>>::Read() deserializes to a fallible array, then converts that to an infallible array. If the deserialization of the fallible array has an allocation failure, then the failure is treated as a deserialization failure, and we crash in IPC code. We should instead treat this as an OOM crash, to make it clear what is going wrong.

This may be causing the mysterious deserialization failures in bug 1259480, given there is other evidence that the preferences can become extremely large (bug 1268662, though I'd guess that is more the result of data than number of prefs, but who knows).

Comment 1

[Nathan Froyd [:froydnj]]

So just switch deserialization to use infallible arrays always?

Comment 2

[Andrew McCreight [:mccr8]]

Oddly, we seem to IPDL codegen the serialization of nsTArray here. See PContentChild::Read(nsTArray<PrefSetting>* v__, [...]). So this won't actually use ParamTraits<InfallibleTArray<E>>::Read(). (Spot checking, this does not seem to apply to things like uint8_t[], which is fortunate.) I guess as a quick fix I'll look into making the codegen use infallible allocation. Longer term, I should figure out why we codegen this, and if it can be eliminated.

Comment 3

[Nathan Froyd [:froydnj]]

We codegen this as a hack, because we don't actually codegen ParamTraits implementations for all the things we need to serialize.  What we do instead is define overloaded Read()/Write() implementations in each of the classes that need to serialize something, and then a catch-all templated Read()/Write() implementation that forwards to our normal ParamTraits specializations.

Unwinding this would require some thought and likely some rethinking of how the IPDL compiler itself works.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Swap fallible and infallible TArray ParamTraits.

This should not change the behavior, because fallible_t is passed to the array methods.

Rename from InfallibleTArray to nsTArray, because that is more usual.

Remove unnecessary spaces from some template definitions.

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

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Make ParamTraits<nsTArray<E>>::Read use infallible allocation.

This will turn allocation failures during deserialization from IPC FatalError crashes into OOM crashes.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Use infallible array allocation in implementSpecialArrayPickling.

As with part 2, this will turn some allocation failures during deserialization from IPC FatalError crashes into OOM crashes.

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8747900 [details] [diff] [review]
part 2 - Make ParamTraits<nsTArray<E>>::Read use infallible allocation.

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

r=me with the comment described below.

::: ipc/glue/IPCMessageUtils.h
@@ +491,5 @@
>        if (!aMsg->ReadBytes(aIter, &outdata, pickledLength)) {
>          return false;
>        }
>  
> +      E* elements = aResult->AppendElements(length);

I think we should have comments somewhere in or above this method that describe why we want to use infallible allocations here, rather than failing to deserialize and try to handle that failure somewhere else.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f4ce8ef6a98
https://hg.mozilla.org/integration/mozilla-inbound/rev/303298849fd0
https://hg.mozilla.org/integration/mozilla-inbound/rev/b56b836c7e2f

Comment 9

[Andrew McCreight [:mccr8]]

(In reply to Nathan Froyd [:froydnj] from comment #7)
> I think we should have comments somewhere in or above this method that
> describe why we want to use infallible allocations here, rather than failing
> to deserialize and try to handle that failure somewhere else.

Good point. I added a comment to the Read() method.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

FWIW the intention here was to make sure that a rogue child process can't cause the parent to crash by sending a huge message. I guess the error handling in the parent process wasn't sufficient on its own, but shouldn't that just be improved rather than turning this into an OOM abort?

Comment 11

[Andrew McCreight [:mccr8]]

(In reply to Ben Turner (not reading bugmail, use the needinfo flag!) from comment #10)
> FWIW the intention here was to make sure that a rogue child process can't
> cause the parent to crash by sending a huge message. I guess the error
> handling in the parent process wasn't sufficient on its own, but shouldn't
> that just be improved rather than turning this into an OOM abort?

My patch is only changing how we crash, not if we crash, because a FatalError() in the parent process will already kill the parent process. That was changed because the prior behavior wasn't getting proper crash reports, or something like that. But yeah, in the long run we want to be able to back this patch out.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Cool, makes sense!

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1f4ce8ef6a98
https://hg.mozilla.org/mozilla-central/rev/303298849fd0
https://hg.mozilla.org/mozilla-central/rev/b56b836c7e2f

Comment 14

[Jim Mathies [:jimm]]

This looks like something we might want to uplift to 47 and 48?

Comment 15

[Andrew McCreight [:mccr8]]

Comment on attachment 8747899 [details] [diff] [review]
part 1 - Swap fallible and infallible TArray ParamTraits.

I was waiting to see if it happens on Nightly, but it doesn't really seem to, so I guess we should uplift it in case it helps.

Approval Request Comment
[Feature/regressing bug #]: e10s
[User impact if declined]: This will make certain kinds of e10s OOM crash reports easier to understand.
[Describe test coverage new/current, TreeHerder]: this code runs frequently
[Risks and why]: low. It should mostly turn one kind of crash into another.
[String/UUID change made/needed]: none

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8747899 [details] [diff] [review]
part 1 - Swap fallible and infallible TArray ParamTraits.

Improvements to better diagnose e10s OOM crashes, Aurora48+, Beta47+

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3aa4f99aeb0e
https://hg.mozilla.org/releases/mozilla-aurora/rev/6983c68ea3d0
https://hg.mozilla.org/releases/mozilla-aurora/rev/4697aa21b170

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/93d727dc3fb4
https://hg.mozilla.org/releases/mozilla-beta/rev/53a8e50cdaf8
https://hg.mozilla.org/releases/mozilla-beta/rev/13f145c74a33