1046341 - [B2G][Camera][Gecko] Camera initialization failure does not release ownership

Status: RESOLVED FIXED

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

Description

[Andrew Osmond [:aosmond] (he/him)]

If the camera initialization fails, we do not release the camera ownership, hence subsequent attempts will all fail due to their already being an owner for the camera.

Comment 1

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

[Blocking Requested - why for this release]: this bug was introduced way back in bug 909542. A weakness in the failure-case tests allowed it to go undetected. Failure to fix could result in the Camera app not being able to access the camera hardware (though, practically, killing the Camera and reopening it would fix the issue).

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

Attachment: bug1046341.patch, v1

Comment 3

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

Comment on attachment 8465060 [details] [diff] [review]
bug1046341.patch, v1

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

r+ with comments addressed. Thanks, Andrew!

::: dom/camera/test/test_camera_hardware_init_failure.html
@@ +29,5 @@
>  
> +var tests = [
> +  {
> +    name: "init-failure",
> +    key: "init-failure", 

nit: extra whitespace and end of line!

@@ +44,5 @@
> +      info("Running test: init-failure");
> +      navigator.mozCameras.getCamera(whichCamera, initialConfig, onSuccess, onError);
> +    }
> +  },
> +  {

Please add a comment here that explicitly states the 'init-success' test MUST follow the 'init-failure' one.

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

Attachment: bug1046341.patch, 1.1

Fix nits.

try: https://tbpl.mozilla.org/?tree=Try&rev=6c179696751e

Comment 5

[Hema Koka [:hema]]

Blocking because of impact mentioned in comment 1

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/4056345284df

Comment 7

[Carsten Book [:Tomcat]]

sorry had to backout this change for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=44957206&tree=B2g-Inbound

Comment 8

[Andrew Osmond [:aosmond] (he/him)]

Attachment: bug1046341.patch, v1.2

try: https://tbpl.mozilla.org/?tree=Try&rev=b73e4df464ae

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/bc8e33fd7a39

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/db09e41be6b3

Note that per the recent rule change announced in dev-gaia, 1.4 blockers don't have auto-approval to land on v2.0. Please nominate this patch for b2g32 uplift if you feel that it needs to land there as well. Sorry for the inconvenience :(

Comment 12

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

Comment on attachment 8465601 [details] [diff] [review]
bug1046341.patch, v1.2

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): improper failure case handling.
User impact if declined: on initialization error, camera hardware may remain open in an undefined state; to be cleared may require, at best, closing the app--at worst, rebooting the phone.
Testing completed: automated test (included in patch) was created to catch this error, then code was updated to fix the failing automated test; patch passes on tbpl.
Risk to taking this patch (and alternatives if risky): trivial to non-existant.
String or UUID changes made by this patch: none.

Comment 13

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

P.S. RyanVM, I pre-emptively checked that the patch applies cleanly to 2.0, and it does, so you should be good to go if/when a=2.0+ arrives.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Backed out from b2g30 for mochitest crashes.
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/605923bdb9c5

https://tbpl.mozilla.org/php/getParsedLog.php?id=45094180&tree=Mozilla-B2g30-v1.4

Comment 15

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

Hmm, I just ran the failing test 1000x locally (by tweaking the test script) and it entirely passes.

Comment 16

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

Attachment: bug1046341.patch, v1.3 [b2g30_v1_4]

Patch for b2g30_v1_4, which is missing a small fix to handle null values from Gonk that was added to 2.0+ in bug 981047.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/89a8a24a8b1b

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/ab92665417c8