885783 - Followup to bug 882136 - Camera share icon gets stuck in the notification bar; media lock held on tab close

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jason Smith [:jsmith]]

Same problem as cited in https://bugzilla.mozilla.org/show_bug.cgi?id=882136#c0. The patch apparently didn't fix the problem.

Comment 1

[Wesley Johnston (:wesj)]

This needs more specific STR.

Comment 2

[Jason Smith [:jsmith]]

(In reply to Wesley Johnston (:wesj) from comment #1)
> This needs more specific STR.

In what sense? This reproduces for me in cases such as:

* Killing the FxAndroid app in the task switcher
* Closing the tab while the camera/mic is still in use

Comment 3

[Wesley Johnston (:wesj)]

Can you list devices/android versions? Does it happen in all cases or just some?

I can't reproduce this (otherwise I wouldn't have checked in the patch).

Comment 4

[Aaron Train [:aaronmt]]

I'm using the same steps from the previous bug:

'This is reproducible on the Nexus 7 (Android 4.2.2) (http://mozilla.github.io/webrtc-landing/gum_test.html, share audio and video, close the active tab)'

Today I used the Samsung Galaxy S4 (Android 4.2.2) though.

Comment 5

[Alexander Wang]

Attachment: Screenshot of Nightly camera entries in notification tray after Nightly is closed

The "Camera is on" notification remains in the notification tray after Nightly is closed. The entries in this screenshot appear twice because I repeated the procedure twice:

1. Go to http://mozilla.github.io/webrtc-landing/gum_test.html
2. Click on Video.
3. Share camera with Nightly.
4. After video stream comes up, press home key.
5. Hold down home key to bring up menu of apps running.
6. Swipe away the Nightly entry.
7. Check notification tray by pulling it down from the top of the screen.

Comment 6

[Alexander Wang]

After Nightly is closed and notifications are left, I can not remove the notifications. The "Nightly: Camera is on" notifications can not be swiped away.

I was able to remove them by going into Settings -> Apps. I found Nightly's entry. Then I tapped on "Force stop", followed by a tap on "OK", which removed the entries in the notification tray.

I am using:
Samsung Galaxy SII, AT&T Skyrocket (SAMSUNG-SGH-I727)
Android 4.1.2, Cyanogenmod version 10.0.0-skyrocket

Comment 7

[gal007]

Attachment: Video demo

http://www.youtube.com/watch?v=mvRCi26Owic&feature=youtu.be

Comment 8

[Gian-Carlo Pascutto [:gcp]]

Moving this to a blocker as we can't really ship the feature with this bug in it.

Comment 9

[Gian-Carlo Pascutto [:gcp]]

After reading through the documentation for Notifications, I don't think the current solution can work, as there is no way for us to remove the notification after the browser has been killed (which can happen at any time).

I think we need this instead:
http://developer.android.com/guide/components/services.html#Foregroundhttp://developer.android.com/guide/components/services.html#Foreground

Comment 10

[Brian Nicholson (:bnicholson)]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #9)
> After reading through the documentation for Notifications, I don't think the
> current solution can work, as there is no way for us to remove the
> notification after the browser has been killed (which can happen at any
> time).

Yes, that's correct. We ran into this same problem with bug 823285 (in particular, see https://bugzilla.mozilla.org/show_bug.cgi?id=823285#c5).

The Android lifecycle gives no reliable way to implement application shutdown events (this is why createShutDownEvent() was also removed in that bug; see https://bugzilla.mozilla.org/show_bug.cgi?id=823285#c21). onDestroy() is useful only for saving state when the activity is killed gracefully with the expectation that it will be restored (e.g., being killed while in the background or with "Don't keep activities" enabled).

I'll see if we can use a similar approach here.

Comment 11

[Brian Nicholson (:bnicholson)]

(In reply to Brian Nicholson (:bnicholson) from comment #10)
> onDestroy() is useful only for saving state when the activity is killed
> gracefully with the expectation that it will be restored

Sorry, onPause() is used for saving state, not onDestroy(). What I should have said is that onDestroy() is useful only for cleaning up resources when the activity is destroyed while the application stays alive. "Don't keep activities" is an obvious situation where this happens.

Comment 12

[Wesley Johnston (:wesj)]

Brian, do you actually have time to look at this? I started looking at it this afternoon...

Comment 13

[Brian Nicholson (:bnicholson)]

Wes already started looking at this, so handing it back.

Comment 14

[Wesley Johnston (:wesj)]

Attachment: Patch

This hooks these notifications up to the same service that downloads use. Trying to keep this fairly small to make uplift safe. This won't work for webapps. I'm willing to punt on that for now. Most of this is just making methods that took AlertNotification objects take more generic Notification objects, and expanding the definition of what can be a foreground notification to include ongoing ones.

Comment 15

[Jason Smith [:jsmith]]

Would it be worthwhile to kick off a try build with this patch and do some exploratory testing on it?

Comment 16

[Wesley Johnston (:wesj)]

Yes. This seemed more reliable to me, but like I mentioned, the old patch worked sometimes as well:

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

Comment 17

[Gian-Carlo Pascutto [:gcp]]

Do a hard kill of Fennec from the task manager (or even better, the commandline). The old patch would always have its icons stay then.

Comment 18

[Brian Nicholson (:bnicholson)]

Comment on attachment 774490 [details] [diff] [review]
Patch

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

r- for some questions, mainly about the way you're changing NotificationHandler to use both Notification and AlertNotification.

I don't like how this is trying to use the service for two different kinds of notifications (our own AlertNotification and the android.app.Notification); it's less OO than having a single, self-contained class that holds its own notification data like we had with AlertNotification. Code starts to smell when you see things like "notification instanceof AlertNotification" and having to change the method signatures from AlertNotification to a pairing of notificationId + Notification. Is AlertNotification not generic enough to be used for these WebRTC notifications? If not, could we create an interface used by both kinds of notifications so NotificationHandler doesn't need to be split between the different notification types?

::: mobile/android/base/NotificationHandler.java
@@ +64,5 @@
> +        mNotificationManager.notify(notificationID, notification);
> +        mNotifications.put(notificationID, notification);
> +    }
> +
> +    public void add(int id, Notification notification) {

Can you add Javadocs to this method for consistency with the rest of the file?

@@ +115,2 @@
>          if (notification != null) {
> +            updateForegroundNotification(notificationID, notification);

What's the point of reordering these lines? I'm worried about the short period between cancelling and updating the foreground notification. During that period, there's no foreground notification set, so I think we could be killed right after cancelling the notification if we're under memory pressure.

@@ +134,5 @@
>       *
>       * @param notificationID the notification to check
>       * @return               whether the notification is progress style
>       */
> +    public boolean isForeground(int notificationID) {

I don't understand the name change. There can only be one foreground notification at a time, so "isForeground" seems misleading. Would something like "isInProgress" make sense?

@@ +140,3 @@
>      }
>  
> +    public boolean isForeground(Notification notification) {

Can you add Javadocs to this method for consistency with the rest of the file?

@@ +140,4 @@
>      }
>  
> +    public boolean isForeground(Notification notification) {
> +        if (isProgressStyle(notification) || isOngoing(notification)) {

Is there a need for both of these? They seem redundant -- when would a progress style notification not be an ongoing one, or vice-versa?

I imagine it's because we're using two different types of notifications, but if so, it would be much nicer if we could consolidate these somehow, either by using AlertNotification for both notifications, or using a shared interface where isOngoing() is implemented by each subclass.

@@ +145,5 @@
> +        }
> +        return false;
> +    }
> +
> +    public boolean isOngoing(Notification notification) {

s/public/private

@@ +149,5 @@
> +    public boolean isOngoing(Notification notification) {
> +        return (notification.flags & Notification.FLAG_ONGOING_EVENT) > 0;
> +    }
> +
> +    public boolean isProgressStyle(Notification notification) {

s/public/private/

::: mobile/android/base/NotificationService.java
@@ +17,5 @@
>  
> +    @Override
> +    public void onCreate() {
> +        mHandler = new NotificationHandler(this) {
> +            @Override

What's the point of moving this into onCreate()?

Comment 19

[Jason Smith [:jsmith]]

Tried out Wes's try build on a Samsung Galaxy Nexus running Android 4.2.

Looks good to me. I'm seeing the persistent share icon disappearing when FxAndroid gets killed by the task manager. I also regression tested the patch other workflows (e.g. tab closed) still worked.

Comment 20

[Brian Nicholson (:bnicholson)]

(In reply to Jason Smith [:jsmith] from comment #19)
> Tried out Wes's try build on a Samsung Galaxy Nexus running Android 4.2.
> 
> Looks good to me. I'm seeing the persistent share icon disappearing when
> FxAndroid gets killed by the task manager. I also regression tested the
> patch other workflows (e.g. tab closed) still worked.

When you say "killed by the task manager", do you mean hitting "Force stop"? I think that using "Force stop" will always get rid of all notifications for an app, even without this fix (at least that's the case on my device; implementations may vary). The problem in this bug is that we kill the activity

Since this uses the same fix as bug 823285, the behavior I would expect is that Fennec actually can't be killed at all as long as the notification is visible. That is, if you try to swipe Fennec from the recent apps list, the notification will still be at the top, and "reopening" Fennec will just show the same page since Fennec was never killed. Is this also what you're seeing?

Comment 21

[Jason Smith [:jsmith]]

(In reply to Brian Nicholson (:bnicholson) from comment #20)
> (In reply to Jason Smith [:jsmith] from comment #19)
> > Tried out Wes's try build on a Samsung Galaxy Nexus running Android 4.2.
> > 
> > Looks good to me. I'm seeing the persistent share icon disappearing when
> > FxAndroid gets killed by the task manager. I also regression tested the
> > patch other workflows (e.g. tab closed) still worked.
> 
> When you say "killed by the task manager", do you mean hitting "Force stop"?
> I think that using "Force stop" will always get rid of all notifications for
> an app, even without this fix (at least that's the case on my device;
> implementations may vary). The problem in this bug is that we kill the
> activity

Killed in the task manager = opening up the set of recent apps I launched and swiping the app to the right to kill the FxAndroid process. I actually didn't try out force stop directly in settings when i originally tested this.

> 
> Since this uses the same fix as bug 823285, the behavior I would expect is
> that Fennec actually can't be killed at all as long as the notification is
> visible. That is, if you try to swipe Fennec from the recent apps list, the
> notification will still be at the top, and "reopening" Fennec will just show
> the same page since Fennec was never killed. Is this also what you're seeing?

Nope, I'm not seeing that. When I swipe Fennec away in the recent apps list, the notification does go away. If I launch Fennec again, then I start at the about:home page for Fennec.

Comment 22

[Brian Nicholson (:bnicholson)]

Interesting...I think I might see what's going on here.

We never backed out bug 882136. Bug 882136 didn't fix the issue because swiping to kill Fennec will normally kill the application, and that means that onDestroy() generally won't be called in the activity. With this patch, we're forcing the application to stay alive because of the foreground notification. Since the application is being forced to stay alive, the activity will go through its normal lifecycle callbacks -- including onDestroy() (the same thing that happens when "Don't keep activities" is enabled. And since we never backed out bug 882136, this patch is actually enabling bug 882136 to work correctly.

If this theory is correct, that means that the notification will disappear whenever "Don't keep activities" is enabled and the user leaves Fennec. Consequently, I'd recommend backing out bug 882136. Notifications will then act the same as download notifications (swipe-killing Fennec will actually not kill the application), and they will behave properly with "Don't keep activities".

Comment 23

[Wesley Johnston (:wesj)]

(In reply to Brian Nicholson (:bnicholson) from comment #18)
> I don't like how this is trying to use the service for two different kinds
> of notifications (our own AlertNotification and the
> android.app.Notification); it's less OO than having a single, self-contained
> class that holds its own notification data like we had with
> AlertNotification. Code starts to smell when you see things like

AlertNotification was only ever created because we needed it to show progress in notifications. That ability is handled in the NotificationCompat class now. I want to kill AlertNotification. Until its dead, it seems good to me to have this class accept generic notification objects, especially since the number of non-notification methods it needs is basically zero. Even the progress method its still using could be replaced by setting some more generic flags on the AlertNotifications. I was trying to minimize changes here since this needs uplift.

> What's the point of reordering these lines? I'm worried about the short

No point. I just didn't need the notification anymore, so I moved the cancel outside the if. I can move it under.

> I don't understand the name change. There can only be one foreground
> notification at a time, so "isForeground" seems misleading. Would something
> like "isInProgress" make sense?

isProgress just seemed wrong, since the webrtc notifications aren't showing progress bars. How about we just make isOngoing the important one here, with the intention of removing isProgress anyway?

> Is there a need for both of these? They seem redundant -- when would a
> progress style notification not be an ongoing one, or vice-versa?

There webrtc notifications are ongoing, but don't show progress. I agree the opposite is pretty rare, but I could imagine there being a non-ongoing process that you'd happily abort if Fennec died.

> What's the point of moving this into onCreate()?
The Service context isn't actually able to get a handle to the NotificationManager if onCreate hasn't been called yet. We have to delay the initialization. I'll add a comment about that.

Comment 24

[Wesley Johnston (:wesj)]

(In reply to Jason Smith [:jsmith] from comment #21)
> (In reply to Brian Nicholson (:bnicholson) from comment #20)
> > (In reply to Jason Smith [:jsmith] from comment #19)
> > > Tried out Wes's try build on a Samsung Galaxy Nexus running Android 4.2.
> > > 
> > > Looks good to me. I'm seeing the persistent share icon disappearing when
> > > FxAndroid gets killed by the task manager. I also regression tested the
> > > patch other workflows (e.g. tab closed) still worked.
> > 
> > When you say "killed by the task manager", do you mean hitting "Force stop"?
> > I think that using "Force stop" will always get rid of all notifications for
> > an app, even without this fix (at least that's the case on my device;
> > implementations may vary). The problem in this bug is that we kill the
> > activity
> 
> Killed in the task manager = opening up the set of recent apps I launched
> and swiping the app to the right to kill the FxAndroid process. I actually
> didn't try out force stop directly in settings when i originally tested this.
> 
> > 
> > Since this uses the same fix as bug 823285, the behavior I would expect is
> > that Fennec actually can't be killed at all as long as the notification is
> > visible. That is, if you try to swipe Fennec from the recent apps list, the
> > notification will still be at the top, and "reopening" Fennec will just show
> > the same page since Fennec was never killed. Is this also what you're seeing?
> 
> Nope, I'm not seeing that. When I swipe Fennec away in the recent apps list,
> the notification does go away. If I launch Fennec again, then I start at the
> about:home page for Fennec.

This was intentional. The NotificationHelper class cleans up the notification when the activity (not application) is killed. WebRTC currently depends on the Fennec context to run anyway, so WebRTC actually dies at that point, along with the notification.

Comment 25

[Wesley Johnston (:wesj)]

(In reply to Wesley Johnston (:wesj) from comment #24)
> (In reply to Jason Smith [:jsmith] from comment #21)
> > (In reply to Brian Nicholson (:bnicholson) from comment #20)
> > > (In reply to Jason Smith [:jsmith] from comment #19)
> > > > Tried out Wes's try build on a Samsung Galaxy Nexus running Android 4.2.
> > > > 
> > > > Looks good to me. I'm seeing the persistent share icon disappearing when
> > > > FxAndroid gets killed by the task manager. I also regression tested the
> > > > patch other workflows (e.g. tab closed) still worked.
> > > 
> > > When you say "killed by the task manager", do you mean hitting "Force stop"?
> > > I think that using "Force stop" will always get rid of all notifications for
> > > an app, even without this fix (at least that's the case on my device;
> > > implementations may vary). The problem in this bug is that we kill the
> > > activity
> > 
> > Killed in the task manager = opening up the set of recent apps I launched
> > and swiping the app to the right to kill the FxAndroid process. I actually
> > didn't try out force stop directly in settings when i originally tested this.
> > 
> > > 
> > > Since this uses the same fix as bug 823285, the behavior I would expect is
> > > that Fennec actually can't be killed at all as long as the notification is
> > > visible. That is, if you try to swipe Fennec from the recent apps list, the
> > > notification will still be at the top, and "reopening" Fennec will just show
> > > the same page since Fennec was never killed. Is this also what you're seeing?
> > 
> > Nope, I'm not seeing that. When I swipe Fennec away in the recent apps list,
> > the notification does go away. If I launch Fennec again, then I start at the
> > about:home page for Fennec.
> 
> This was intentional. The NotificationHelper class cleans up the
> notification when the activity (not application) is killed. WebRTC currently
> depends on the Fennec context to run anyway, so WebRTC actually dies at that
> point, along with the notification.

Also, I think users will be very confused if they "kill" Fennec and it keeps using their camera/mic.

Comment 26

[Brian Nicholson (:bnicholson)]

(In reply to Wesley Johnston (:wesj) from comment #23)
> AlertNotification was only ever created because we needed it to show
> progress in notifications. That ability is handled in the NotificationCompat
> class now. I want to kill AlertNotification. Until its dead, it seems good
> to me to have this class accept generic notification objects, especially
> since the number of non-notification methods it needs is basically zero.
> Even the progress method its still using could be replaced by setting some
> more generic flags on the AlertNotifications. I was trying to minimize
> changes here since this needs uplift.

OK, instead of using AlertNotification for both, it sounds like the plan is to use Notification for both. I just wanted to avoid using two different types of notifications, so that's fine with me. Can you a file a follow-up for this change?

> isProgress just seemed wrong, since the webrtc notifications aren't showing
> progress bars. How about we just make isOngoing the important one here, with
> the intention of removing isProgress anyway?

Sure, sounds like that could be addressed in the same follow-up.

Comment 27

[Brian Nicholson (:bnicholson)]

(In reply to Wesley Johnston (:wesj) from comment #24)
> 
> This was intentional. The NotificationHelper class cleans up the
> notification when the activity (not application) is killed. WebRTC currently
> depends on the Fennec context to run anyway, so WebRTC actually dies at that
> point, along with the notification.

I guess this makes sense for now since WebRTC breaks anyway, but it seems like that should be fixed... Gecko's lifetime != GeckoApp's lifetime. Since WebRTC is running in Gecko, it thus makes no sense that the WebRTC's lifetime would be tied to the activity either. And once we fix this context issue, I think we'll want to find another approach other than using the activity's onDestroy() for the same reason.

Comment 28

[Wesley Johnston (:wesj)]

(In reply to Brian Nicholson (:bnicholson) from comment #27)
> I guess this makes sense for now since WebRTC breaks anyway, but it seems
> like that should be fixed... Gecko's lifetime != GeckoApp's lifetime. Since
> WebRTC is running in Gecko, it thus makes no sense that the WebRTC's
> lifetime would be tied to the activity either. And once we fix this context
> issue, I think we'll want to find another approach other than using the
> activity's onDestroy() for the same reason.

The nice thing with this setup is that when GeckoApp dies, we kill the notification, which releases the foreground service and allows Gecko to die as well killing any webrtc sessions at the same time.

i.e. this actually has the behaviour that I think users expect when they swipe Fennec from recent apps. I'm a bit curious if they expect our downloads to continue or die in the same situation...

Comment 29

[Brian Nicholson (:bnicholson)]

(In reply to Wesley Johnston (:wesj) from comment #28)
> (In reply to Brian Nicholson (:bnicholson) from comment #27)
> > I guess this makes sense for now since WebRTC breaks anyway, but it seems
> > like that should be fixed... Gecko's lifetime != GeckoApp's lifetime. Since
> > WebRTC is running in Gecko, it thus makes no sense that the WebRTC's
> > lifetime would be tied to the activity either. And once we fix this context
> > issue, I think we'll want to find another approach other than using the
> > activity's onDestroy() for the same reason.
> 
> The nice thing with this setup is that when GeckoApp dies, we kill the
> notification, which releases the foreground service and allows Gecko to die
> as well killing any webrtc sessions at the same time.
> 
> i.e. this actually has the behaviour that I think users expect when they
> swipe Fennec from recent apps. I'm a bit curious if they expect our
> downloads to continue or die in the same situation...

(In reply to Wesley Johnston (:wesj) from comment #28)
> The nice thing with this setup is that when GeckoApp dies, we kill the
> notification, which releases the foreground service and allows Gecko to die
> as well killing any webrtc sessions at the same time.

Note that the application staying alive is pretty standard for any app running a service. For example, if I start playing music in Pandora and swipe Pandora, the activity is killed, but the music service keeps playing. Similarly, when I start a download in Fennec, I'm starting a service, so I would expect Fennec to keep running.

I do agree this interaction is weirder for recording audio/video, but the current fix comes at the expense of breaking behavior with "Don't keep activities" enabled. If I'm in the middle of a recording and I receive a phone call, the notification goes away and the recording is frozen on the page. That seems like a big red flag that we're doing it wrong.

I won't complain too much about stopping WebRTC with the activity as long as we handle it more gracefully -- we should send some kind of message to Gecko rather than just having it die. In this particular case, I would expect the page to reset to its initial state, asking me how I want to start a recording again. Hopefully we have an API so pages can handle events like this?

Comment 30

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Brian Nicholson (:bnicholson) from comment #29)

> (In reply to Wesley Johnston (:wesj) from comment #28)
> > The nice thing with this setup is that when GeckoApp dies, we kill the
> > notification, which releases the foreground service and allows Gecko to die
> > as well killing any webrtc sessions at the same time.
> 
> Note that the application staying alive is pretty standard for any app
> running a service. For example, if I start playing music in Pandora and
> swipe Pandora, the activity is killed, but the music service keeps playing.
> Similarly, when I start a download in Fennec, I'm starting a service, so I
> would expect Fennec to keep running.
> 
> I do agree this interaction is weirder for recording audio/video, but the
> current fix comes at the expense of breaking behavior with "Don't keep
> activities" enabled. If I'm in the middle of a recording and I receive a
> phone call, the notification goes away and the recording is frozen on the
> page. That seems like a big red flag that we're doing it wrong.
> 
> I won't complain too much about stopping WebRTC with the activity as long as
> we handle it more gracefully -- we should send some kind of message to Gecko
> rather than just having it die. In this particular case, I would expect the
> page to reset to its initial state, asking me how I want to start a
> recording again. Hopefully we have an API so pages can handle events like
> this?

Do we have services for downloads or webrtc, such that they would possibly stay alive if GeckoApp was killed? If not now, we might in the future. But in any case, we could probably move ahead with this patch approach for now and file a followup for anything that should be changed in the short term. Otherwise, we wait until we have true services and need to worry about it.

Is there even a graceful way to tell the Gecko webrtc system to stop from the notification? If so, we could do that in the short term. Otherwise it seems like scope creep in this bug.

Comment 31

[Brian Nicholson (:bnicholson)]

(In reply to Mark Finkle (:mfinkle) from comment #30)
> Do we have services for downloads or webrtc, such that they would possibly
> stay alive if GeckoApp was killed?

The notification service is an actual Android service, set as a foreground service using setForeground(). So yes, that service keeps Gecko (and downloads) alive when GeckoApp is killed.

> Is there even a graceful way to tell the Gecko webrtc system to stop from
> the notification?

Assuming we could tell WebRTC to stop by sending some Gecko message, we could do that directly in GeckoApp's onDestroy() since that's where we're killing the notification.

Comment 32

[Wesley Johnston (:wesj)]

This is off track a bit, but Webrtc already tries to listen to app lifecycle somewhat (only onPause, but onDestroy wouldn't be hard to add. i.e. See:

http://mxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/video_capture/android/java/org/webrtc/videoengine/VideoCaptureAndroid.java#179

and: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#1485

The one thing I'm slightly changing my mind about, if we're holding a foreground service in Fennec, I don't think that does anything to encourage Android to hold onto GeckoApp, just keeps the application alive. Is that right?

If you're trying to use webrtc for a phone call, you likely don't want it to die because Android kills GeckoApp (but I still think you'd expect it to die if you intentionally swiped Fennec from recent apps). AFAIK, there's no way to distinguish those two situations though.

Comment 33

[Brian Nicholson (:bnicholson)]

(In reply to Wesley Johnston (:wesj) from comment #32)
> The one thing I'm slightly changing my mind about, if we're holding a
> foreground service in Fennec, I don't think that does anything to encourage
> Android to hold onto GeckoApp, just keeps the application alive. Is that
> right?

Yeah, setForeground() prevents the service from dying. That means keeping the service's process alive, and since the service is running in the application process, that in turn means we're keeping the application alive.

Comment 34

[Gian-Carlo Pascutto [:gcp]]

>This is off track a bit, but Webrtc already tries to listen to app lifecycle 
>somewhat

Nope, it doesn't, and it doesn't care. The Android Camera driver knows about it. WebRTC is oblivious. Those two aren't the same - WebRTC has no idea the video image is interrupted...

>If you're trying to use webrtc for a phone call, you likely don't want it to die because Android 
>kills GeckoApp

...because Android can't support sharing the camera and requires a "visible" preview surface in Android < 4.0.

Comment 35

[Brian Nicholson (:bnicholson)]

Comment on attachment 774490 [details] [diff] [review]
Patch

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

::: mobile/android/base/NotificationHandler.java
@@ +135,5 @@
>       * @param notificationID the notification to check
>       * @return               whether the notification is progress style
>       */
> +    public boolean isForeground(int notificationID) {
> +        return isForeground(notificationID);

I missed this on the first go, but you have isForeground(int) calling itself here, resulting in infinite recursion (found thanks to bug 893882).

Comment 36

[Wesley Johnston (:wesj)]

Attachment: Patch

This doesn't address all the lifetime stuff, but does address all the comments I think.

Comment 37

[Brian Nicholson (:bnicholson)]

Comment on attachment 776711 [details] [diff] [review]
Patch

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

r+ with comments addressed.

::: mobile/android/base/NotificationHandler.java
@@ +151,5 @@
> +     * @param notification   the notification to check
> +     * @return               whether the notification is ongoing
> +     */
> +    public boolean isOngoing(Notification notification) {
> +        if (isProgressStyle(notification) || ((notification.flags & Notification.FLAG_ONGOING_EVENT) > 0)) {

I think you need a `notification != null` check at the beginning of this if statement. It's possible that mNotifications.get(notificationID) in the above isOngoing() method can return null.

@@ +159,5 @@
> +    }
> +
> +    /**
> +     * Helper method to determines whether a notification is an AlertNotification that is showing progress
> +     * This method will be deprecated when AlertNotifications are removed (bug 893289). 

Nit: trailing space

@@ +171,5 @@
> +        }
> +        return false;
> +    }
> +
> +    protected void setForegroundNotification(Integer id, Notification notification) {

s/Integer/int/

@@ +176,4 @@
>          mForegroundNotification = notification;
>      }
>  
> +    private void updateForegroundNotification(Integer id, Notification oldNotification) {

s/Integer/int/

::: mobile/android/base/NotificationService.java
@@ +17,4 @@
>  
> +    @Override
> +    public void onCreate() {
> +        mHandler = new NotificationHandler(this) {

Can you add a comment explaining why this must be delayed until onCreate()?

@@ +18,5 @@
> +    @Override
> +    public void onCreate() {
> +        mHandler = new NotificationHandler(this) {
> +            @Override
> +            protected void setForegroundNotification(Integer id, Notification notification) {

s/Integer/int/

Comment 38

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0dd0605ba1c

Comment 39

[Wesley Johnston (:wesj)]

Comment on attachment 776711 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Webrtc
User impact if declined: webrtc notification can live forever
Testing completed (on m-c, etc.): landed on mc today.
Risk to taking this patch (and alternatives if risky): this isn't tiny, but is reusing as much existing code as I can. Its mostly just converting our NotificationsService to handle more generic notifications (i.e. accept Notifications instead of AlertNotifications), and then using it for these webrtc notifications). As an alternative, we could do something that's more of a hack like what I have in bug 892867, but I don't think that solution (hiding the notification if its still around when Fennec starts) is probably good enough to ship.
String or IDL/UUID changes made by this patch: none

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e0dd0605ba1c

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7d47ae7af1fc