Closed Bug 882136 Opened 6 years ago Closed 6 years ago

Camera share icon gets stuck in the notification bar; media lock held on tab close

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 24
Tracking Status
firefox24 --- verified
fennec 24+ ---

People

(Reporter: ibarlow, Assigned: wesj)

References

(Blocks 1 open bug)

Details

(Keywords: reproducible, Whiteboard: [getUserMedia][android-gum+])

Attachments

(1 file, 1 obsolete file)

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.
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!
tracking-fennec: --- → ?
OS: Mac OS X → Android
Hardware: x86 → ARM
Whiteboard: [getUserMedia][android-gum?]
Whiteboard: [getUserMedia][android-gum?] → [getUserMedia][android-gum+]
Duplicate of this bug: 882627
Assignee: nobody → wjohnston
tracking-fennec: ? → 24+
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).
Summary: Camera share icon gets stuck in the notification bar → Camera share icon gets stuck in the notification bar; media lock held on tab close
Blocks: 881875
Keywords: reproducible
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #763913 - Flags: review?(lucasr.at.mozilla)
Attached patch PatchSplinter Review
Removed some logging and comments.
Attachment #763913 - Attachment is obsolete: true
Attachment #763913 - Flags: review?(lucasr.at.mozilla)
Attachment #763916 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #763916 - Flags: review?(lucasr.at.mozilla) → review+
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
"NotificationHelper_ID" ? underscores in strings?
https://hg.mozilla.org/mozilla-central/rev/189e40137b04
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Keywords: verifyme
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.
bug 885783 filed as a followup.
Depends on: 885783
Keywords: verifyme
Status: RESOLVED → VERIFIED
I too can reproduce this well (Nightly 06/21), let's investigate in bug 885783.
You need to log in before you can comment on or make changes to this bug.