1015979 - Assert that the images (video frames) created have a non-zero size

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Solenne Bienner]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140428193838

Comment 1

[Eric Phan]

Hello, concerning the bug n°6 on https://etherpad.mozilla.org/media-decoder-refactoring-ideas-2013.
I don't really get which size we need to assert is non-zero because in the first Create method (http://dxr.mozilla.org/mozilla-central/source/content/media/MediaData.cpp?from=#232) there is already a verification for the image size.

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

That NS_WARNING should be a MOZ_ASSERT.

Comment 3

[Eric Phan]

(In reply to Chris Pearce (:cpearce) from comment #2)
> That NS_WARNING should be a MOZ_ASSERT.

Ok thank you Chris,
One more question : is there a need to replace the NS_WARNING on line 238 (http://dxr.mozilla.org/mozilla-central/source/content/media/MediaData.cpp?from=#238) and 251 ?
Because it doesn't concern non-zero size, but replacing NS_WARNING by MOZ_ASSERT there seems to be better.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

I'd rather not convert the warnings on lines 238 and 251. The idea here is to guard against programmer error. Several times I and others have forgotten to pass in a non zero size here, and lost time debugging that. I want to prevent that in future.

Whereas those other cases guard against hostile input (passing INT32_MAX for the width and height of a frame, in order to trigger malloc failure and a crash). Arguably input could have 0 sized frame anyway and we'd hit this fatal assert in debug builds, but I think that's a good tradeoff to make here.

Comment 5

[Rose Badoux]

Attachment: bug-1015979-fix

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8432472 [details] [diff] [review]
bug-1015979-fix

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

You still need to return nullptr in both those cases, otherwise we'll crash or worse in build where assertions are disabled, i.e. Release Firefox builds!

The purpose of this bug is to make size-0 frames mistakes more noticeable.

::: content/media/MediaData.cpp
@@ +381,5 @@
>      return v.forget();
>    }
>  
>    // The following situations could be triggered by invalid input
> +  MOZ_ASSERT(aPicture.width <= 0 || aPicture.height <= 0, "Empty picture rect");

This doesn't need to be an assert.

Comment 7

[Eric Phan]

Attachment: bug-1015979-fix

Comment 8

[Eric Phan]

Comment on attachment 8433294 [details] [diff] [review]
bug-1015979-fix

Concerning your last comment, why is there no need for a MOZ_ASSERT there http://dxr.mozilla.org/mozilla-central/source/content/media/MediaData.cpp#389

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8433294 [details] [diff] [review]
bug-1015979-fix

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

Bravo!

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Eric Phan from comment #8)
> Comment on attachment 8433294 [details] [diff] [review]
> bug-1015979-fix
> 
> Concerning your last comment, why is there no need for a MOZ_ASSERT there
> http://dxr.mozilla.org/mozilla-central/source/content/media/MediaData.cpp#389

Because what I want is a way to prevent simple developer mistakes, that is, of forgetting to initialize the width and height so that they're non-0. Whereas negative frame sizes is a mistake I've never made.

Comment 11

[Eric Phan]

Attachment: bug-1015979-fix

Modification of commit message

Comment 12

[Paul Adenot (:padenot)]

Backed out and relanded, because I screwed up the bug number in the commit message.

Eric, congratulation on your first patch, onto the next one !

https://hg.mozilla.org/integration/mozilla-inbound/rev/5086f344b647
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9ce0328d40d
https://hg.mozilla.org/integration/mozilla-inbound/rev/cef880caa887

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5086f344b647