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

[Ian Barlow (:ibarlow)]

Steps to reproduce: 
- Pref on webrtc
- Go to http://mozilla.github.io/webrtc-landing/gum_test.html
- Enable camera/audio sharing
- Close gum_test tab

expected result
- Camera icon disappears 

actual result
- Camera icon remains in the notification tray and cannot be cleared, neither by swiping, nor closing firefox, nor starting and stopping a new webrtc session. I have to restart my phone to get rid of it.

Comment 1

[Aaron Train [:aaronmt]]

I can reproduce this on the Alcatel One Touch 8008X (Android 4.1) as well; I closed the tab and the surface preview was still visible with the notification still present.

I suspect the larger issue here is not severing the local media connection on closing a tab!

Comment 3

[Aaron Train [:aaronmt]]

I believe this bug is the symptom of the issue with local media being active on tab close.

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). Results are what I see in comment #1 (notification stuck and surface preview still visible).

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch v1

This basically keeps a list of notifications (at least the ones that go through the NotificationHelper) and forces them all closed when the app is destroyed (or the notification is tapped). Notifications that go through other channels shouldn't be affected. Ongoing notifications stay around even if tapped.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch

Removed some logging and comments.

Comment 6

[Lucas Rocha (:lucasr)]

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

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

Looks good (with suggested fixes).

::: mobile/android/base/GeckoApp.java
@@ +1781,5 @@
>          }
>          handleNotification(ACTION_ALERT_CALLBACK, alertName, alertCookie);
> +
> +        if (intent.hasExtra("NotificationHelper_ID")) {
> +            String id = intent.getStringExtra("NotificationHelper_ID");

final String id...

::: mobile/android/base/NotificationHelper.java
@@ +27,4 @@
>  public class NotificationHelper implements GeckoEventListener {
>      private static final String LOGTAG = "GeckoNotificationManager";
> +    private Context mContext;
> +    List<String> mShowing;

private?

@@ +31,4 @@
>  
>      public NotificationHelper(Context context) {
>          mContext = context;
> +        mShowing = new ArrayList<String>();

I'm going to assume that this list will be very short in most cases. Checking 'contains' in a List is not very efficient. If efficiency is needed here, I'd use something like a SparseBooleanArray or similar.

@@ +98,5 @@
>          notificationIntent.setFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
> +        // if this isn't an ongoing notification, add the id to the intent so that we
> +        // can remove the notification from our list of active notifications if its clicked
> +        if (!ongoing) {
> +            notificationIntent.putExtra("NotificationHelper_ID", id);

Turn "NotificationHelper_ID" into a public constant in NotificationHelper and use it here and in BrowserApp.

@@ +118,5 @@
>              id = message.getString("id");
>          } catch (JSONException ex) {
>              Log.i(LOGTAG, "Error parsing", ex);
>              return;
>          }

nit: empty line here.

@@ +127,3 @@
>          NotificationManager manager = (NotificationManager) mContext.getSystemService(Context.NOTIFICATION_SERVICE);
>          manager.cancel(id.hashCode());
> +        if (mShowing.contains(id)) {

remove() is idempotent, no need to check if it contains id before trying.

Comment 7

[Wesley Johnston (:wesj)]

I switch from an ArrayList to a HashSet for the list. This list should never be very big, but it should be more performant. And these IDs should be unique:

https://hg.mozilla.org/integration/mozilla-inbound/rev/189e40137b04

Comment 8

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

"NotificationHelper_ID" ? underscores in strings?

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/189e40137b04

Comment 10

[Jason Smith [:jsmith]]

Not working for me or one other person in the testday today. The issue cited in comment 0 is still present even with landing of this patch. Filing a followup.

Comment 11

[Jason Smith [:jsmith]]

bug 885783 filed as a followup.

Comment 12

[Aaron Train [:aaronmt]]

I too can reproduce this well (Nightly 06/21), let's investigate in bug 885783.