Closed Bug 919149 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:koi+, firefox25 wontfix, firefox26 fixed, firefox27 fixed, b2g-v1.2 verified)

RESOLVED FIXED
blocking-b2g koi+
Tracking Status
firefox25 --- wontfix
firefox26 --- fixed
firefox27 --- fixed
b2g-v1.2 --- verified

People

(Reporter: schien, Assigned: schien)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
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.
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.
blocking-b2g: --- → koi?
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.
Manage recording status with childId and send additional recording status while content process get killed.
Assignee: nobody → schien
Attachment #811983 - Flags: review?(rjesup)
Attachment #811983 - Flags: review?(khuey)
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+
Attachment #811983 - Flags: review?(rjesup) → review+
Comment on attachment 811983 [details] [diff] [review]
manage-child-window-lifecycle.patch

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

r=me
Attachment #811983 - Flags: review?(khuey) → review+
blocking-b2g: koi? → koi+
Update patch for handling multiple gUM requests in one process, carry khuey's r+.
Attachment #811983 - Attachment is obsolete: true
Attachment #817094 - Flags: review?(rjesup)
Attachment #817094 - Flags: review?(21)
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+
Attachment #817094 - Flags: review?(rjesup) → review+
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.
Attachment #817094 - Flags: review?(21)
(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.
Flags: needinfo?(21)
(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?
Flags: needinfo?(21) → needinfo?(schien)
(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.
Flags: needinfo?(schien)
update patch according to review comment, carry khuey and jesup's r+.
Attachment #817094 - Attachment is obsolete: true
Attachment #818200 - Flags: review?(21)
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?
Attachment #818200 - Flags: review?(21)
(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?
Flags: needinfo?(21)
(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 ?
Flags: needinfo?(21)
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.
Attachment #818200 - Flags: review+
Bug 928206 is filed for the "shutdown without starting" issue.
update according to review comment, carry r+.
Attachment #818200 - Attachment is obsolete: true
Attachment #818815 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/365abb5f2998
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Keywords: verifyme
Verified on 11/1 1.2 build - I saw the recording icon disappear after killing the browser & app process while gUM audio was active
Duplicate of this bug: 833571
You need to log in before you can comment on or make changes to this bug.