App on Android reloads itself when notification arrives in standalone app

baronchng commented 4 years ago

🐛 Bug Report

Summary of Issue

When user is on app foreground, app will reload by itself when notification arrives. It happens only in standalone app, does not happen using Expo Client. It does NOT happen when the app is in background. It does NOT happen on iOS, only happens on Android (tested on a few devices). No errors were captured.

Environment - output of `expo diagnostics` & the platform(s) you're targeting

Expo CLI 3.28.2 environment info:
    System:
      OS: macOS 10.15.7
      Shell: 5.7.1 - /bin/zsh
    Binaries:
      Node: 14.15.0 - /usr/local/opt/node@14/bin/node
      Yarn: 1.22.4 - /usr/local/bin/yarn
      npm: 6.14.8 - /usr/local/opt/node@14/bin/npm
      Watchman: 4.9.0 - /usr/local/bin/watchman
    SDKs:
      iOS SDK:
        Platforms: iOS 12.4, macOS 10.14, tvOS 12.4, watchOS 5.3
      Android SDK:
        API Levels: 28, 29
        Build Tools: 28.0.3, 29.0.2, 30.0.0
        System Images: android-21 | Google APIs ARM EABI v7a, android-21 | Google APIs Intel x86 Atom, android-29 | Google APIs Intel x86 Atom
    IDEs:
      Android Studio: 3.6 AI-192.7142.36.36.6392135
      Xcode: 10.3/10G8 - /usr/bin/xcodebuild
    npmPackages:
      expo: ^39.0.0 => 39.0.3 
      react: 16.13.1 => 16.13.1 
      react-dom: 16.13.1 => 16.13.1 
      react-native: https://github.com/expo/react-native/archive/sdk-39.0.3.tar.gz => 0.63.2 
      react-native-web: ~0.13.7 => 0.13.18 
    Expo Workflow: managed

Platform: Android

Reproducible Demo

if (Platform.OS === 'android') {
        Notifications.setNotificationChannelAsync('default', {
          name: 'default',
          importance: Notifications.AndroidImportance.MAX,
        });
      }

const pushToken = await Notifications.getExpoPushTokenAsync();

React.useEffect(() => {
    const handleNotificationReceived = (
      notification: Notifications.Notification
    ) => {
      const data = notification.request.content.data; // payload

      // app in foreground
      if (appState === 'active') {
        // do things here
      }
    };

     const notificationReceivedSubscription = Notifications.addNotificationReceivedListener(
       handleNotificationReceived
     );

    return () => {
      notificationReceivedSubscription.remove();
    };
  }, [appState]);

Steps to Reproduce

Setup a listener on notification received.
Open standalone android app, leave it in foreground.
Send a notification to the Expo Push Token obtained from Notifications.getExpoPushTokenAsync().
Android app reloads itself (crashes? but no error reported to Bugsnag).

Expected Behavior vs Actual Behavior

Expected: Android App is expected to receive notification without crashing/reloading app.

Actual: Android App reloads itself whenever it receives notification in the foreground with listener set.

Extras

This problem was not happening in SDK 37, SDK 38. It only started happening in SDK 39.
When this project is upgraded from SDK 37 -> SDK 39, and without changing any of the legacy code using import { Notifications } from 'expo', it already reloads/crashes.
After switching from expo to expo-notifications, it still reloads/crashes.
I also tried commenting out the listener code segment, and then the app stopped reloading/crashing. The app can still receive notification. I can still see the notification going into notification tray.

cruzach commented 4 years ago

thanks for the report! I built an APK on sdk 39 using the code from this snack - https://snack.expo.io/@charliecruzan/10894 - and wasn't able to reproduce any reloading when receiving push notifications with the app foregrounded (we also haven't had any other reports of this behavior). This means it's probably as issue in your code, so hopefully that snack helps you to debug a little bit more. If you're specifically handling things when your app is foregrounded (which it looks like you are doing from your code if (appState === 'active') {), then I would start my investigation there if I were you

Btw- make sure you've set android.useNextNotificationsApi to true in your app.json

baronchng commented 4 years ago

Thanks @cruzach! You're right. I tested and finally found the culprit to be Vibration.vibrate(). I am trying to push a new app bundle to Play Store with permission "VIBRATE" and see it that fixes the problem. It seems like SDK 39 didn't add in the permission automatically but this is a totally different issue than what is posted here. Do you know if this is an expected behaviour?

I will come back to close this issue after I confirmed the fix in a few days.

cruzach commented 4 years ago

Yeah, SDK 39 saw some changes to android permissions. You can read about that in our release notes- https://blog.expo.io/expo-sdk-39-is-now-available-4c10aa825e3f

We also did a full audit of how we handle native Android permissions as part of our bug bash. With the help of your feedback, we updated our permissions documentation to better communicate each module’s required and optional permissions. We also added more context around the behavior of permissions on different platforms.

baronchng commented 4 years ago

I confirmed the issue to be caused by missing permission VIBRATE as Vibration.vibrate() was included in part of the workflow mentioned above. Thanks again @cruzach for the prompt response.

I have some feedbacks on the documentation though. Permission Documentation My apologies as this is not directly related to the issue opened here.

https://forums.expo.io/t/how-to-decrease-permission-on-android/43525/4 This could be added to documentation to inform that upgrading to SDK 39 will auto add ACCESS_BACKGROUND_LOCATION. This has caused my app to be rejected by PlayStore as I do not use background location service.

https://github.com/expo/expo/issues/10206#issuecomment-700967220 Documentation could add in what is included in the default permission list, and what is not included when permissions key is specified in app.json. This is because when I wanted to remove ACCESS_BACKGROUND_LOCATION from the issue mentioned above, it also removes the CAMERA, RECORD_AUDIO, etc. permissions that are crucial to app flow.

https://github.com/expo/expo-cli/blob/master/packages/xdl/src/detach/AndroidShellApp.js#L919 This code segment is actually pretty good in determining what is needed to be included in the permissions. I think the information should be updated in the documentation too.

cruzach commented 4 years ago

Regarding ACCESS_BACKGROUND_LOCATION- we document that all permissions are included if you don't specify an android.permissions key, and document exactly how to limit permissions.

We do document which permissions are included by default (i.e. if you don't specify android.permissions) here- https://docs.expo.io/versions/v39.0.0/config/app/#permissions

baronchng commented 4 years ago

In https://docs.expo.io/versions/v39.0.0/config/app/#permissions, I can't find what permissions are included by default. It seems like it's not listed there. However, it does list down the options of permission we can specify in the permissions array though.

As for ACCESS_BACKGROUND_LOCATION, will it be added by default? This is because from this link https://forums.expo.io/t/how-to-decrease-permission-on-android/43525/6, the answer seems to say ACCESS_BACKGROUND_LOCATION will not be added by default.

Additionally, https://github.com/expo/expo/blob/sdk-39/template-files/android/AndroidManifest.xml#L11-L40 also does not include ACCESS_BACKGROUND_LOCATION.

However, I just did a expo build:android, downloaded the .aab and investigated the AndroidManifest.xml. The .aab did include ACCESS_BACKGROUND_LOCATION. For this build, I did not specify any permissions key in app.json.

cruzach commented 4 years ago

this list is included in that link:

ACCESS_COARSE_LOCATION
ACCESS_FINE_LOCATION
ACCESS_BACKGROUND_LOCATION
CAMERA
RECORD_AUDIO
READ_CONTACTS
WRITE_CONTACTS
READ_CALENDAR
WRITE_CALENDAR
READ_EXTERNAL_STORAGE
WRITE_EXTERNAL_STORAGE
USE_FINGERPRINT
USE_BIOMETRIC
WRITE_SETTINGS
VIBRATE
READ_PHONE_STATE
com.anddoes.launcher.permission.UPDATE_COUNT
com.android.launcher.permission.INSTALL_SHORTCUT
com.google.android.c2dm.permission.RECEIVE
com.google.android.gms.permission.ACTIVITY_RECOGNITION
com.google.android.providers.gsf.permission.READ_GSERVICES
com.htc.launcher.permission.READ_SETTINGS
com.htc.launcher.permission.UPDATE_SHORTCUT
com.majeur.launcher.permission.UPDATE_BADGE
com.sec.android.provider.badge.permission.READ
com.sec.android.provider.badge.permission.WRITE
com.sonyericsson.home.permission.BROADCAST_BADGE

ACCESS_BACKGROUND_LOCATION will be included if you don't specify an android.permissions key. (In most cases, users should specify an android.permissions key

expo / expo