Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Random crashing behavior on Android, and crashing after system restarts all apps #2446

Open
leaf-node opened this issue Nov 7, 2024 · 16 comments · May be fixed by #2481
Open

Random crashing behavior on Android, and crashing after system restarts all apps #2446

leaf-node opened this issue Nov 7, 2024 · 16 comments · May be fixed by #2481

Comments

@leaf-node
Copy link

Describe the bug

App randomly crashes every few hours, or crashes when trying to start it after the app is stopped by the system following app optimization. My app is using a foreground service notification that at least some of the time may be persisting when the app and its background process gets closed by the user.

Tested on Android 15 with 34 target SDK with flutter_local_notifications 17.2.4.

To Reproduce

  1. Upgrade GrapheneOS version
  2. Reboot phone
  3. Wait for apps to optimize
  4. Press GrapheneOS notification to restart apps
  5. Start app manually
  6. It crashes

Crashing also occurs randomly every few hours. I started noticing this issue in the last few days, about a week after I upgraded dependencies for my app, including this module from version 17.2.3 to 17.2.4.

Expected behavior

The app shouldn't crash.

Sample code to reproduce the problem

I don't have source code available for this yet, hoping that it's an easy bug to solve. If not, please let me know, and I'll consider creating a basic app with this problem. Thanks!

** Crash Logs **

type: crash
osVersion: google/shiba/shiba:15/AP3A.241005.015/2024110400:user/release-keys
flags: dev options enabled
package: com.example.foo:1, targetSdk 34
process: com.example.foo
processUptime: 92 + 323 ms

java.lang.RuntimeException: Unable to start service com.dexterous.flutterlocalnotifications.ForegroundService@c4128db with null: java.lang.NullPointerException: Attempt to invoke virtual method 'java.io.Serializable android.content.Intent.getSerializableExtra(java.lang.String, java.lang.Class)' on a null object reference
at android.app.ActivityThread.handleServiceArgs(ActivityThread.java:5199)
at android.app.ActivityThread.-$$Nest$mhandleServiceArgs(Unknown Source:0)
at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2478)
at android.os.Handler.dispatchMessage(Handler.java:107)
at android.os.Looper.loopOnce(Looper.java:232)
at android.os.Looper.loop(Looper.java:317)
at android.app.ActivityThread.main(ActivityThread.java:8744)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:580)
at com.android.internal.os.ExecInit.main(ExecInit.java:50)
at com.android.internal.os.RuntimeInit.nativeFinishInit(Native Method)
at com.android.internal.os.RuntimeInit.main(RuntimeInit.java:369)
Caused by: java.lang.NullPointerException: Attempt to invoke virtual method 'java.io.Serializable android.content.Intent.getSerializableExtra(java.lang.String, java.lang.Class)' on a null object reference
at com.dexterous.flutterlocalnotifications.b1.a(SourceFile:1)
at com.dexterous.flutterlocalnotifications.ForegroundService.onStartCommand(Unknown Source:10)
at android.app.ActivityThread.handleServiceArgs(ActivityThread.java:5181)
... 11 more
@leaf-node
Copy link
Author

Using version 18.0.0 of flutter_local_notifications seems to prevent the random crashes that were also occurring with similar stack traces. Hopefully this also resolves the issues with starting the app after automatic app restarts.

@leaf-node
Copy link
Author

I'm still getting this on version 18.0.0.

@leaf-node
Copy link
Author

I'm also seeing this on the 17.2.3 version of the library, but I don't recall this error before, so it's possible that the crashing behavior could be due to a bug in my code.

@leaf-node leaf-node changed the title Upgrading from version 17.2.3 to 17.2.4 may have introduced crashing behavior Random crashing behavior on Android, and crashing after system restarts all apps Nov 10, 2024
@leaf-node
Copy link
Author

I believe that this is where the error is occuring:

https://github.com/MaikuB/flutter_local_notifications/blob/master/flutter_local_notifications/android/src/main/java/com/dexterous/flutterlocalnotifications/ForegroundService.java#L21

I wonder if the ForegroundServiceStartParameter class' static variables are not initialized before they are accessed? One possible reason for that is that there could be a circular dependency between classes, and so initialization order is not guaranteed. However, a I didn't see any circular dependencies after a brief check.

Also, another user of my app reports no crashing behavior, while I was seeing a lot of crashes for a while.

@Levi-Lesches
Copy link
Contributor

Levi-Lesches commented Nov 26, 2024

Judging by the error, it's not saying the parameters are null, which would be EXTRA and .class like you're suggesting, but rather that the code is trying to call android.content.Intent.getSerializableExtra on a null reference.

So the intent itself is null. This is also indicated by Unable to start service ... with null, meaning the intent passed to the service is null. The plugin code doesn't have any checks for a null intent and crashes when trying to use it.

A quick look at the Android Services docs shows that onStartCommand must return with specific values. In this case, we're interested in START_STICKY:

If the system kills the service after onStartCommand() returns, recreate the service and call onStartCommand(), but do not redeliver the last intent. Instead, the system calls onStartCommand() with a null intent unless there are pending intents to start the service. In that case, those intents are delivered. This is suitable for media players (or similar services) that are not executing commands but are running indefinitely and waiting for a job.

You can get yourself into this situation with the plugin if you call plugin.startForegroundService() and leave startType to AndroidServiceStartType.startSticky, which is the default. The docs of this plugin don't warn against using any particular value, but they do warn about some setup you'll need to do.

@leaf-node, can you confirm that you're using this function with startSticky, added the relevant sections to your AndroidManifest.xml, and are still getting this issue? It seems to be in line with your description of the system killing your service and then restarting it. Perhaps you'd want to use startNotSticky or startRedeliverIntent instead.

@EP-u-NW, @EPNW-Eric, since this was your PR, do you have any insight here?

@MaikuB It seems if the plugin is going to endorse using startSticky as an option, it should also check for a null intent in the code. It seems #2298 has the same issue as well. I'm not sure what the app should even do in the case of a null intent, and perhaps it's better to remove that option from the Dart API instead.

Here is the relevant native code:

public int onStartCommand(Intent intent, int flags, int startId) {
ForegroundServiceStartParameter parameter;
if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.TIRAMISU) {
parameter =
(ForegroundServiceStartParameter)
intent.getSerializableExtra(
ForegroundServiceStartParameter.EXTRA, ForegroundServiceStartParameter.class);
} else {
parameter =
(ForegroundServiceStartParameter)
intent.getSerializableExtra(ForegroundServiceStartParameter.EXTRA);
}

@EPNW-Eric
Copy link

@Levi-Lesches thanks for the good issue analysis and brining this to my attention!

I've already looked into it provide a write up tomorrow.

@EPNW-Eric
Copy link

My reason for needing a foreground service was audio recording and processing using dart. By starting a foreground service the process is treated as in foreground (even if not visible) and my dart code continued running.

I think the best solution would be using START_REDELIVER_INTENT as default and warning about START_STICKY. Also, on second thought START_STICKY makes only sense if the services would actualy do something, but I guess most flutter developers only use the foreground service like me to just keep the process in foreground.

As a sidenote, while I was thinking about the bug today I considered storing the start parameters in a static variable instead of an intent, but I then realized, that when the process is terminated by the system while under load, the variable would also lose its value, so this is not a solution.

About wheter to remove the foreground feature or not this is my opinion: The fact that this bug surfaced shows that people are using the feature. Foreground services are tightly connected to notifications. Moving foreground services in to a new package would also mean that the new package would need to redo a lot of the notification related things this package already solves.

If we want to keep the feature I can provide a PR until Friday (but feel free to do it yourself if you can get to it earlier).

@Levi-Lesches
Copy link
Contributor

Right, to clarify, I don't think anyone wants to remove the feature. If anything, I was suggesting removing or at least deprecating the start sticky option. Seems like you agree that flag isn't inherently useful.

In case we do keep it, what should we do on a null intent? Just return and quit? It seems the startup code you wrote is very dependent on there being an intent with some data, so I'm not sure what useful value the plugin has if there is no intent.

If the solution is as simple as returning if null and deprecating the flag, I can take care of it. If there's something else that should be done, a PR would be great.

@leaf-node
Copy link
Author

leaf-node commented Nov 26, 2024

Thanks for both of your help with this. : ) I am using https://github.com/AhsanSarwar45/flutter_boot_receiver to restart the app if the OS reboots, and it may eventually also handle restarting after app upgrades.

I see that I can specify the service start type here: https://pub.dev/documentation/flutter_local_notifications/latest/flutter_local_notifications/AndroidFlutterLocalNotificationsPlugin/startForegroundService.html, but it's not yet clear to me how to update AndroidManifest.xml this information. I'll keep doing research on that.

Edit: Oh, I see that the changes to the AndroidManifest.xml are only somewhat related.

Here is the relevant section of my AndroidManifest.xml:

        <service
            android:name="com.dexterous.flutterlocalnotifications.ForegroundService"
            android:exported="false"
            android:stopWithTask="false"
            android:foregroundServiceType="dataSync" />
        <service
            android:name="com.flux.flutter_boot_receiver.BootHandlerService"
            android:exported="false"
            android:permission="android.permission.BIND_JOB_SERVICE" />
        <receiver
            android:enabled="true"
            android:exported="true"
            android:name="com.flux.flutter_boot_receiver.BootBroadcastReceiver"
            android:permission="android.permission.RECEIVE_BOOT_COMPLETED">
            <intent-filter>
                <action android:name="android.intent.action.BOOT_COMPLETED" />
                <category android:name="android.intent.category.DEFAULT" />
            </intent-filter>
        </receiver>
    </application>
    <uses-permission android:name="android.permission.FOREGROUND_SERVICE"/>
    <uses-permission android:name="android.permission.FOREGROUND_SERVICE_DATA_SYNC"/>
    <uses-permission android:name="android.permission.RECEIVE_BOOT_COMPLETED"/>
    <uses-permission android:name="android.permission.WAKE_LOCK"/>
    <uses-permission android:name="android.permission.INTERNET"/>

@leaf-node
Copy link
Author

I should note that I tried wiping and uninstalling the app, and the crashing stopped. So hopefully I can reproduce the bug on a new build the next time GrapheneOS has an update for my phone.

I wonder if there is an interplay between app optimizations after a phone reboot on the one hand, and the fact that flutter_boot_receiver doesn't actually handle restarting the app after app upgrades yet? It's just odd that it keeps happening after that condition occurs.

@EPNW-Eric
Copy link

I'm not sure if I would remove the flag, just because I don't see a usecase yet, doesn't mean some dev out there is actually benefiting from it...

I'm not even sure if I would change something in native code. Yes, the app crashes but maybe thats a good thing: Imagine if it does not crash and just returns; users would not reach out to developers because from their perspective, the app would work "fine". Also developers investigating the reason why sometimes there seems to be no foreground service in their app would have no clue where to start since there is no kind logging.

If we were about to do anything in native code, an idea might be to throw an even more explicit exception instead of a null pointer:

throw new 
IllegalStateException("The intent carrying the notification data is null, most likley beause the service start type was START_STICKY.\r\nConsider using START_REDELIVER_INTENT and see https://github.com/MaikuB/flutter_local_notifications/issues/2446 for more details.");

Nethertheless we should we warn about START_STICKY in the documentation and make START_REDELIVER_INTENT the default.

@leaf-node Hm an interplay with optimizers is an interesting thought. I never actually came across a situation where a system under load terminates an app with a foreground service. Maybe optimizers are way more strict here and start "resource saving" earlyer.

@leaf-node
Copy link
Author

I tested an older version of my app with the app optimization, and persistent crashing returned. Sideloading an updated app with the recommended change, using START_REDELIVER_INTENT, among other changes, caused the crashing to stop. I don't know if that is a complete test, so I'll see what happens after the next upgrade and app optimization pass.

Also, GrapheneOS does app optimization before the phone unlocks, and more after the screen is unlocked. It's this second round that highlighted problems for me. There have been no issues during optimization, but at the end of the second round, a notification appears that basically says to "tap here to automatically restart open apps", and that's when this bug was activated.

@EPNW-Eric
Copy link

@leaf-node thanks for the report! Could you do some testing with START_NOT_STICKY? Since we are currently discussing in the PR if we should remove the other start options I wonder if you are able to achieve your desired behavior with this?

@leaf-node
Copy link
Author

Yes, I can test out START_NOT_STICKY at some point and see if it works for me. My hunch is that this will not be something that my app needs, if the START_REDELIVER_INTENT (and / or START_STICKY) is the only way of resuming an interrupted foreground service. (For starting after boot, I am using a package for that, and may use my own code to replace it).

As for START_STICKY, it could also work, right? It may also be appropriate for my case, because i don't need to deliver any information about the reason for the previous foreground service to the new one. I think that the looping behavior was because the method affected by it was crashing when it didn't know what to do with null. If it returned any of the three values (most likely START_STICKY), would it work then too, right?

@Levi-Lesches
Copy link
Contributor

Levi-Lesches commented Dec 3, 2024

We could theoretically make start sticky work by just returning if the intent is null. But the question we ran into is, fundamentally, what work is being done that the notification should be reinstated?

Other packages allow you to link Dart code to your foreground service, but this plugin just makes notification and that's it. So if the foreground service is sticky, the user will get notification again, but no actual logic will be running to justify it. For that reason we are thinking of removing the start type option entirely and only use start not sticky.

In other words, say you're using this plugin to start a foreground service because you're downloading some files. The download is only happening in your dart code, not in the service. Well having a service makes it less likely your app will be killed, but if it is, and the system reinstates it because it's sticky, there's no way for the Java code to restart the download operation that's happening in Dart.

There are other packages that actually let you pass a function when creating a foreground service, and that function is actually tied to the service such that it will be restarted as well. But that's a bit out of scope for this plugin now. See #2481, where I removed the option and update the documentation instead

@leaf-node
Copy link
Author

Using START_NOT_STICKY does not lead to any crashes for me after apps are optimized in GrapheneOS and then restarted automatically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants