1345179 - Crash in OOM | large | mozalloc_abort | mozalloc_handle_oom | moz_xmalloc | mozilla::MakeUnique<T>

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[alex_mayorga]

This bug was filed from the Socorro interface and is 
report bp-71a3295a-4127-4d9f-a5df-c721d2170306.
=============================================================

¡Hola!

Crashed like above, found 104 other crashes at https://crash-stats.mozilla.com/signature/?product=Firefox&signature=OOM%20%7C%20large%20%7C%20mozalloc_abort%20%7C%20mozalloc_handle_oom%20%7C%20moz_xmalloc%20%7C%20mozilla%3A%3AMakeUnique%3CT%3E#summary

Unfortunately I have no steps that lead to the crash.

¡Gracias!
Alex

Comment 1

[Mats Palmgren (inactive)]

Your crash seems to be associated with allocating data for a <video> element:
https://hg.mozilla.org/mozilla-central/annotate/966464a68a2c/dom/media/platforms/agnostic/BlankDecoderModule.cpp#l136
(changing the Component based on that)

I'm guessing "sizeY + sizeCbCr" is controlled by content, so this allocation
should probably be fallible and return an error when it fails (if possible).

Comment 2

[Mats Palmgren (inactive)]

(There are other root causes associated with this signature though, so we should add
"mozilla::MakeUnique<T>" to the ignore list to get the next stack frame added to
the signature so we can differentiate between them.  I filed bug 1345196 on that.)

Comment 3

[Mats Palmgren (inactive)]

BTW, is there a security issue here? (assuming that mFrameWidth and mFrameHeight are
controlled by content, they can be chosen to overflow uint32_t when multiplied)
Also, why are we using a natural (signed) 'int' for the result?
That just seems wrong in addition to not follow our coding style rules.

Comment 4

[Tzuhao Kuo [:kaku]]

Bug 1309492 is also modifying this part of code. So, I will handle this bug after Bug 1309492 is landed (should be soon and keep myself be NI-ed.)

Bug 1309492 is going to remove the temporary `const int sizeY and const int sizeCbCr`, so the security issue will no longer exist.

Also, I think we could replace the MakeUnique() with MakeUniqueFallible() which should solve the root cause of this bug.

Comment 5

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1345179 - use MakeUniqueFallible() in BlankVideoDataCreator;

If OOM happends, just return null and the DummyMediaDataDecoder will reject the DecodePromise with NS_ERROR_OUT_OF_MEMORY.

Review commit: https://reviewboard.mozilla.org/r/120836/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120836/

Comment 6

[Tzuhao Kuo [:kaku]]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e1d00531f7918eadac02caac9b339626a490bbd
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8ce9e1299ec35b1c706e5476053cfb023448525

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8847923 [details]
Bug 1345179 - use MakeUniqueFallible() in BlankVideoDataCreator;

https://reviewboard.mozilla.org/r/120836/#review122822

Comment 8

[Tzuhao Kuo [:kaku]]

Thanks for the review!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0d4976ed4a31
use MakeUniqueFallible() in BlankVideoDataCreator; r=jwwang

Comment 10

[Mats Palmgren (inactive)]

I'm concerned about "mFrameWidth * mFrameHeight" overflowing (they are uint32_t).
I think you need to use CheckedInt<> for this code to be safe, unless mFrameWidth and
mFrameHeight are always clamped when they are assigned so that they cannot possibly
overflow when multiplied.  Is that the case?

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0d4976ed4a31

Comment 12

[Jean-Yves Avenard [:jya]]

How can we get crash report of a code that isn't enabled to start with?

Comment 13

[Tzuhao Kuo [:kaku]]

(In reply to Jean-Yves Avenard [:jya] from comment #12)
> How can we get crash report of a code that isn't enabled to start with?
Not sure what you mean. The BlankDecoderModule WAS used as the dummy decoder module for the suspend-video-decoder feature and it is enabled in nightly. (We have switched to NullDecoderModule for the suspend-video-decoder now.)

Comment 14

[Tzuhao Kuo [:kaku]]

(In reply to Mats Palmgren (:mats) from comment #10)
> I'm concerned about "mFrameWidth * mFrameHeight" overflowing (they are
> uint32_t).
> I think you need to use CheckedInt<> for this code to be safe, unless
> mFrameWidth and
> mFrameHeight are always clamped when they are assigned so that they cannot
> possibly
> overflow when multiplied.  Is that the case?

Fair enough, open bug 1348237 for this issue.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Looks like this might still be happening?
https://crash-stats.mozilla.com/report/index/944cefe5-1caa-435c-b286-0089a2170319

Comment 16

[Mats Palmgren (inactive)]

Ryan, please file a new bug report since that's a different call site.
(we need a fix for bug 1345196 to differentiate these)

Comment 17

[Ryan VanderMeulen [:RyanVM]]

In that case, please request Aurora/Beta/ESR52 approval on this when you get a chance :)

Comment 18

[Tzuhao Kuo [:kaku]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #17)
> In that case, please request Aurora/Beta/ESR52 approval on this when you get
> a chance :)

Sorry for my late reply. Actually, all the client coded that might trigger this issue are not enabled in Aurora/Beta/Release channels, so I think that we don't need to uplift all of this because, make sense?

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Sounds good to me, thanks for the update.