919149 - Recording icon doesn't disappear if app is killed from card view

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Killing app from card view will directly kill the content process, therefore, we don't have a timing to send the recording status notification back to chrome process like the normal stopping scenario in [1].

[1] http://dxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#l1796

[STR]
1. Go to http://mozilla.github.io/webrtc-landing/gum_test.html
2. Select Audio
3. Accept permissions
4. Long press the home button
5. Kill Browser app from card view

Expected:
Recording icon disappears after browser app is killed.

Actual:
Recording icon still appears in the status bar.

Comment 1

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

The similar scenario occurs on audio channel change notification but it does send when app is killed.

If we could do the same thing in gecko, the alternative workaround is let system app knows who(page) is doing recording and watch the life cycle of the page frame.

Comment 2

[Jason Smith [:jsmith]]

FWIW - With bug 828600 already existing, this issue isn't immediately obvious that it exists from a user's perspective, but it certainly makes the current situation with the recording icon worse.

Comment 3

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Chrome process need to keep the list of child processes that start recording and listen to "ipc:content-shutdown" event. While content process get killed, chrome process need to send out "recording-device-events:shutdown" event for the killed process. We can do it in ContentParent.cpp.

Comment 4

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: manage-child-window-lifecycle.patch

Manage recording status with childId and send additional recording status while content process get killed.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 811983 [details] [diff] [review]
manage-child-window-lifecycle.patch

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

I have very little to say about this bug, as I don't know shell.js, but so far as I see, r+

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 811983 [details] [diff] [review]
manage-child-window-lifecycle.patch

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

r=me

Comment 7

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: manage-child-window-lifecycle.patch

Update patch for handling multiple gUM requests in one process, carry khuey's r+.

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 817094 [details] [diff] [review]
manage-child-window-lifecycle.patch

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

Mostly out of my area, but so far as I can tell r+

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 817094 [details] [diff] [review]
manage-child-window-lifecycle.patch

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

Let me know what you think about my suggestions.

::: b2g/chrome/content/shell.js
@@ +1246,5 @@
>  (function recordingStatusTracker() {
>    let gRecordingActiveCount = 0;
> +  let gRecordingActiveProcesses = {};
> +
> +  let recording_handler = function(aSubject, aTopic, aData) {

recording_handler -> recordingHandler

@@ +1253,5 @@
> +
> +    if (aTopic === 'recording-device-events') {
> +      switch (aData) {
> +        case 'starting':
> +          count = 1;

I'm not sure to understand, don't you want to increment the count instead of setting it to '1' ? (same thing for -1).

@@ +1300,5 @@
> +        type: 'recording-status',
> +        active: (gRecordingActiveCount > 0)
> +      });
> +    }
> +  };

What about:

let gRecordingActiveCount = 0;
let gRecordingActiveProcesses = {};

let recordHandler = function(aSubject, aTopic, aData) {
  if (aTopic !== 'recording-device-events')
    return; // XXX do we really need recording-device-ipc-events?
  
   // XXX Also I don't know if there is more messages than
   // starting/shutdown. If yes then we can also early return 
   // so every messages here means there is a change in
   // gRecordingActiveCount.

  // Maintain the recording status per processes.
  let props = aSubject.QueryInterface(Ci.nsIPropertyBag2);
  let processId = aSubject.get('childID') || 'main';
  if (processId &&  !gRecordingActiveProcesses.hasOwnProperty(processId)) {
    gRecordingActiveProcesses[processId] = 0;
  }

  let currentActive = gRecordingActiveProcesses[processId];
  switch (aData) {
    case 'starting':
      gRecordingActiveCount++;
      currentActive++;
      break;

    case 'shutdown':
    case 'content-shutdown': // XXX do we need content-shutdown too?
      gRecordingActiveCount--;
      currentActive--;
      break;
  }

  if (currentActive === 0) {
    delete gRecordingActiveProcesses[processId];
  }

  shell.sendChromeEvent({
    type: 'recording-status',
    active: (gRecordingActiveCount > 0)
  });
}

@@ +1302,5 @@
> +      });
> +    }
> +  };
> +  Services.obs.addObserver(recording_handler, 'recording-device-events', false);
> +  Services.obs.addObserver(recording_handler, 'recording-device-ipc-events', false);

Let's use only recording-device-events.

@@ +1309,5 @@
> +    // send additional recording events if content process is being killed
> +    let props = aSubject.QueryInterface(Ci.nsIPropertyBag2);
> +    let childId = aSubject.get('childID');
> +    if (gRecordingActiveProcesses.hasOwnProperty(childId) >= 0) {
> +      Services.obs.notifyObservers(aSubject, 'recording-device-ipc-events', 'content-shutdown');

'recording-device-ipc-events' -> 'recording-device-events' ?
'content-shutdown' -> 'shutdown' ?

::: dom/ipc/ContentParent.cpp
@@ +1933,5 @@
> +        nsRefPtr<nsHashPropertyBag> props = new nsHashPropertyBag();
> +        props->SetPropertyAsUint64(NS_LITERAL_STRING("childID"), mChildID);
> +        obs->NotifyObservers((nsIPropertyBag2*) props,
> +                             "recording-device-ipc-events",
> +                             aRecordingStatus.get());

I'm not qualified to review this change.

Comment 10

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #9)
> Comment on attachment 817094 [details] [diff] [review]
> manage-child-window-lifecycle.patch
> 
> Review of attachment 817094 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Let me know what you think about my suggestions.
> 
> ::: b2g/chrome/content/shell.js
> @@ +1246,5 @@
> >  (function recordingStatusTracker() {
> >    let gRecordingActiveCount = 0;
> > +  let gRecordingActiveProcesses = {};
> > +
> > +  let recording_handler = function(aSubject, aTopic, aData) {
> 
> recording_handler -> recordingHandler
OK for me.
> 
> @@ +1253,5 @@
> > +
> > +    if (aTopic === 'recording-device-events') {
> > +      switch (aData) {
> > +        case 'starting':
> > +          count = 1;
> 
> I'm not sure to understand, don't you want to increment the count instead of
> setting it to '1' ? (same thing for -1).
Yes, this variable should be named as countDifference. For starting event, count is increased for 1. For shutdown event, count is decreased for 1. For content-shutdown, count is decreased depending on how many active count is remain for that content process since we can open multiple gUM request in one content process.
> 
> @@ +1300,5 @@
> > +        type: 'recording-status',
> > +        active: (gRecordingActiveCount > 0)
> > +      });
> > +    }
> > +  };
> 
> What about:
> 
> let gRecordingActiveCount = 0;
> let gRecordingActiveProcesses = {};
> 
> let recordHandler = function(aSubject, aTopic, aData) {
>   if (aTopic !== 'recording-device-events')
>     return; // XXX do we really need recording-device-ipc-events?
multiple gUM requests in content process is possible, so we'll need bookkeeping the active count for each content process. See reasons in below.
>   
>    // XXX Also I don't know if there is more messages than
>    // starting/shutdown. If yes then we can also early return 
>    // so every messages here means there is a change in
>    // gRecordingActiveCount.
> 
>   // Maintain the recording status per processes.
>   let props = aSubject.QueryInterface(Ci.nsIPropertyBag2);
>   let processId = aSubject.get('childID') || 'main';
>   if (processId &&  !gRecordingActiveProcesses.hasOwnProperty(processId)) {
>     gRecordingActiveProcesses[processId] = 0;
>   }
> 
>   let currentActive = gRecordingActiveProcesses[processId];
>   switch (aData) {
>     case 'starting':
>       gRecordingActiveCount++;
>       currentActive++;
>       break;
> 
>     case 'shutdown':
>     case 'content-shutdown': // XXX do we need content-shutdown too?
content-shutdown should decrease all the active count for that content process. See reason in below.
>       gRecordingActiveCount--;
>       currentActive--;
>       break;
>   }
> 
>   if (currentActive === 0) {
>     delete gRecordingActiveProcesses[processId];
>   }
> 
>   shell.sendChromeEvent({
>     type: 'recording-status',
>     active: (gRecordingActiveCount > 0)
>   });
> }
> 
> @@ +1302,5 @@
> > +      });
> > +    }
> > +  };
> > +  Services.obs.addObserver(recording_handler, 'recording-device-events', false);
> > +  Services.obs.addObserver(recording_handler, 'recording-device-ipc-events', false);
> 
> Let's use only recording-device-events.
We need a way bookkeeping the number of active recording for each content process so that we can decrease all the counts if content process is killed, e.g. by user or by OOM killer. That's the reason I separate into a difference topic.

I have a test page for multiple gUM requests in a single content process. http://people.mozilla.org/~schien/gum-test.html
> 
> @@ +1309,5 @@
> > +    // send additional recording events if content process is being killed
> > +    let props = aSubject.QueryInterface(Ci.nsIPropertyBag2);
> > +    let childId = aSubject.get('childID');
> > +    if (gRecordingActiveProcesses.hasOwnProperty(childId) >= 0) {
> > +      Services.obs.notifyObservers(aSubject, 'recording-device-ipc-events', 'content-shutdown');
> 
> 'recording-device-ipc-events' -> 'recording-device-events' ?
> 'content-shutdown' -> 'shutdown' ?
The same reason above.
> 
> ::: dom/ipc/ContentParent.cpp
> @@ +1933,5 @@
> > +        nsRefPtr<nsHashPropertyBag> props = new nsHashPropertyBag();
> > +        props->SetPropertyAsUint64(NS_LITERAL_STRING("childID"), mChildID);
> > +        obs->NotifyObservers((nsIPropertyBag2*) props,
> > +                             "recording-device-ipc-events",
> > +                             aRecordingStatus.get());
> 
> I'm not qualified to review this change.
That's ok, khuey reviewed this part already.

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Shih-Chiang Chien [:schien] from comment #10)
> > > +
> > > +    if (aTopic === 'recording-device-events') {
> > > +      switch (aData) {
> > > +        case 'starting':
> > > +          count = 1;
> > 
> > I'm not sure to understand, don't you want to increment the count instead of
> > setting it to '1' ? (same thing for -1).
> Yes, this variable should be named as countDifference. For starting event,
> count is increased for 1. For shutdown event, count is decreased for 1. For
> content-shutdown, count is decreased depending on how many active count is
> remain for that content process since we can open multiple gUM request in
> one content process.

Ok so does the code I suggest with a:

case 'content-shutdown':
  gRecordActiveCount -= currentActive;
  currentActive = 0;
  break;

will work for you if you keep the specific "ipc" names?

Comment 12

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #11)
> (In reply to Shih-Chiang Chien [:schien] from comment #10)
> > > > +
> > > > +    if (aTopic === 'recording-device-events') {
> > > > +      switch (aData) {
> > > > +        case 'starting':
> > > > +          count = 1;
> > > 
> > > I'm not sure to understand, don't you want to increment the count instead of
> > > setting it to '1' ? (same thing for -1).
> > Yes, this variable should be named as countDifference. For starting event,
> > count is increased for 1. For shutdown event, count is decreased for 1. For
> > content-shutdown, count is decreased depending on how many active count is
> > remain for that content process since we can open multiple gUM request in
> > one content process.
> 
> Ok so does the code I suggest with a:
> 
> case 'content-shutdown':
>   gRecordActiveCount -= currentActive;
>   currentActive = 0;
>   break;
> 
> will work for you if you keep the specific "ipc" names?

Yes, it should work. Let me update my patch.

Comment 13

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: manage-child-window-lifecycle.patch v3

update patch according to review comment, carry khuey and jesup's r+.

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 818200 [details] [diff] [review]
manage-child-window-lifecycle.patch v3

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

I would like to understand how we can receive a 'shutdown' that does not match a 'starting' before r+'ing.

::: b2g/chrome/content/shell.js
@@ +1262,5 @@
> +        gRecordingActiveCount++;
> +        currentActive++;
> +        break;
> +      case 'shutdown':
> +        if (currentActive > 0) {

Can it really happens?

@@ +1281,5 @@
> +    }
> +
> +    // We need to track changes from N <-> 0
> +    if ((oldCount === 0 && gRecordingActiveCount > 0) ||
> +        (gRecordingActiveCount === 0 && oldCount > 0)) {

I think this is related to the 'shutdown' case where I was wondering if it can really happens that we received a shutdown without having receive a starting before. If it can not happens then you are sure that the count has changed when you entered this function. Or am I missing something?

Comment 15

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #14)
> Comment on attachment 818200 [details] [diff] [review]
> manage-child-window-lifecycle.patch v3
> 
> Review of attachment 818200 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I would like to understand how we can receive a 'shutdown' that does not
> match a 'starting' before r+'ing.
> 
> ::: b2g/chrome/content/shell.js
> @@ +1262,5 @@
> > +        gRecordingActiveCount++;
> > +        currentActive++;
> > +        break;
> > +      case 'shutdown':
> > +        if (currentActive > 0) {
> 
> Can it really happens?
Currently this happens in Camera app. The camera app will trigger a shutdown event even if no ongoing video recording. I'll file a follow-up bug to fix this mismatching in camera control API, but I would like to keep this statement as a safe guard.
> 
> @@ +1281,5 @@
> > +    }
> > +
> > +    // We need to track changes from N <-> 0
> > +    if ((oldCount === 0 && gRecordingActiveCount > 0) ||
> > +        (gRecordingActiveCount === 0 && oldCount > 0)) {
> 
> I think this is related to the 'shutdown' case where I was wondering if it
> can really happens that we received a shutdown without having receive a
> starting before. If it can not happens then you are sure that the count has
> changed when you entered this function. Or am I missing something?
One single page can request multiple gUM request. I follow the original logic to only send mozChromeEvent at the first recording start and at the last active recording shutdown. Do you want to change the behavior to send it on every valid recording device event?

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Shih-Chiang Chien [:schien] from comment #15)
> (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #14)
> > Comment on attachment 818200 [details] [diff] [review]
> > manage-child-window-lifecycle.patch v3
> > 
> > Review of attachment 818200 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I would like to understand how we can receive a 'shutdown' that does not
> > match a 'starting' before r+'ing.
> > 
> > ::: b2g/chrome/content/shell.js
> > @@ +1262,5 @@
> > > +        gRecordingActiveCount++;
> > > +        currentActive++;
> > > +        break;
> > > +      case 'shutdown':
> > > +        if (currentActive > 0) {
> > 
> > Can it really happens?
> Currently this happens in Camera app. The camera app will trigger a shutdown
> event even if no ongoing video recording. I'll file a follow-up bug to fix
> this mismatching in camera control API, but I would like to keep this
> statement as a safe guard.
> > 

A followup would be fine. Can you add a bug number to the followup next to the if statement ?

Comment 17

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 818200 [details] [diff] [review]
manage-child-window-lifecycle.patch v3

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

r+ but please add a comment with the bug number.

Comment 18

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Bug 928206 is filed for the "shutdown without starting" issue.

Comment 19

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: manage-child-window-lifecycle.patch v3.1

update according to review comment, carry r+.

Comment 20

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 21

[(PTO June 19-July 7) Ryan VanderMeulen]

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

Comment 22

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/365abb5f2998

Comment 23

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d26ba3e1683b

Comment 24

[Jason Smith [:jsmith]]

Verified on 11/1 1.2 build - I saw the recording icon disappear after killing the browser & app process while gUM audio was active