975481 - [B2G][Camera]Repeatedly switching between stop and record after recording a video causes the Camera app to become unresponsive

Status: VERIFIED FIXED

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

Description

[Andrew Stole [:astole]]

After pressing record, repeatedly pressing the stop and record button causes the Camera app to become unresponsive. On some occasions, the app is responsive, but every time the user presses record, the error message seen in the video that says "Video not recorded. An error prevented Camera from recording the video" appears. 

Repro Steps:
1) Updated Buri to BuildID: 20140221040202
2) Open camera app and switch to video mode
3) Press record
4) After a video has started recording, repeatedly press the stop button and record button multiple times

Actual:
Camera app becomes unresponsive.

Expected:
The camera app is able to repeatedly stop and record videos without becoming unresponsive.

Environmental Variables:
Device: Buri v1.4 M/C Mozilla RIL
BuildID: 20140221040202
Gaia: 35365feace970bfc51276428f40a477c9c86b7bb
Gecko: 7010ab83a06e
Version: 30.0a1
Firmware Version: V1.2-device.cfg

Repro frequency: 100%
See attached: Video

Comment 1

[Jason Smith [:jsmith]]

This is likely going to fail in QC's stabilization tests.

Two things:

1. Can we try to get more realistic end-user STR here?
2. Does this reproduce on 1.3?

Comment 2

[Chris Kreinbring [:CKreinbring]]

Unable to repro on Buri 1.3 mozilla RIL.  Tapping the start/stop recording button repeatedly does not cause the app to freeze up.  No error messages appear either.

Build ID: 20140221004002
Gecko: https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/e5f25becc0e7
Gaia: 8039a5cb7519adfa81677df577f494c6a4de6140
Platform Version: 28.0
Firmware Version: V1.2-device.cfg

Comment 3

[Jason Smith [:jsmith]]

Oh - I missed the part originally about the fact that the camera app was becoming unresponsive. In that case, don't worry about getting better STR here - this is going block a QC stability test easily, as I believe they have a test case that does this exact test case.

Comment 4

[John Hammink]

I'm not able to reproduce this on 1.4 either.   Andrew, can you try restarting the phone and see if the issue repros for you still on 1.4?

Comment 5

[John Hammink]

Ahh no, my bad.  Needed to go much faster on the button.  If I do 2-3 taps/second then I'm able to repro.

Comment 6

[Jason Smith [:jsmith]]

Not a smoketest blocker.

However, what's a bit scary about this bug is that when this bug occurs, recording continues in the background outside of the user's control. And I've definitely confirmed the lack of responsiveness.

Comment 7

[Hema Koka [:hema]]

Justin, Please take a look at this issue

Comment 8

[Justin D'Arcangelo [:justindarc]]

Attachment: GitHub Pull Request #16634

Comment 9

[David Flanagan [:djf]]

Comment on attachment 8381656 [details] [review]
GitHub Pull Request #16634

Justin,

I'm giving r- here because I don't think we can rely on setTimeout(). Unfortunately, I think the general problem is a lot harder than this.

More detailed comments on github.

Comment 10

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8381656 [details] [review]
GitHub Pull Request #16634

David: Please re-review. I addressed the setTimeouts and resolved the issue directly at the source of the problem:

`Camera.prototype.startRecording`

and

`Camera.prototype.stopRecording`

Since we can't have a video less than 1sec anyway, this prevents multiple calls to start/stop and multiple calls to process video metadata (which wouldn't exist if 1sec hadn't been recorded).

NOTE: I kept the BUTTON elements in this patch since I still think its a good idea to use BUTTON's instead of A's. See attached screenshots in GH pull req to compare before/after.

Comment 11

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

Would it be better to give stopRecording() a callback to let you know when it has actually happened?

Comment 12

[Justin D'Arcangelo [:justindarc]]

Mike: That would certainly be helpful. I believe the cause of this issue had to do with start/stop being called multiple times and the event queue was getting backed up causing all sorts of strange behavior. Having a callback here would increase stability. Does it make sense to have one on startRecording as well?

Comment 13

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

Justin, startRecording() has a callback! :)
http://dxr.mozilla.org/mozilla-central/source/dom/webidl/CameraControl.webidl#292

Comment 14

[Justin D'Arcangelo [:justindarc]]

Mike: ::facepalm:: - I'm not at my computer at the moment, but you're right and we already are using it. But, a stopRecording() callback would be helpful because we could definitively disable the button in the UI until it is called.

Comment 15

[Justin D'Arcangelo [:justindarc]]

Mike: Ok, back at my laptop now and we are definitely using the callback in startRecording(). In fact, part of the issue was that we were waiting for that callback before updating the 'state' of the UI to indicate we are recording. Because of this, it was possible to hammer on the capture button and startRecording() would be called multiple times. So, in my PR, I am now immediately setting the 'state' of the app to 'recording' as soon as the record button is pressed (preventing multiple calls to startRecording()). Then, if startRecording() calls the error callback, I update the app state again to not be 'recording'.

Comment 16

[David Flanagan [:djf]]

Comment on attachment 8381656 [details] [review]
GitHub Pull Request #16634

r- because there are too many things I don't understand and because I think that Diego's patch for bug 975453 might fix it.  And if it doesn't fix it, I think you should work t build upon his patch rather than take a new approach here.

Comment 17

[Chris Kreinbring [:CKreinbring]]

Bug first appeared on the 1/10 build and appears to be Gaia based.

No repro
Build ID: 20140109040203
Gecko: http://hg.mozilla.org/mozilla-central/rev/9409405e0739
Gaia: 47206ac66b084c6f6c4503a3b10d0e0760df2b6f
Platform Version: 29.0a1
Firmware Version: V1.2-device.cfg

Repro
Build ID: 20140110040206
Gecko: http://hg.mozilla.org/mozilla-central/rev/37516445a0b5
Gaia: f400efc804366c7b7cf5476d1d5d325e6651ee71
Platform Version: 29.0a1
Firmware Version: V1.2-device.cfg

Old Gaia and new Gecko: No repro
Gecko: http://hg.mozilla.org/mozilla-central/rev/37516445a0b5
Gaia: 47206ac66b084c6f6c4503a3b10d0e0760df2b6f

New Gaia and old Gecko: Repro
Gecko: http://hg.mozilla.org/mozilla-central/rev/9409405e0739
Gaia: f400efc804366c7b7cf5476d1d5d325e6651ee71

Push Log:  https://github.com/mozilla-b2g/gaia/compare/47206ac66b084c6f6c4503a3b10d0e0760df2b6f...f400efc804366c7b7cf5476d1d5d325e6651ee71

Comment 18

[Justin D'Arcangelo [:justindarc]]

Patch that landed for bug 975453 resolves this as well:

https://github.com/mozilla-b2g/gaia/commit/a2d48c1a41f78bb7896df32359a19011b713ac5f

There is still a bug related to APZ that I will be filing separately.

Comment 19

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

A simpler option for API feedback would be to just expose OnRecorderStateChange("Stopped") to the DOM/JS. It's currently swallowed[1] for no good reason.

1. http://dxr.mozilla.org/mozilla-central/source/dom/camera/DOMCameraControl.cpp#1063

Comment 20

[Justin D'Arcangelo [:justindarc]]

Mike: Actually, you're probably right because wouldn't that also cover the case where recording stops for a reason other than the user pressing 'stop'? Either way, it would be much more elegant than how we currently look for the file to be written.

Comment 22

[Jason Smith [:jsmith]]

Reopening per the dupe - comment 18 isn't right here. This is definitely still reproducing on trunk right now.

Comment 24

[Justin D'Arcangelo [:justindarc]]

Jason: Is this still reproducible on `camera-new-features`? I believe this has been resolved by the patch for Bug 984616.

Comment 25

[Jason Smith [:jsmith]]

I don't know. If we really want to find out, then let's retest after that branch merges into the mainline Gaia branches.

Comment 26

[Justin D'Arcangelo [:justindarc]]

The fix for this has landed with the merging of camera-new-features to master:

https://github.com/mozilla-b2g/gaia/commit/e328318d5e27a3253061fb1679ab9abc82e60fcc

Comment 27

[David Flanagan [:djf]]

Clearing the 1.4+ flag since this bug was resolved by other bugs. Nothing attached to this bug ever landed, so there is nothing needing uplift here.

Comment 28

[Jason Smith [:jsmith]]

That actually doesn't have any effect on the blocking decision. Bugs triaged here are triaged independent of how the issue was fixed - it just about whether the issue itself blocks the release or not.

Comment 29

[J Zimbrick]

This issue appears fixed in 1.4 and 1.5 on the latest Buri builds.

Following the steps provided does not result in the camera app becoming unresponsive.

Verifying as fixed.

1.4 Environmental Variables: 
Device: Buri
BuildID: 20140407000203
Gaia: 86de7fcce674ef6196d68e7e23552d219a3d72db
Gecko: 6e028297be14
Version: 30.0a2
Base Image: V1.2-device.cfg

1.5 Environmental Variables: 
Device: Buri
BuildID: 20140407040202
Gaia: f1a98bfaa3ab2480945bd7018831fd56c61cdc24
Gecko: 5405d6f4e3c6
Version: 31.0a1
Base Image: V1.2-device.cfg