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

NPE on flushBufferToDisk #513

Closed
rostopira opened this issue Apr 26, 2018 · 13 comments · Fixed by #545
Closed

NPE on flushBufferToDisk #513

rostopira opened this issue Apr 26, 2018 · 13 comments · Fixed by #545

Comments

@rostopira
Copy link

rostopira commented Apr 26, 2018

Description:
20% of devices on new release experience critical issue
Reports error log:

Exception java.lang.NullPointerException: Attempt to invoke interface method 'android.content.SharedPreferences$Editor android.content.SharedPreferences.edit()' on a null object reference
com.onesignal.OneSignalPrefs$WritePrefHandlerThread.flushBufferToDisk (OneSignalPrefs.java)
com.onesignal.OneSignalPrefs$WritePrefHandlerThread.access$000 (OneSignalPrefs.java)
com.onesignal.OneSignalPrefs$WritePrefHandlerThread$1.run (OneSignalPrefs.java)
android.os.Handler.handleCallback (Handler.java:751)
android.os.Handler.dispatchMessage (Handler.java:95)
android.os.Looper.loop (Looper.java:154)
android.os.HandlerThread.run (HandlerThread.java:61)

Environment

  1. Android SDK 27
  2. Crash occurs on different devices (Samsung, Motorola, ZTE and others) on all API levels that we support (from Jelly Bean to Oreo)
  3. OneSignal SDK 3.8.4 (same crash were on 3.8.3 as well)
  4. Issue wasn't present in 3.8.2

Steps to Reproduce Issue:
Unknown

@jkasten2
Copy link
Member

@rostopira Thanks for the details. Could you share which OneSignal SDK methods you are calling? Are your calls to OneSignal on the main thread? Or are some on a background thread? We haven't seen other reports of this crash so it might be an race condition with this.

@rostopira
Copy link
Author

Here is code, that is used in my app

At the Application onCreate:

OneSignal.setLocationShared(false)
    OneSignal
        .startInit(this)
        .setNotificationOpenedHandler {
            if (it.notification.payload.collapseId?.contains("chat") == true)
                startActivity(newIntent<ChatActivity>(Intent.FLAG_ACTIVITY_NEW_TASK))
            else {
                if (!it.notification.payload.launchURL.isNullOrBlank()) {
                    val intent = Intent(Intent.ACTION_VIEW, Uri.parse(it.notification.payload.launchURL))
                    intent.flags = Intent.FLAG_ACTIVITY_NEW_TASK
                    startActivity(intent)
                } else
                    startActivity(newIntent<SplashScreen>(Intent.FLAG_ACTIVITY_NEW_TASK))
            }
        }
        .init()
    OneSignal.setInFocusDisplaying(OneSignal.OSInFocusDisplayOption.None)
    OneSignal.idsAvailable { osId, _ ->
        val kgl = this@initOneSignal
        kgl.user.pushId = osId
        with(kgl.defaultSharedPreferences) {
            if (getString("oneSignalId", null) != osId) {
                edit().putString("oneSignalId", osId).apply()
                Command(ID_UPDATE, osId).send()
            }
        }
    }

NotificationExtenderService:

class PushProcessor: NotificationExtenderService() {
    override fun onNotificationProcessing(notification: OSNotificationReceivedResult): Boolean {
        val cp = notification.payload?.collapseId
        if (cp?.contains("promo") == true) {
            val sp = defaultSharedPreferences
            if (sp.getBoolean(cp, false))
                return true
            sp.set(cp to true)
            return UserData.appHidden
        }
        val addData = notification.payload?.additionalData ?: return UserData.appHidden
        return (notification.payload?.collapseId?.contains("chat") == true && ChatActivity.opened)
                || processPush(addData, this) || UserData.appHidden
    }
}

private fun processPush(data: JSONObject, ctx: Service): Boolean {
    try {
        val id = data.getString("id") ?: return false
        val type = data.getInt("t")
        val value = data.getString("v") ?: ""
        if (type == Pushable.MESSAGE && ChatActivity.opened) return true
        if (id != "web" && !ctx.kgl.family.contains(id))
            ctx.kgl.refresh()
        if (type<10) {
            val n = PushNotification(id, type, value)
            if (data.has("ts")) {
                n.ts = data.getLong("ts")
                if (n.ts != 0L && timestamp()-n.ts > DateUtils.HOUR_IN_MILLIS * 2)
                    return true
                if (type == Pushable.SAFE || type == Pushable.DANGER) {
                    val sps = ctx.getSharedPreferences("spsts", Context.MODE_PRIVATE)
                    if (sps.getLong(id, 0L) > n.ts)
                        return true
                    sps.edit().putLong(id, n.ts).apply()
                }
            }
            n.show(ctx)
        } else {
            if (BuildConfig.DEBUG) {
                Log.e("OS", "Received command: $type")
            }
            Command(id, type, value).exec(ctx)
        }
        return true
    } catch (parseException: JSONException) {
        parseException.fly()
        return false
    } catch (unitialized: UninitializedPropertyAccessException) {
        return false
    }
}

Other internal usage (used on background thread and main thread as well):

package com.kid.gl.backend

import android.util.Log
import com.google.firebase.database.*
import com.kid.gl.FSU_TAG
import com.kid.gl.utils.*
import com.mcxiaoke.koi.ext.timestamp
import com.onesignal.OneSignal
import org.json.JSONObject
import java.util.*
import kotlin.concurrent.schedule

abstract class Pushable {
    companion object {
        const val PARENTS = "par"
        const val KIDS = "kids"
        const val MAX_TTL = 86400

        const val SAFE = 1
        const val DANGER = -1
        const val GPS_DISABLED = -3
        const val MESSAGE = 0
        const val BATTERY_LOW = -4
        const val SHUTDOWN = -5

        private val dbRef get() = DBMan.getDb().child("enlv").child(tryGetKGL()!!.famkey)
    }
    abstract val value: String
    abstract val type: Int
    @get:Exclude protected val kgl by lazy { tryGetKGL() }

    @get:Exclude private var retries = 0

    open fun send(to: String? = null, callback: ((Boolean)->Unit)? = null) {
        if (kgl==null) {
            FirePig.fly("Pushable: Failed to get app context")
            return
        }
        if (kgl!!.state<0) {
            Log.wtf(FSU_TAG, "Id or famkey is null")
            return
        }
        val withWeb = type < 10 //&& type != 0
        val ids = when(to) {
            null -> Ids[A, withWeb]
            PARENTS -> Ids[P, withWeb]
            KIDS -> Ids[K, withWeb]
            else -> Ids[to]
        }
        if (ids=="") {
            if (kgl!!.family.size>0) {
                Ids.refresh(kgl!!.family)
                defer(to)
            }
            return
        }

        OneSignal.postNotification(json(ids), object : OneSignal.PostNotificationResponseHandler{
            override fun onSuccess(response: JSONObject?) {
                callback?.invoke(true)
            }
            override fun onFailure(response: JSONObject) {
                callback?.invoke(false)
                Log.wtf("Push delivery error", response.toString())
                Requirements.INTERNET.check(kgl!!)
                try {
                    val err = response.getJSONObject("errors")
                    if (err.has("invalid_player_ids")) {
                        val invalidIds = err.getJSONArray("invalid_player_ids")
                        for (i in 0 until invalidIds.length())
                            Ids.resend(invalidIds.getString(i), this@Pushable)
                    }
                } catch (e: Exception) {
                    val str = response.toString()
                    if (str.toLowerCase(Locale.US).contains("unknown"))
                        defer(to)
                    else {
                        FirePig.fly(response.toString())
                        e.fly("Pushable")
                    }
                }
            }
        })

        if (retries == 0 && (type == SAFE || type == DANGER)) {
            dbRef.push().setValue(mapOf(
                "id" to kgl!!.userId,
                "message" to "${if (type==SAFE) "entered" else "outof"}://$value",
                "timestamp" to timestamp()
            ))
        }
    }

    open fun defer(to: String?): Boolean {
        return if (retries++<4) {
            Timer().schedule(1000L) { send(to) }
            true
        } else false
    }

    protected abstract fun json(ids: String): JSONObject
    protected open val ttl get() = 3600
}

Another one (on both threads as well):

fun Context.setUserProperty(name: String, value: Any) {
    anal.setUserProperty(name, value.toString())
    OneSignal.sendTag(name, value.toString())
}

@jkasten2 jkasten2 added Medium Priority Possible Bug Maintainers need to confirm or reproduce labels Apr 27, 2018
@rostopira
Copy link
Author

Is there any progress on this?

Nightsd01 added a commit that referenced this issue May 17, 2018
Fixes #513, where in very rare cases a concurrency issue could occur where shared preferences was accessed during a buffer flush before the object was actually initialized
• Fixed by adding synchronization to all accesses/initialization of this object
@Nightsd01 Nightsd01 mentioned this issue May 17, 2018
jkasten2 pushed a commit that referenced this issue May 18, 2018
Fixes #513, where in very rare cases a concurrency issue could occur where shared preferences was accessed during a buffer flush before the object was actually initialized
• Fixed by adding synchronization to all accesses/initialization of this object
jkasten2 pushed a commit that referenced this issue May 18, 2018
Fixes #513, where in very rare cases a concurrency issue could occur where shared preferences was accessed during a buffer flush before the object was actually initialized
• Fixed by adding synchronization to all accesses/initialization of this object
@jkasten2
Copy link
Member

The issue was fixed in the PR above and is now released in version 3.9.1

@ominfowave
Copy link

ominfowave commented May 31, 2018

i have tested version 3.9.1 and 3.9.3 both causes the same issue.

Fatal Exception: java.lang.NullPointerException
       at com.onesignal.OneSignalPrefs$WritePrefHandlerThread.flushBufferToDisk(OneSignalPrefs.java:121)
       at com.onesignal.OneSignalPrefs$WritePrefHandlerThread.access$000(OneSignalPrefs.java:84)
       at com.onesignal.OneSignalPrefs$WritePrefHandlerThread$1.run(OneSignalPrefs.java:112)
       at android.os.Handler.handleCallback(Handler.java:733)
       at android.os.Handler.dispatchMessage(Handler.java:95)
       at android.os.Looper.loop(Looper.java:146)
       at android.os.HandlerThread.run(HandlerThread.java:61)

@VincentRoest
Copy link

This is still not resolved. Running into this with 3.9.1 as well.

@Nightsd01 Nightsd01 reopened this Jun 4, 2018
@rostopira
Copy link
Author

rostopira commented Jun 5, 2018

More than 3k of my users are currently affected by this issue (stats for 48 hours) on 3.9.1 as well
(Total: more than 6.2k crashes for 48 hours)

@Nightsd01
Copy link
Contributor

@rostopira apologies about the crashes your users are experiencing. We’ve identified the cause of the problem and will be releasing an update to fix it shortly. It’s very similar to the issue we fixed in 3.9.1. We’ll be checking the SDK thoroughly to make sure there aren’t further NPE’s like this.

@rostopira
Copy link
Author

@Nightsd01 any progress on this? This already caused delay in our release cycle

@jkasten2 jkasten2 removed the Possible Bug Maintainers need to confirm or reproduce label Jun 11, 2018
@jkasten2
Copy link
Member

@rostopira Sorry for the delay on this, we believe we have identified the root issue now. We should have a fix next week. We will update this ticket with our progress

@rostopira
Copy link
Author

@jkasten2 more than week passed already. I'm starting to loose hope. If you don't have any ETA, than I'll rollback to the old version

@jkasten2
Copy link
Member

@rostopira Sorry for the delay. We just released 3.9.2 which includes a fix for this issue.

@rostopira
Copy link
Author

I can confirm this issues is fixed
Great job

baptiste-nv added a commit to nventive/OneSignal-Android-SDK that referenced this issue Jul 4, 2018
* Unity Proxy setLocationShared

• Adds setLocationShared(bool) to the Unity proxy

* Fixed runtime issue when firebase-auth is added

* Specific when com.google.firebase:firebase-auth:15.1.0 is used the following error would throw
   - java.lang.IllegalArgumentException: Given String is empty or null
   - at com.google.android.gms.common.internal.Preconditions.checkNotEmpty(Unknown Source)
   - at com.google.firebase.auth.api.internal.zzcq.<init>(Unknown Source)
   - ...
   - at com.google.firebase.FirebaseApp.initializeApp(Unknown Source)
   - at com.onesignal.PushRegistratorFCM.initFirebaseApp(PushRegistratorFCM.java:64)

* Synchronize State

• Fixes a crash where the generateJsonDiff() method of the currentUserState property in UserStateSynchronizer is called before the currentUserState is initialized, by adding a synchronization step to the initUserState() method
• Adds a check to make sure the user state is initialized before internalSyncUserState() attempts to access it.

* Synchronize LocationGMS

• The GoogleAPIClient instance in LocationGMS (accessed through a proxy via reflection) was being accessed before it was being connected due to a race condition
• Introduces synchronization to ensure access to the instance only occurs after it is initialized & connected.

* Synchronize LocationUpdateListener

• Adds synchronization to LocationUpdateListener, a class that also access the same instance of GoogleApiClient (from the previous commit)

* Fixed proguard compatibility

* Proguard is now renaming public classes so checked to static class based checks.
  - This way when Proguard renames the public class name it will be updated in OSUtils.
  - This is better then adding a proguard rule to keep class as allows renaming which means a smaller APK.

* Removed reading from META-INF files

* This was not reliable after some additional testing on a device

* Fix Notification Restoration Crash for Android 7.0

• Fixes a crash (OneSignal#263) by disabling notification restoration after app upgrades for Android 7.0 devices.

* Update Comment

• Fixes an out of date comment, this change only checks against a specific version of android (not a range)

* Fix Null Pointer Concurrency Issue

• Fixes OneSignal#513, where in very rare cases a concurrency issue could occur where shared preferences was accessed during a buffer flush before the object was actually initialized
• Fixed by adding synchronization to all accesses/initialization of this object

* 3.9.1 Release commit

* Update location tracking (#1)

* Update location tracking

* Fix NRE

* Add lat/long tags
baptiste-nv added a commit to nventive/OneSignal-Android-SDK that referenced this issue Jul 11, 2018
* Unity Proxy setLocationShared

• Adds setLocationShared(bool) to the Unity proxy

* Fixed runtime issue when firebase-auth is added

* Specific when com.google.firebase:firebase-auth:15.1.0 is used the following error would throw
   - java.lang.IllegalArgumentException: Given String is empty or null
   - at com.google.android.gms.common.internal.Preconditions.checkNotEmpty(Unknown Source)
   - at com.google.firebase.auth.api.internal.zzcq.<init>(Unknown Source)
   - ...
   - at com.google.firebase.FirebaseApp.initializeApp(Unknown Source)
   - at com.onesignal.PushRegistratorFCM.initFirebaseApp(PushRegistratorFCM.java:64)

* Synchronize State

• Fixes a crash where the generateJsonDiff() method of the currentUserState property in UserStateSynchronizer is called before the currentUserState is initialized, by adding a synchronization step to the initUserState() method
• Adds a check to make sure the user state is initialized before internalSyncUserState() attempts to access it.

* Synchronize LocationGMS

• The GoogleAPIClient instance in LocationGMS (accessed through a proxy via reflection) was being accessed before it was being connected due to a race condition
• Introduces synchronization to ensure access to the instance only occurs after it is initialized & connected.

* Synchronize LocationUpdateListener

• Adds synchronization to LocationUpdateListener, a class that also access the same instance of GoogleApiClient (from the previous commit)

* Fixed proguard compatibility

* Proguard is now renaming public classes so checked to static class based checks.
  - This way when Proguard renames the public class name it will be updated in OSUtils.
  - This is better then adding a proguard rule to keep class as allows renaming which means a smaller APK.

* Removed reading from META-INF files

* This was not reliable after some additional testing on a device

* Fix Notification Restoration Crash for Android 7.0

• Fixes a crash (OneSignal#263) by disabling notification restoration after app upgrades for Android 7.0 devices.

* Update Comment

• Fixes an out of date comment, this change only checks against a specific version of android (not a range)

* Fix Null Pointer Concurrency Issue

• Fixes OneSignal#513, where in very rare cases a concurrency issue could occur where shared preferences was accessed during a buffer flush before the object was actually initialized
• Fixed by adding synchronization to all accesses/initialization of this object

* 3.9.1 Release commit

* Added GcmBroadcastReceiver to Eclipse example

* This is required to receive pushes and was remove by mistake in a previous commit

* Added google() to allprojects to root build.gradle

Fixes issue with jcenter not always resolving google dependencies

* Fix idsAvailable not firing a 2nd time

* Fixed issue where idsAvailable would not fire a 2nd time if registrationId the first time

* Revert "Fix Null Pointer Concurrency Issue"

This reverts commit 69f99c9.

* Added setAppContext which triggers dependents

* Added null check to flushBufferToDisk

* 3.9.2 Release commit

* 3.9.2 Release jar

* Fixes crash with FirebaseInstanceId

* This crash happens due Google not handling the case where there isn't a default FireBase app.
* Resolves OneSignal#552

* Handling IllegalStateException on Oreo

* Normally startWakefulService can be used when notification is sent with high FCM priority
  however this may still not be allowed in some cases.
* Resolves OneSignal#498

* Added Gradle plugin and updated to match Android SDK setup

* updated gradle files

* App gradle update
baptiste-nv added a commit to nventive/OneSignal-Android-SDK that referenced this issue Mar 20, 2019
* Unity Proxy setLocationShared

• Adds setLocationShared(bool) to the Unity proxy

* Fixed runtime issue when firebase-auth is added

* Specific when com.google.firebase:firebase-auth:15.1.0 is used the following error would throw
   - java.lang.IllegalArgumentException: Given String is empty or null
   - at com.google.android.gms.common.internal.Preconditions.checkNotEmpty(Unknown Source)
   - at com.google.firebase.auth.api.internal.zzcq.<init>(Unknown Source)
   - ...
   - at com.google.firebase.FirebaseApp.initializeApp(Unknown Source)
   - at com.onesignal.PushRegistratorFCM.initFirebaseApp(PushRegistratorFCM.java:64)

* Synchronize State

• Fixes a crash where the generateJsonDiff() method of the currentUserState property in UserStateSynchronizer is called before the currentUserState is initialized, by adding a synchronization step to the initUserState() method
• Adds a check to make sure the user state is initialized before internalSyncUserState() attempts to access it.

* Synchronize LocationGMS

• The GoogleAPIClient instance in LocationGMS (accessed through a proxy via reflection) was being accessed before it was being connected due to a race condition
• Introduces synchronization to ensure access to the instance only occurs after it is initialized & connected.

* Synchronize LocationUpdateListener

• Adds synchronization to LocationUpdateListener, a class that also access the same instance of GoogleApiClient (from the previous commit)

* Fixed proguard compatibility

* Proguard is now renaming public classes so checked to static class based checks.
  - This way when Proguard renames the public class name it will be updated in OSUtils.
  - This is better then adding a proguard rule to keep class as allows renaming which means a smaller APK.

* Removed reading from META-INF files

* This was not reliable after some additional testing on a device

* Fix Notification Restoration Crash for Android 7.0

• Fixes a crash (OneSignal#263) by disabling notification restoration after app upgrades for Android 7.0 devices.

* Update Comment

• Fixes an out of date comment, this change only checks against a specific version of android (not a range)

* Fix Null Pointer Concurrency Issue

• Fixes OneSignal#513, where in very rare cases a concurrency issue could occur where shared preferences was accessed during a buffer flush before the object was actually initialized
• Fixed by adding synchronization to all accesses/initialization of this object

* 3.9.1 Release commit

* Update location tracking (#1)

* Update location tracking

* Fix NRE

* Add lat/long tags
baptiste-nv added a commit to nventive/OneSignal-Android-SDK that referenced this issue Mar 20, 2019
* Unity Proxy setLocationShared

• Adds setLocationShared(bool) to the Unity proxy

* Fixed runtime issue when firebase-auth is added

* Specific when com.google.firebase:firebase-auth:15.1.0 is used the following error would throw
   - java.lang.IllegalArgumentException: Given String is empty or null
   - at com.google.android.gms.common.internal.Preconditions.checkNotEmpty(Unknown Source)
   - at com.google.firebase.auth.api.internal.zzcq.<init>(Unknown Source)
   - ...
   - at com.google.firebase.FirebaseApp.initializeApp(Unknown Source)
   - at com.onesignal.PushRegistratorFCM.initFirebaseApp(PushRegistratorFCM.java:64)

* Synchronize State

• Fixes a crash where the generateJsonDiff() method of the currentUserState property in UserStateSynchronizer is called before the currentUserState is initialized, by adding a synchronization step to the initUserState() method
• Adds a check to make sure the user state is initialized before internalSyncUserState() attempts to access it.

* Synchronize LocationGMS

• The GoogleAPIClient instance in LocationGMS (accessed through a proxy via reflection) was being accessed before it was being connected due to a race condition
• Introduces synchronization to ensure access to the instance only occurs after it is initialized & connected.

* Synchronize LocationUpdateListener

• Adds synchronization to LocationUpdateListener, a class that also access the same instance of GoogleApiClient (from the previous commit)

* Fixed proguard compatibility

* Proguard is now renaming public classes so checked to static class based checks.
  - This way when Proguard renames the public class name it will be updated in OSUtils.
  - This is better then adding a proguard rule to keep class as allows renaming which means a smaller APK.

* Removed reading from META-INF files

* This was not reliable after some additional testing on a device

* Fix Notification Restoration Crash for Android 7.0

• Fixes a crash (OneSignal#263) by disabling notification restoration after app upgrades for Android 7.0 devices.

* Update Comment

• Fixes an out of date comment, this change only checks against a specific version of android (not a range)

* Fix Null Pointer Concurrency Issue

• Fixes OneSignal#513, where in very rare cases a concurrency issue could occur where shared preferences was accessed during a buffer flush before the object was actually initialized
• Fixed by adding synchronization to all accesses/initialization of this object

* 3.9.1 Release commit

* Added GcmBroadcastReceiver to Eclipse example

* This is required to receive pushes and was remove by mistake in a previous commit

* Added google() to allprojects to root build.gradle

Fixes issue with jcenter not always resolving google dependencies

* Fix idsAvailable not firing a 2nd time

* Fixed issue where idsAvailable would not fire a 2nd time if registrationId the first time

* Revert "Fix Null Pointer Concurrency Issue"

This reverts commit 69f99c9.

* Added setAppContext which triggers dependents

* Added null check to flushBufferToDisk

* 3.9.2 Release commit

* 3.9.2 Release jar

* Fixes crash with FirebaseInstanceId

* This crash happens due Google not handling the case where there isn't a default FireBase app.
* Resolves OneSignal#552

* Handling IllegalStateException on Oreo

* Normally startWakefulService can be used when notification is sent with high FCM priority
  however this may still not be allowed in some cases.
* Resolves OneSignal#498

* Added Gradle plugin and updated to match Android SDK setup

* updated gradle files

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

Successfully merging a pull request may close this issue.

5 participants