Notification is dismissing on Android O

[naeemgul90]

Shuttle version:

v1.x.x

Device, OS:

Device info here

Description of bug:

Bug description here

Steps to reproduce:

1. Step 1 Play any song
2. Step 2 Pause the song while the app is closed
3. Step 3 Notification is dismissed

Expected outcome:

Expected outcome here Notification should not be dismissed

Observations/Actual Result:

Observations here What if we don't stop the service after dummy notification dismiss

[timusus]

@naeemgul90 which version of Shuttle?

[naeemgul90]

This issue is on the live version as well

[naeemgul90]

Steps to reproduce.

1. Clear the app from memory.
2. Open app and play song
3. Pause the song through notification playback

The dummy notification will appear and after that both notifications will be dismssed by itself

[GeoZac]

Reproducible on latest beta of Shuttle+. It could be helpful if the notification is not dismissed after the set time period.

[naeemgul90]

But showing dummy notification on playback notification events is bit teasing for users. Sometimes this dummy notification is not canceable until we clear the app from memory.

[naeemgul90]

Chaning this code in DummyNotificationHelper is fixing the problem of playback notification dismiss with dummy notificaiton and unwanted dummy notification appearance.

if(!isForegroundedByApp) {
           if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
               if (!isShowingDummyNotification) {
                   NotificationManager notificationManager = service.getSystemService(NotificationManager.class);
                   NotificationChannel channel = notificationManager.getNotificationChannel(CHANNEL_ID);
                   if (channel == null) {
                       channel = new NotificationChannel(CHANNEL_ID, service.getString(R.string.app_name), NotificationManager.IMPORTANCE_DEFAULT);
                       channel.enableLights(false);
                       channel.enableVibration(false);
                       channel.setSound(null, null);
                       channel.setShowBadge(false);
                       channel.setImportance(NotificationManager.IMPORTANCE_LOW);
                       notificationManager.createNotificationChannel(channel);
                   }

                   Notification notification = new Notification.Builder(service, CHANNEL_ID)
                           .setContentTitle(service.getString(R.string.app_name))
                           .setContentText(service.getString(R.string.notification_text_musi_running))
                           .setSmallIcon(R.drawable.ic_stat_notification)
                           .build();

                   notificationManager.notify(NOTIFICATION_ID_DUMMY, notification);
                   service.startForeground(NOTIFICATION_ID_DUMMY, notification);
                   isShowingDummyNotification = true;

               }
           }

           if (dummyNotificationDisposable != null) {
               dummyNotificationDisposable.dispose();
           }
           dummyNotificationDisposable = Completable.timer(NOTIFICATION_STOP_DELAY, TimeUnit.MILLISECONDS).doOnComplete(() -> removeDummyNotification(service)).subscribe();
       }

[mhammond]

@timusus is there a description of the issue being fixed by dummy notifications in ebbef32cb6446020d9d4616ce89704ed054bf530?

[timusus]

I think people are reading a little too far into the purpose and effect of this dummy notification. It's purely an auxiliary function; it serves to prevent the system from causing an ANR, by ensuring a foreground notification is displayed when the service is running, in certain conditions where there would otherwise not be a notification.

It used to be that Shuttle would wait for 5 minutes of inactivity before shutting down the service and dismissing the notification, but unfortunately Google have made a lot of poorly documented changes in this area, and it seems the system may kill the service pretty quickly (within 2 minutes on my device and maybe quicker on others) once the service is backgrounded. This may result in the notification being dismissed.

I'll have to investigate this one a bit further. Would be interested in steps to reproduce this issue I'd anyone can reliably reproduce it.

@naeemgul90 can you explain what this code is you're 'chaining' and how it solves the problem?

[mhammond]

I also see https://github.com/timusus/Shuttle/issues/445#issuecomment-492308937 - 2 notifications for exactly 12 seconds then both go away. Reverting ebbef32 gives the expected behaviour - 1 notification which persists for many minutes, the same as "play music" does.

Is seeing both notifications expected?

[timusus]

The second ('dummy') notification is basically a hacky workaround for a problem where the system will cause an anr if the service is started, then subsequently stopped, within a short timeframe without displaying a foreground notification. For example, if a 'pause' intent is delivered while the app is already paused, the service will launch to handle the command, discover it has nothing to do, then shut down. Since no notification is displayed, the system ANR's.

But it sounds like what you've observed is a bug probably introduced with these changes, where the playback notification is erroneously dismissed at the same time as the dummy notification.

[mhammond]

Since no notification is displayed, the system ANR's.

Why can't we display the notification in this case?

[timusus]

The service is launched, detects no action to perform, and shuts down. It doesn't make sense to me to display the playback notification in this case.

[mhammond]

Thanks for the explanation. I can't seem to reproduce the "pause intent delivered while paused" scenario - can you recall how to reproduce it?

The service is launched, detects no action to perform, and shuts down. It doesn't make sense to me to display the playback notification in this case.

I see what you mean, but IIUC, instead of showing a playback notification you are showing a "dummy" notification. I don't understand why showing a dummy notification with no functionality for 12 seconds is better than showing the playback notification for 12 seconds. I understand neither is an ideal scenario, just don't yet understand why the dummy is preferred over a functional one.

[timusus]

You have to send the pause intent via ADB to trigger this bug (in my testing). There are other edge cases as well, such as a 'play' command when the user's library is empty. Basically, any intent which launches the service, only for it to discover it doesn't actually need to be running, and shuts itself down without displaying a foreground notification. In Android O, if you start the service but don't display a foreground notification within ~10 seconds, they cause a deliberate ANR, even if that service was shut down.

My thinking was that in these cases where the service isn't needed, there's no need to display a notification to the user. In fact, in the case where the user's library is empty, I don't think it's even possible to show a playback notification, since we have no song to populate it with.

The real design flaw here, is that the service is launched to then determine that the service isn't needed.

I wanted a quick workaround to prevent these ANR's occurring (there were hundreds per week), and I wanted to implement it in a way which had as little impact on the rest of the app as possible. In particular, I didn't want a solution which might erroneously dismiss the playback notification, as this could then cause the system to kill the service (since it's no longer running in the foreground).

This area of the codebase is an absolute nightmare. There's so much legacy code, so intertwined that it's very difficult to debug or refactor. I've actually been rewriting this from the ground up for maybe a year now, which is why I've been quiet on these issues. I hope to eventually drop in a replacement service which a) doesn't involve these issues and b) is easier to navigate and debug.