974855 - Camera app crashes when misdetecting requested picture size

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Camera, defect)

Description

[:gerard-majax]

Reproducing on 1.4 with Nexus S, Peak and Inari devices.

STR:
 0. Compose a new SMS
 1. Tap the attachment icon in Messages app, select 'Photo' to take a picture
 2. Camera app gets launched, part of the UI loads (buttons) no preview visible
 3.a. On Nexus S and Peak, camera app crashes at this time
 3.b. On Inari, camera app is frozen

Expected:
 Picture taken

Actual:
 Crash or freeze

Please note that:
 - taking a picture in the camera app works very well
 - using the pick activity from Twitter app (for example) also works well

I noticed some spurious message on Nexus S when reproducing the issue:
E/GeckoConsole( 1618): [JavaScript Error: "TypeError: pictureSize is undefined" {file: "app://camera.gaiamobile.org/js/main.js" line: 9759}]

I more or less date this bug from roughly one week, previously it was working fine.

Comment 1

[Julien Wajsberg [:julienw]]

I don't see the same log on my Peak, but I do see the crash.

Comment 2

[Jason Smith [:jsmith]]

Can we check to see if this reproduces on Buri? If so, can we get a crash report URL?

Comment 3

[Mike Habicher [:mikeh] (high bugzilla latency)]

I can't reproduce this on either inari or hamachi using the latest nightly build.

Comment 4

[:gerard-majax]

(In reply to Mike Habicher [:mikeh] from comment #3)
> I can't reproduce this on either inari or hamachi using the latest nightly
> build.

I do confirm that after updating my Inari this afternoon, I don't have the freeze anymore.

Comment 5

[Matthew Vaughan]

(In reply to Jason Smith [:jsmith] from comment #2)
> Can we check to see if this reproduces on Buri? If so, can we get a crash
> report URL?

This issue does not reproduce for me on my Buri device using the 02/20/14 1.4 build.

Device: Buri v1.4 MOZ RIL
BuildID: 20140220040203
Gaia: 6e71ab4da1b08586ea0c758edb7aa199ee34cd2f
Gecko: 660b62608951
Version: 30.0a1
Firmware Version: V1.2-device.cfg

Comment 6

[:gerard-majax]

Still crashing on Nexus S.

Comment 7

[:gerard-majax]

Partial backtrace:
Program received signal SIGSEGV, Segmentation fault.
0x417ac1ca in mozalloc_abort (msg=<value optimized out>) at /home/alex/codaz/Mozilla/b2g/devices/NexusS/B2G/gecko/memory/mozalloc/mozalloc_abort.cpp:30
30	    MOZ_CRASH();
(gdb) bt
#0  0x417ac1ca in mozalloc_abort (msg=<value optimized out>) at /home/alex/codaz/Mozilla/b2g/devices/NexusS/B2G/gecko/memory/mozalloc/mozalloc_abort.cpp:30
#1  0x417ac1e0 in abort () at /home/alex/codaz/Mozilla/b2g/devices/NexusS/B2G/gecko/memory/mozalloc/mozalloc_abort.cpp:39
#2  0x417ac1e0 in abort () at /home/alex/codaz/Mozilla/b2g/devices/NexusS/B2G/gecko/memory/mozalloc/mozalloc_abort.cpp:39
Backtrace stopped: previous frame identical to this frame (corrupt stack?)

Comment 8

[:gerard-majax]

I did some printf debug. Turns out that when it crashes, we are setting some picture size to undefined.

Taking a picture from the camera app:
E/GeckoConsole( 5421): Content JS DEBUG at app://camera.gaiamobile.org/js/main.js:9753 in CameraController.prototype.onSettingsConfigured: pictureSizes=={"height":1920,"width":2560}
E/GeckoConsole( 5421): Content JS DEBUG at app://camera.gaiamobile.org/js/main.js:9761 in CameraController.prototype.onSettingsConfigured: pictureSize=={"key":"pictureSizes","_cbs":{"change:selected":[null,null]},"_data":{"title":"Camera Resolution","icon":"icon-picture-size","options":[{"key":"5mp","title":"5MP","index":0,"value":{"height":1920,"width":2560}},{"key":"3mp","title":"3MP","index":1,"value":{"height":1536,"width":2048}},{"key":"1mp","title":"1MP","index":2,"value":{"height":960,"width":1280}},{"key":"vga","title":"VGA","index":3,"value":{"height":480,"width":640}}],"persistent":true,"menu":4,"key":"pictureSizes","optionsHashAll":{"5mp":{"key":"5mp","title":"5MP","index":0,"value":{"height":1920,"width":2560}},"3mp":{"key":"3mp","title":"3MP","index":1,"value":{"height":1536,"width":2048}},"1mp":{"key":"1mp","title":"1MP","index":2,"value":{"height":960,"width":1280}},"vga":{"key":"vga","title":"VGA","index":3,"value":{"height":480,"width":640}},"qvga":{"key":"qvga","title":"QVGA","index":4,"value":"qvga"}},"optionsHa
E/GeckoConsole( 5421): Content JS DEBUG at app://camera.gaiamobile.org/js/main.js:9762 in CameraController.prototype.onSettingsConfigured: pictureSize.value=={"height":1920,"width":2560}

Taking a picture from the pick activity triggered from sms app:
E/GeckoConsole( 5091): Content JS DEBUG at app://camera.gaiamobile.org/js/main.js:9752 in CameraController.prototype.onSettingsConfigured: pictureSizes==undefined
E/GeckoConsole( 5091): Content JS DEBUG at app://camera.gaiamobile.org/js/main.js:9760 in CameraController.prototype.onSettingsConfigured: pictureSize=={"key":"pictureSizes","_cbs":{"change:selected":[null,null]},"_data":{"title":"Camera Resolution","icon":"icon-picture-size","options":[],"persistent":true,"menu":4,"key":"pictureSizes","optionsHashAll":{"5mp":{"key":"5mp","title":"5MP","index":0,"value":{"height":1920,"width":2560}},"3mp":{"key":"3mp","title":"3MP","index":1,"value":{"height":1536,"width":2048}},"1mp":{"key":"1mp","title":"1MP","index":2,"value":{"height":960,"width":1280}},"vga":{"key":"vga","title":"VGA","index":3,"value":{"height":480,"width":640}},"qvga":{"key":"qvga","title":"QVGA","index":4,"value":"qvga"}},"optionsHash":{},"selected":"vga"}}
E/GeckoConsole( 5091): Content JS DEBUG at app://camera.gaiamobile.org/js/main.js:9761 in CameraController.prototype.onSettingsConfigured: pictureSize.value==undefined

Comment 9

[:gerard-majax]

Okay, so I have the correct explanation now.

When the SMS app triggers the pick activity, it sets a maxFileSizeBytes attribute to be in the limits of MMS. This value is 307200 bytes. Which is VGA resolution (640x480).

The code flows up to the Activity controller in apps/camera/js/controllers/activity.js, and more specifically, in the configurePictureSize() method, at https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/controllers/activity.js#L46

In this method, we detect if the pick activity has any size limitation, and in this case we select the biggest picture size that matches the size requirement. This is getPictureSizesSmallerThan https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/controllers/activity.js#L75 which iterates over a set of objects that are retrieved from https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/config/app.js. Specifically, this defines the set of resolutions we support. Filtering is done by comparating size.width*size.height with the expected file size.

Comparison is in the strict sense. But 640x480 == 307200. Hence, coming out of this helper method, we have NOTHING. This explains the TypeError.

Inari does not reproduce the issue because it has a different set of pictures sizes to select from: [{"key":"1mp","title":"1MP","index":2,"value":{"height":768,"width":1024}},{"key":"vga","title":"VGA","index":3,"value":{"height":480,"width":640}},{"key":"qvga","title":"QVGA","index":4,"value":{"height":240,"width":320}}]

While the Nexus S has: [{"key":"5mp","title":"5MP","index":0,"value":{"height":1920,"width":2560}},{"key":"3mp","title":"3MP","index":1,"value":{"height":1536,"width":2048}},{"key":"1mp","title":"1MP","index":2,"value":{"height":960,"width":1280}},{"key":"vga","title":"VGA","index":3,"value":{"height":480,"width":640}}]

And the Peak has: [{"key":"5mp","title":"5MP","index":0,"value":{"height":1944,"width":2592}},{"key":"3mp","title":"3MP","index":1,"value":{"height":1536,"width":2048}},{"key":"1mp","title":"1MP","index":2,"value":{"height":960,"width":1280}},{"key":"vga","title":"VGA","index":3,"value":{"height":480,"width":640}}]

The strict comparison makes the Inari selecting 320x240, but nothing on the Nexus S nor the Peak. A quick and easy fix would be to make the comparison not strict, that does work well on all the tested devices. But probably that the code definitively needs to be made more robust against this lack of picture size.

Comment 10

[:gerard-majax]

Attachment: Link to Github https://github.com/mozilla-b2g/gaia/pull/16495

Please find attached a link to the github pull request. This patch should probably be followed by a follow up bug: I really think there is need for more guard against setting invalid values.

Comment 11

[Diego Marcos [:dmarcos]]

Sending the review to Wilson. This part is his work. He's in a better position than me to decide to take this patch or implement something more robust.

Comment 12

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

related to bug 974455?

Comment 13

[:gerard-majax]

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #12)
> related to bug 974455?

That might, but I can't test now.

Comment 14

[David Flanagan [:djf]]

Wilson merged this to master: https://github.com/mozilla-b2g/gaia/commit/0fe7d2b94d9e26ea3cd90d222e2131fe1bae2e3d