Opened 21 months ago

Closed 21 months ago

Last modified 21 months ago

#6389 closed defect (fixed)

ABP for Samsung Internet Crashes when closed on specific devices

Reported by: mario Assignee: jwangenheim
Priority: P2 Milestone:
Module: Adblock-Plus-for-Samsung-Browser Keywords:
Cc: Anton, diegocarloslima Blocked By:
Blocking: Platform: Samsung Browser
Ready: yes Confidential: no
Tester: Unknown Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29703555/

Description

Environment

Google Pixel 1 (not occurred on other devices)
ABP for Samsung Internet 1.1.3

How to reproduce

  1. Launch ABP for Samsung Internet
  2. Use the menu + swipe gesture to close ABP

Observed behaviour

ABP either freezes or crashes with the following error log:

02-14 11:04:47.582: E/AndroidRuntime(10588): FATAL EXCEPTION: main
02-14 11:04:47.582: E/AndroidRuntime(10588): Process: org.adblockplus.adblockplussbrowser, PID: 10588
02-14 11:04:47.582: E/AndroidRuntime(10588): java.lang.RuntimeException: Unable to get provider org.adblockplus.sbrowser.contentblocker.ContentBlockerContentProvider: java.lang.IllegalStateException: Not allowed to start service Intent { cmp=org.adblockplus.adblockplussbrowser/org.adblockplus.sbrowser.contentblocker.engine.EngineService }: app is in background uid UidRecord{8456a05 u0a20 SVC  idle change:idle|uncached procs:1 seq(0,0,0)}
02-14 11:04:47.582: E/AndroidRuntime(10588): 	at android.app.ActivityThread.installProvider(ActivityThread.java:6242)
02-14 11:04:47.582: E/AndroidRuntime(10588): 	at android.app.ActivityThread.installContentProviders(ActivityThread.java:5805)
02-14 11:04:47.582: E/AndroidRuntime(10588): 	at android.app.ActivityThread.handleBindApplication(ActivityThread.java:5722)
02-14 11:04:47.582: E/AndroidRuntime(10588): 	at android.app.ActivityThread.-wrap1(Unknown Source:0)
02-14 11:04:47.582: E/AndroidRuntime(10588): 	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1656)
02-14 11:04:47.582: E/AndroidRuntime(10588): 	at android.os.Handler.dispatchMessage(Handler.java:106)
02-14 11:04:47.582: E/AndroidRuntime(10588): 	at android.os.Looper.loop(Looper.java:164)
02-14 11:04:47.582: E/AndroidRuntime(10588): 	at android.app.ActivityThread.main(ActivityThread.java:6494)
02-14 11:04:47.582: E/AndroidRuntime(10588): 	at java.lang.reflect.Method.invoke(Native Method)
02-14 11:04:47.582: E/AndroidRuntime(10588): 	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:438)
02-14 11:04:47.582: E/AndroidRuntime(10588): 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:807)
02-14 11:04:47.582: E/AndroidRuntime(10588): Caused by: java.lang.IllegalStateException: Not allowed to start service Intent { cmp=org.adblockplus.adblockplussbrowser/org.adblockplus.sbrowser.contentblocker.engine.EngineService }: app is in background uid UidRecord{8456a05 u0a20 SVC  idle change:idle|uncached procs:1 seq(0,0,0)}
02-14 11:04:47.582: E/AndroidRuntime(10588): 	at android.app.ContextImpl.startServiceCommon(ContextImpl.java:1521)
02-14 11:04:47.582: E/AndroidRuntime(10588): 	at android.app.ContextImpl.startService(ContextImpl.java:1477)
02-14 11:04:47.582: E/AndroidRuntime(10588): 	at android.content.ContextWrapper.startService(ContextWrapper.java:650)
02-14 11:04:47.582: E/AndroidRuntime(10588): 	at org.adblockplus.sbrowser.contentblocker.ContentBlockerContentProvider.onCreate(Unknown Source:22)
02-14 11:04:47.582: E/AndroidRuntime(10588): 	at android.content.ContentProvider.attachInfo(ContentProvider.java:1917)
02-14 11:04:47.582: E/AndroidRuntime(10588): 	at android.content.ContentProvider.attachInfo(ContentProvider.java:1892)
02-14 11:04:47.582: E/AndroidRuntime(10588): 	at android.app.ActivityThread.installProvider(ActivityThread.java:6239)
02-14 11:04:47.582: E/AndroidRuntime(10588): 	... 10 more

Expected behaviour

ABP doesn't freeze or crash but closes

Change History (13)

comment:1 Changed 21 months ago by jwangenheim

02-14 09:21:17.931  1000   894  2369 I ActivityManager: Killing 6431:org.adblockplus.adblockplussbrowser/u0a20 (adj 1001): remove task
02-14 09:21:18.002  1000   894  2262 W ActivityManager: Scheduling restart of crashed service org.adblockplus.adblockplussbrowser/org.adblockplus.sbrowser.contentblocker.engine.EngineService in 1000ms
02-14 09:21:18.009  1000   894   940 W ActivityManager: setHasOverlayUi called on unknown pid: 6431
02-14 09:21:18.373  1000   894  2262 I ActivityManager: START u0 {act=android.intent.action.MAIN cat=[android.intent.category.HOME] flg=0x10200000 cmp=com.google.android.apps.nexuslauncher/.NexusLauncherActivity} from uid 10042
02-14 09:21:18.620  1000   894  2262 I SoundTriggerHelper: startRecognition successful.
02-14 09:21:19.014  1000   894   919 I ActivityManager: Start proc 6482:org.adblockplus.adblockplussbrowser/u0a20 for service org.adblockplus.adblockplussbrowser/org.adblockplus.sbrowser.contentblocker.engine.EngineService
02-14 09:21:19.041  1000   894  2362 W ActivityManager: Stopping service due to app idle: u0a20 -8s866ms org.adblockplus.adblockplussbrowser/org.adblockplus.sbrowser.contentblocker.engine.EngineService

The crash seems to be related to the fact that we currently start the service with START_STICKY. That means the system tries to create a fresh copy of the service when it was terminated before the task was finished. The above crashlog shows that right after the app was killed manually, a restart is scheduled. When attempting to restart the service, the app crashes with a IllegalStateException.
I'll set the EngineService to Start_NOT_STICKY and send a new test APK to @rscott.

comment:2 Changed 21 months ago by rscott

  • Verified working set

Fixed in adblockplussbrowser-1.1.3-release-signed-aligned-4.apk. Great stuff!

comment:3 Changed 21 months ago by jwangenheim

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:4 Changed 21 months ago by asmirnov

Why it happens on Google pixel 1 and does not happen on other devices?

comment:5 Changed 21 months ago by jwangenheim

To improve the user experience, Android 8.0 (API level 26) imposes limitations on what apps can do while running in the background:
https://developer.android.com/about/versions/oreo/background.html#services

This exact crash can only happen on devices with at least Android 8.0, as the system will try to re-create the service without having the permission to do it.
I don't know how many other Android 8 devices were tested. I was not able to reproduce the error on a OnePlus5.

comment:6 Changed 21 months ago by jwangenheim

@anton:
Rick said he also testend on other Android 8 devices but the bug only appeared on the Pixel 1. With the fix the crash did not appear anymore and everything else worked as expected on all tested devices.
As there were no problems during the tests, I don't see a reason why we should not implement the fix.

comment:7 Changed 21 months ago by jwangenheim

  • Cc Anton diegocarloslima added

comment:8 Changed 21 months ago by diegocarloslima

I understand @anton's concern about that change. It's a small change that could potentially have undesired side effects. However, for our particular project, I think that this change is fine (but we should do QA to confirm that anyways). Our EngineService is started in many different scenarios:

  • When the user opens our app
  • When Samsung Internet requests our filters file through our ContentBlockerContentProvider
  • When ConnectivityChanged broadcast receiver is triggered

With that fix, the only change on behavior that occurs to me is that our app might stop checking for updates when the user intentionally kills the app. But once he goes through any of the scenarios above, the update check will resume. Anyway, I think that its worth to open a follow up issue in order to refactor our project to start using JobScheduler to check for updates.

comment:9 Changed 21 months ago by jwangenheim

I have created #6414 as a follow up ticket

Last edited 21 months ago by jwangenheim (previous) (diff)

comment:11 Changed 21 months ago by jwangenheim

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:12 Changed 21 months ago by jwangenheim

  • Ready set

comment:13 Changed 21 months ago by diegocarloslima

  • Owner set to jwangenheim
  • Priority changed from Unknown to P2
Note: See TracTickets for help on using tickets.