Closed Bug 992341 Opened 7 years ago Closed 7 years ago

[B2G][Camera] Double tapping in camera causes self-timer to countdown in negative numbers

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:1.4+, b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
1.4 S5 (11apr)
blocking-b2g 1.4+
Tracking Status
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: bzumwalt, Assigned: wilsonpage)

References

()

Details

(Whiteboard: [1.4-camera-exploratory])

Attachments

(2 files)

Attached file Logcat
Description:
With self-timer set to 3, 5, or 10 seconds in camera app, pressing button to take picture, then double-tapping button when countdown reaches zero causes numbers to continue counting into negative numbers.

This negative countdown will remain on screen until app is closed. If user tries to take another picture the countdown does not restart and the picture will be taken immediately without a delay.

Button must be pressed quickly just as the number 1 is disappearing. Spamming the button works better for reproducing, but only a double tap is needed.

Repro Steps:
1) Updated Buri to BuildID: 20140404000202
2) Launch camera app
3) Open options menu and set self timer to 3 seconds
4) Return to main camera screen and press take picture button
5) As soon as the numer one disappears from the countdown double tap the take picture button

Actual:
Countdown continues into negative numbers.

Expected:
New countdown starts for second picture.

Environmental Variables:
Device: Buri v1.4 Mozilla RIL
BuildID: 20140404000202
Gaia: b4f3b84ec68233a99fd5865c15cfe28aebe26531
Gecko: 3186bbc50050
Version: 30.0a2
Firmware Version: v1.2-device.cfg

Notes: 
Repro frequency: 3/3, 100%
See attached: Youtube video clip & logcat
Youtube link: http://youtu.be/wEk445hRDOU
Whiteboard: [1.4-camera-exploratory]
blocking-b2g: 1.4? → ---
Well that's a silly bug.
blocking-b2g: --- → 1.4?
Please assign to ashish who worked on self-timer feature.
Flags: needinfo?(jjoons79)
Dear Ashish! 

Please take this bug.
Assignee: nobody → singhashish1887
Flags: needinfo?(jjoons79)
broken feature - blocking
blocking-b2g: 1.4? → 1.4+
Attempted 50+ times. Unable to reproduce on latest master.
Flags: needinfo?(bzumwalt)
(In reply to Wilson Page [:wilsonpage] from comment #5)
> Attempted 50+ times. Unable to reproduce on latest master.

Wilson, did you try this on 1.4 using Buri
Flags: needinfo?(wilsonpage)
Issue DOES occur for me on latest 1.4; Issue does NOT occur for me in latest Master M-C

Environmental Variables:
Device: Buri v1.4 Mozilla RIL
BuildID: 20140408000202
Gaia: 26983f356ecb1bcf30e862d334b5de790071803e
Gecko: 70b076fc7558
Version: 30.0a2
Firmware Version: v1.2-device.cfg

Device: Buri Master M-C Mozilla RIL
BuildID: 20140408040204
Gaia: 1958454595b1fa0e061f0652ae965629993f5708
Gecko: 8883360b1edb
Version: 31.0a1
Firmware Version: v1.2-device.cfg
Flags: needinfo?(bzumwalt)
Brogan seems to have clarified. But for the record I could not repro on v1.4 Gaia with latest Gecko either.
Flags: needinfo?(wilsonpage)
Ashish, if you are unable to get to reproduce this on nexus4 with 1.4 branch codebase, please set the assignee back to nobody so someone here who has a buri can take it up

Thanks
Hema
We are not able to reproduce this issue on Nexus-4 with V1.4 Gaia.
As per the Comment#9 & Comment#10. I am changing assigned to Nobody.
Assignee: singhashish1887 → administration
Assignee: administration → nobody
Marcia - Can you check this?
Flags: needinfo?(mozillamarcia.knous)
I tried that too on today's 1.4 - it's extremely hard to reproduce, still possible though. I got it 1/30 times. Should double tap the capture button right after the number one disappears from the countdown, not too early, not too late...

Buri 1.4
BuildID: 20140409000202
Gaia: 26983f356ecb1bcf30e862d334b5de790071803e
Gecko: e450e07e3a58
Version: 30.0a2
I can't reproduce this either, and I can't figure out why it is happening, but it seems clear that it is a race condition, and the video is enough to go looking through the code to try to figure out what could be going on.

I found two things of interest in controllers/timer.js:

First, this line in the tick() function sticks out to me:

  if (!(--this.seconds)) {

This if clause is being triggered only if seconds is exactly equal to zero.  If this.seconds somehow becomes negative, this code for cancelling the timer does not run and the negative number gets displayed.
So changing this to `if (--this.seconds <= 0)` would prevent the display of negative numbers here.

Second, this line in the start() method also sticks out:

  this.interval = setInterval(this.tick, 1000);

The code does not check whether there is an existing interval defined and clear it first, it just overwrites any existing interval. So if somehow a fast double tap causes start() to be called a second time while a countdown is already in progress, the new interval will overwrite the old one. And when the original timer reaches 0 it will call clear(), but that will clear the new interval, leaving the old one running, and it will continue on down to negative numbers and keep displaying them.

I can't figure out how that could happen, though. I also don't understand what is preventing clicks on the controls from being handled while the timer is counting down, so I think I'm missing something. It seems like the timerActive flag ought to prevent multiple calls to start() until after the existing interval has already been cleared. (I'm assuming that this this.app.emit('startcountdown') call in controllers/camera.js is synchronous.  If not, then I suppose a quick double click could start two timers.)  (I've heard justin and russ talking about weird event dispatching bugs in recent versions of gecko. If 1.4 is effected, could we somehow be getting the tap event that stops the countdown being dispatched further and also triggering the capture button again?)

Even if we can't figure out the race, we can still take steps to prevent it.  Modifying tick() to not display negative numbers might be good, and modifying start() to clear any existing interval would also be good. Even better, though, might be switching from setInterval() to setTimeout() so we can't get into this kind of runaway situation would also be good.

Wilson: does this give you any ideas of why this might be happening? If you agree that it is worth hardening those two lines to try to prevent that, do you want to take this and prepare a patch so we can ask Brogan whether it fixes it for him?
Flags: needinfo?(wilsonpage)
Attached file pull-request (master)
Implemented improved robustness as per suggestions.
Attachment #8404626 - Flags: review?(dflanagan)
Flags: needinfo?(wilsonpage)
Brogan,

If you apply Wilson's patch, are you still able to reproduce the bug?
Flags: needinfo?(bzumwalt)
Assignee: nobody → wilsonpage
Comment on attachment 8404626 [details] [review]
pull-request (master)

r+, but please add a clearTimeout() in scheduleTick() just to be absolutely sure that you're not overwriting an existing timer that is still pending.

Since we don't know how the race condition is being triggered (unless you figured it out?), I want to cover every possible weak point that we can see.
Attachment #8404626 - Flags: review?(dflanagan) → review+
Landed on 'master': https://github.com/mozilla-b2g/gaia/commit/b367ecf682c5cfbe7f8b425cbfb6a31ef35b8a5f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
(In reply to Natalya Kot [:nkot] from comment #13)
> I tried that too on today's 1.4 - it's extremely hard to reproduce, still
> possible though. I got it 1/30 times. Should double tap the capture button
> right after the number one disappears from the countdown, not too early, not
> too late...
> 
> Buri 1.4
> BuildID: 20140409000202
> Gaia: 26983f356ecb1bcf30e862d334b5de790071803e
> Gecko: e450e07e3a58
> Version: 30.0a2

I forgot to update this bug on Thursday - I did try as well on 1.4 and like nkot I had quite a bit of difficulty even trying to reproduce it. Now that is has on Master and 1.4 I will test again.
Flags: needinfo?(mozillamarcia.knous)
Removing my old needinfo as Wilson's patch landed before I got a chance to test.
Flags: needinfo?(bzumwalt)
You need to log in before you can comment on or make changes to this bug.