1465102 - Android O NotificationService startForeground changes

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Vlad Baicu]

On Android O you can no longer call startService if the app is in background, use startForegroundService with a following call to startForeground.

Comment 1

[Vlad Baicu]

Attachment: Bug 1465102 - Updated NotificationService for Oreo.

Using stopService instead of calling startService with a null notification in order to stop NotificationService.

Review commit: https://reviewboard.mozilla.org/r/247654/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/247654/

Comment 2

[Jan Henning [:JanH]]

Comment on attachment 8981555 [details]
Bug 1465102 - Updated NotificationService for Oreo.

https://reviewboard.mozilla.org/r/247654/#review253702

The basic idea of the patch is absolutely fine, but the issues below need fixing before this can land.

::: commit-message-f01bb:3
(Diff revision 1)
> +Bug 1465102 - Updated NotificationService for Oreo. r?sdaswani
> +
> +Using stopService instead of calling startService with a null notification in order to stop NotificationService.

Not a terrible issue, but for the future, can you please add line breaks to the extended commit message so as to stay at or below 80 characters per line? I've been guilty of this myself for a long time, but I've since noticed that not everything handling commit messages will automatically break overly long lines.

The only exception is the first line, since fitting bug number, reviewer(s) and a reasonable summary in 80 characters or less would be a bit challenging, so exceeding the limit there a little is nothing to worry about.

::: commit-message-f01bb:3
(Diff revision 1)
> +Bug 1465102 - Updated NotificationService for Oreo. r?sdaswani
> +
> +Using stopService instead of calling startService with a null notification in order to stop NotificationService.

Can you expand on the reasoning a little more, like e.g. adding "That way, starting the service will always result in a call to startForeground, meaning that startForegroundService on Android O can be used without risk."

::: mobile/android/base/java/org/mozilla/gecko/notifications/NotificationClient.java:304
(Diff revision 1)
>          }
>          return false;
>      }
>  
> +    @SuppressLint("NewApi")
>      private void setForegroundNotificationLocked(final String name,

Since we've now got a special method for getting rid of the notification again and the NotificationService now assumes it can always get a notification from the intent, can you make both parameters @NonNull please?

::: mobile/android/base/java/org/mozilla/gecko/notifications/NotificationClient.java:317
(Diff revision 1)
> +        } else {
> +            mContext.startForegroundService(intent);
> +        }
>      }
>  
> +    private void setForegroundNotificationClosed() {

`removeForegroundNotification` might be more succint?

Also, because you're going to be touching `mForegroundNotification` (see below), I think you should add a "locked" suffix to indicate that this method should be called only from within a synchronised method/block, so make it `removeForegroundNotificationLocked`, please.

::: mobile/android/base/java/org/mozilla/gecko/notifications/NotificationClient.java:318
(Diff revision 1)
> +            mContext.startForegroundService(intent);
> +        }
>      }
>  
> +    private void setForegroundNotificationClosed() {
> +        mContext.stopService(new Intent(mContext, NotificationService.class));

To fully replicate the effect of a `setForegroundNotificationLocked(null, null)` call, you also need to clear mForegroundNotification here.

Comment 3

[Vlad Baicu]

Attachment: Bug 1465102 - Updated NotificationService for Oreo.

Changed NotificationClient to use stopService instead of calling startService
with a null notification in order to stop NotificationService.
This way, we always have a guaranteed call to startForeground in our service,
abiding by Android Oreo regulations.

Review commit: https://reviewboard.mozilla.org/r/247938/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/247938/

Comment 4

[Jan Henning [:JanH]]

Comment on attachment 8981915 [details]
Bug 1465102 - Updated NotificationService for Oreo.

https://reviewboard.mozilla.org/r/247938/#review254368

Looks good, thanks.

::: commit-message-f01bb:8
(Diff revision 1)
> +Changed NotificationClient to use stopService instead of calling startService
> +with a null notification in order to stop NotificationService.
> +This way, we always have a guaranteed call to startForeground in our service,
> +abiding by Android Oreo regulations.
> +
> +MozReview-Commit-ID: 4CzM4pvANJt

I'm not sure what exactly you're doing, but can you please try *amending/histediting* the original commit and most importantly, try keeping the MozReview-Commit-ID intact?

Whatever you're doing right now leads Mozreview into thinking you've completely abandoned the original commit series, which means that instead of *adding* a new revision for an existing review request you're opening a completely fresh review.

This means that all the issues and review comments opened for the previous revision will be lost in Mozreview (well, not completely lost, but they're no longer linked to the current review request) and the interdiff facility doesn't work either.

::: mobile/android/base/java/org/mozilla/gecko/notifications/NotificationClient.java:308
(Diff revision 1)
> +                                                 @NonNull final Notification notification) {
>          mForegroundNotification = name;
>  
>          final Intent intent = new Intent(mContext, NotificationService.class);
>          intent.putExtra(NotificationService.EXTRA_NOTIFICATION, notification);
> +        if (AppConstants.Versions.preO) {

Please don't forget that you either
- need to wait for bug 1444776 to land first
- or else add this in AppConstants.java.in yourself

::: mobile/android/base/java/org/mozilla/gecko/notifications/NotificationClient.java:320
(Diff revision 1)
> +    private void removeForegroundNotificationLocked() {
> +        mForegroundNotification = null;
> +        mContext.stopService(new Intent(mContext, NotificationService.class));
> +    }
> +
> +

Nit: Additional unnecessary newline.

Comment 5

[Vlad Baicu]

I'm gonna sync with Andrei and ask for this to get landed after his one does. Thanks

Comment 6

[Jan Henning [:JanH]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0581ff7ccfefa35ae2efdfd08a807ef5adde0abc
Bug 1465102 - Updated NotificationService for Oreo. r=JanH

Comment 7

[Jan Henning [:JanH]]

Fixed the extraneous newline and pushed it for you, so you can close the review in Mozreview as submitted.

Comment 8

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/0581ff7ccfef

Comment 9

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/b73265d537e6
Backed out changeset 0581ff7ccfef because of a possible framework issue as request by vladbaicu. a=backout

Comment 10

[Vlad Baicu]

From what I gathered during investigation and testing I can see two possible cases where this might crash in our case. We either call stopService before startForeground has a chance to get called or startForeground simply does not get called within the timer allocated by Android. 

I can think of two possible designs of how we can avoid this:

- make the service a bound service and use startForeground/stopForeground accordingly

- implement queueing logic and check whether our service has actually called startForeground

I think that the second option is less elegant than the first one, Jan what do you think about this ?

Comment 11

[Jan Henning [:JanH]]

(In reply to Vlad Baicu from comment #10)
> - make the service a bound service and use startForeground/stopForeground
> accordingly

From what I remember from the attempt to use a bound service construct in bug 1384866, isn't it a problem that startForeground/stopForeground only works for services that have actually been started (as opposed to merely having been bound to), so we'd still have to start and stop the service as well in order to show a foreground notification?
Plus binding to a service is async, too, so we'd still have to track the current state so as to know what to do if multiple events arrive in short succession.

So please correct me if I'm wrong here, but I think we'd need some sort of queuing logic regardless of which way the service was launched, and since we still need to actually *start* the service in order to show the foreground notification, it's easier to just stick to that and let the NotificationClient/Service keep track among themselves when it is safe to stop the service again.

Comment 12

[Vlad Baicu]

Comment on attachment 8981915 [details]
Bug 1465102 - Updated NotificationService for Oreo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247938/diff/1-2/

Comment 13

[Vlad Baicu]

Updated the patch with a fix, I think it's cleaner to take advantage of the start service calls but with different intents instead of making our own logic for queueing.

Comment 14

[Vlad Baicu]

Comment on attachment 8981915 [details]
Bug 1465102 - Updated NotificationService for Oreo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247938/diff/2-3/

Comment 15

[Jan Henning [:JanH]]

Comment on attachment 8981915 [details]
Bug 1465102 - Updated NotificationService for Oreo.

https://reviewboard.mozilla.org/r/247938/#review262154

Comment 16

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1465102 - Updated NotificationService for Oreo.

Modified NotificationClient to use new start/stop intents for NotificationService for
Android O. We are keeping a reference to the notification the service was first
started with in order to comply with the mandatory startForeground call for Oreo.

Review commit: https://reviewboard.mozilla.org/r/256946/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256946/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1022b1a54372
Updated NotificationService for Oreo. r=JanH

Comment 18

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/1022b1a54372