1071451 - [Telemetry] App usageTime timer does not stop when taking incoming call

Status: VERIFIED FIXED

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

Description

[Shing Lyu [:shinglyu]]

# Summary
  When taking incoming call, the app usage time is still counting for the app opened previously. Also, the proximity sensor during call will interrupt with the timer.

# Steps
  * (optional) Change the app usage upload interval to 1 min.
  * Enable Wi-Fi or 3G, and enable app usage tracking in Settings > Improve B2G OS > Submit data
  * Open the clock app, and wait for 5 sec.
  * Call the phone under test from another phone.

  Case 1:
    * Don't answer the phone
    * Wait 10 sec, and end the call from the other phone.
    ## Expected
      The usageTime for clock app should be ~5 sec. 
    ## Actual
      The usageTime for the clock app is 5+10 ~= 15 sec.

  Case 2:
    * Answer the phone
    * Talk for 10 sec, then end the call.
    ## Expected
      The usageTime for clock app should be ~5 sec. 
    ## Actual
      The usageTime for the clock app is 5+10 ~= 15 sec.

  Case 3:
    * Answer the phone.
    * Talk for 10 sec, then cover the proximity sensor.
    * Wait for 10 sec, then uncover the proximity sensor.
    * Talk for 10 more sec, then end the call.
    ## Expected
      The usageTime for clock app should be ~5 sec. 
    ## Actual
      The usageTime for the clock app is 5+10(talk before proximity sensor lock)+10(talk after proximity sensor lock) ~= 25 sec.

# Reproduction Frequency
  Always

Comment 1

[Shing Lyu [:shinglyu]]

This bug is based on the discussion in Bug 982663 comment #57

Comment 2

[Marshall Culpepper [:marshall_law]]

[Blocking Requested - why for this release]:
We need to properly handle incoming calls

Comment 3

[Hema Koka [:hema]]

Blocking Reason: We need Correctly calculate the usage of the app. Incoming call time should probably be accounted in the dialer app and not part of the clock app as described in the bug.

Comment 4

[Gregor Wagner [:gwagner]]

Marshall, any updates here?

Comment 5

[Marshall Culpepper [:marshall_law]]

Attachment: attention windows - v1

We now properly handle attention windows (such as callscreen) by recording their manifestURLs the same way we do any other app, except they are kept in a stack because the 'appopened' event isn't fired when the last attention window is closed.

Added a unit test that tests 2 stacked attention windows on top of an app, and also did manual testing on this with actual phone calls and the proximity sensor, and the use cases listed here are now working properly..

Comment 6

[David Flanagan [:djf]]

Looks like this patch does not deal with the proximity sensor part of this bug. I think you can detect that in the screenchange event by checking e.detail.screenOffBy. If that property is "proximity", then just ignore the event, I think.  (Though you'll want to make sure that the property is set to "proximity" both when the screen goes on and when it goes off, because otherwise we'll mess things up.)

Comment 7

[Marshall Culpepper [:marshall_law]]

(In reply to David Flanagan [:djf] from comment #6)
> Looks like this patch does not deal with the proximity sensor part of this
> bug. I think you can detect that in the screenchange event by checking
> e.detail.screenOffBy. If that property is "proximity", then just ignore the
> event, I think.  (Though you'll want to make sure that the property is set
> to "proximity" both when the screen goes on and when it goes off, because
> otherwise we'll mess things up.)

It isn't clear to me if the proximity sensor should actually count for anything.. During a phone call, if the screen is disabled because of the proximity sensor, the phone call is still active, and I'd think we'd want to count that against the callscreen? This may just be a corner case that we can hardcode though.. do we know any other apps that turn off the screen with the proximity sensor?

Comment 8

[David Flanagan [:djf]]

Comment on attachment 8503406 [details] [review]
attention windows - v1

I'm giving r+ on this, but also requesting Alive's feedback because I honestly don't understand when the attentionopened and attentionclosed events are fired.

Various nits listed on github. Mostly code and comment clarification things.

I assume you have tested this carefully on 2.1 and on master?  And you've tried things like having an alarm go off while you're in the middle of a call?  (That's the only realistic scenario I can think of where there would be more than one attention screen).

Have you checked what happens when you have one or two attention screens up and press the home button?  Are all the attentionclosed events properly sent to clear out your stack?

When an attention screen is visible, can you swipe left or right to another app? Does the attentionclosed event get sent in that case?

Obviously what I'm worried about here is your stack getting out of sync with reality.  But if you've tested these cases, and if they work in 2.1 and on master, then I think this is ready to land.

Comment 9

[David Flanagan [:djf]]

r+ on the new verison of the patch, but see github before landing. I'd like you to fix the code that incorrectly pairs find() and pop(). Also, there's another test scenario (receive call, lock phone, terminate call, wake phone) on github to consider if you haven't already tested it.

Comment 10

[Marshall Culpepper [:marshall_law]]

master: https://github.com/mozilla-b2g/gaia/commit/aae86c910fbfb8f22e431479806cfb09c0ea9a1e

Comment 11

[Marshall Culpepper [:marshall_law]]

Attachment: v2.1 PR

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Bug 1033549
[User impact] if declined: Phone calls / alarms won't be properly recorded in App Usage Metrics, proximity sensor based screen changes won't count toward the currently running app.
[Testing completed]: Tracking phone call and alarm attention windows stacked on top of each other and on other apps, along with the proximity sensor triggered screen change.
[Risk to taking this patch] (and alternatives if risky): Unknown cases in attention window logic / stacking could cause incorrect counting in app metrics
[String changes made]: None

Comment 12

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8503406 [details] [review]
attention windows - v1

Use attentionopened/attentionclosed is something correct;
however I would very concern about why do we need to maintain a list of attention windows in this module. This makes it become another manager of window manager of windows and need to be consistent with any window management change.

Comment 13

[[:fabrice] Fabrice Desré]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #12)
> Comment on attachment 8503406 [details] [review]
> attention windows - v1
> 
> Use attentionopened/attentionclosed is something correct;
> however I would very concern about why do we need to maintain a list of
> attention windows in this module. This makes it become another manager of
> window manager of windows and need to be consistent with any window
> management change.

Alive, does that mean that you want a follow up or a backout of the current patch that landed on master?

Comment 14

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Fabrice Desré [:fabrice] from comment #13)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #12)
> > Comment on attachment 8503406 [details] [review]
> > attention windows - v1
> > 
> > Use attentionopened/attentionclosed is something correct;
> > however I would very concern about why do we need to maintain a list of
> > attention windows in this module. This makes it become another manager of
> > window manager of windows and need to be consistent with any window
> > management change.
> 
> Alive, does that mean that you want a follow up or a backout of the current
> patch that landed on master?

I don't think we need backout. I'm trying to state this is hard to maintain afterwards.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/5e4e19d032549e5eaa5bfc1bc3edda3e6fe51922

Comment 16

[Shing Lyu [:shinglyu]]

Verified.

Step: 
* Open the clock app.
* Make a call to the phone under test.
* Hangup the call

Result: (logcat snippet)
  E/GeckoConsole( 1568): Content JS LOG at app://system.gaiamobile.org/js/app_usage_metrics.js:120 in debug: [AppUsage] app://verticalhome.gaiamobile.org/manifest.webapp ran for 4
  E/GeckoConsole( 1568): Content JS LOG at app://system.gaiamobile.org/js/app_usage_metrics.js:120 in debug: [AppUsage] Saved app usage data
  E/GeckoConsole( 1568): Content JS LOG at app://system.gaiamobile.org/js/app_usage_metrics.js:120 in debug: [AppUsage] app://clock.gaiamobile.org/manifest.webapp ran for 16
  E/GeckoConsole( 1568): Content JS LOG at app://system.gaiamobile.org/js/app_usage_metrics.js:120 in debug: [AppUsage] Saved app usage data
  E/GeckoConsole( 1568): Content JS LOG at app://system.gaiamobile.org/js/app_usage_metrics.js:120 in debug: [AppUsage] app://callscreen.gaiamobile.org/manifest.webapp ran for 8
  E/GeckoConsole( 1568): Content JS LOG at app://system.gaiamobile.org/js/app_usage_metrics.js:120 in debug: [AppUsage] Saved app usage data

Version:
  Gaia      1ea74943cfe525c76a074ca1d7de8e51a70f6b98
  Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/7fa82c9acdf2
  BuildID   20141008000201
  Version   34.0a2
  ro.build.date   Mon Sep 29 19:42:41 EDT 2014
ro.bootloader   L1TC10011800
ro.build.version.incremental   eng.cltbld.20140929.194231