Crash with no UpdateCheckDoneCallback passed

[asmirnov]

Environment

We have to force update subscriptions right after preloaded resource is returned. So we invoke filterEngine.forceUpdateCheck() in java code and it invokes engine->forceUpdateCheck(0):
​https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-android/jni/JniFilterEngine.cpp#l246

When the callback is about to be invoked the app crashes with signal 6 SIGABRT.
I think it tries to invoke a callback using null pointer.

How to reproduce

1. invoke filterEngine.forceUpdateCheck()
2. wait for few seconds
3. make sure the app crashed

...

Observed behaviour

the app crashes

Expected behaviour

check is done with no any callback fired

---

I've modified c+ jni class to output ptr value passed:

static void JNICALL JniForceUpdateCheck(JNIEnv* env, jclass clazz, jlong ptr, jlong updaterPtr)
{
  AdblockPlus::FilterEngine* engine = JniLongToTypePtr<AdblockPlus::FilterEngine>(ptr);
  JniUpdateCheckDoneCallback* callback =
      JniLongToTypePtr<JniUpdateCheckDoneCallback>(updaterPtr);

  AdblockPlus::FilterEngine::UpdateCheckDoneCallback
      updateCheckDoneCallback = 0;

  __android_log_print(ANDROID_LOG_DEBUG, "abp_ndk", "updatePtr = %d", (int)updaterPtr);

  if (updaterPtr)
  {
    updateCheckDoneCallback =
        std::bind(&JniUpdateCheckDoneCallback::Callback, callback,
                       std::placeholders::_1);
  }

  try
  {
    engine->ForceUpdateCheck(updateCheckDoneCallback);
  }
  CATCH_AND_THROW(env)
}

and you can see 0 is passed:

03-20 12:46:51.535 4848-4848/org.adblockplus.libadblockplus.android.webviewapp D/abp_ndk: updatePtr = 0

stacktrace:

03-20 12:46:56.201 5169-5169/? A/DEBUG: signal 6 (SIGABRT), code -6 (SI_TKILL), fault addr --------
03-20 12:46:56.201 5169-5169/? A/DEBUG:     r0 00000000  r1 000013cd  r2 00000006  r3 00000008
03-20 12:46:56.201 5169-5169/? A/DEBUG:     r4 d0683978  r5 00000006  r6 d0683920  r7 0000010c
03-20 12:46:56.201 5169-5169/? A/DEBUG:     r8 eb85eb60  r9 d0682ec4  sl d5734711  fp d39e7040
03-20 12:46:56.201 5169-5169/? A/DEBUG:     ip 00000000  sp d0682970  lr f1a5b497  pc f1a5dd18  cpsr 200b0010
03-20 12:46:56.210 5169-5169/? A/DEBUG: backtrace:
03-20 12:46:56.210 5169-5169/? A/DEBUG:     #00 pc 00049d18  /system/lib/libc.so (tgkill+12)
03-20 12:46:56.210 5169-5169/? A/DEBUG:     #01 pc 00047493  /system/lib/libc.so (pthread_kill+34)
03-20 12:46:56.210 5169-5169/? A/DEBUG:     #02 pc 0001d769  /system/lib/libc.so (raise+10)
03-20 12:46:56.211 5169-5169/? A/DEBUG:     #03 pc 00019261  /system/lib/libc.so (__libc_android_abort+34)
03-20 12:46:56.211 5169-5169/? A/DEBUG:     #04 pc 000172c8  /system/lib/libc.so (abort+4)
03-20 12:46:56.211 5169-5169/? A/DEBUG:     #05 pc 0049819d  /data/app/org.adblockplus.libadblockplus.android.webviewapp-1/lib/arm/libadblockplus-jni.so
03-20 12:46:56.211 5169-5169/? A/DEBUG:     #06 pc 00498269  /data/app/org.adblockplus.libadblockplus.android.webviewapp-1/lib/arm/libadblockplus-jni.so
03-20 12:46:56.211 5169-5169/? A/DEBUG:     #07 pc 0048d0c7  /data/app/org.adblockplus.libadblockplus.android.webviewapp-1/lib/arm/libadblockplus-jni.so
03-20 12:46:56.211 5169-5169/? A/DEBUG:     #08 pc 0048c777  /data/app/org.adblockplus.libadblockplus.android.webviewapp-1/lib/arm/libadblockplus-jni.so (__cxa_throw+182)
03-20 12:46:56.211 5169-5169/? A/DEBUG:     #09 pc 001af54d  /data/app/org.adblockplus.libadblockplus.android.webviewapp-1/lib/arm/libadblockplus-jni.so (_ZN11AdblockPlus12FilterEngine15UpdateCheckDoneERKNSt3__112basic_stringIcNS1_11char_traitsIcEENS1_9allocatorIcEEEENS1_8functionIFvS9_EEERNS1_6vectorINS1_10shared_ptrINS_7JsValueEEENS5_ISG_EEEE+120)
03-20 12:46:56.211 5169-5169/? A/DEBUG:     #10 pc 001affb5  /data/app/org.adblockplus.libadblockplus.android.webviewapp-1/lib/arm/libadblockplus-jni.so (_ZNSt3__110__function6__funcINS_6__bindIMN11AdblockPlus12FilterEngineEFvRKNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEENS_8functionIFvSC_EEERNS_6vectorINS_10shared_ptrINS3_7JsValueEEENS8_ISJ_EEEEEJPS4_RSA_RSF_RNS_12placeholders4__phILi1EEEEEENS8_ISW_EEFvSM_EEclESM_+96)
03-20 12:46:56.211 5169-5169/? A/DEBUG:     #11 pc 001b14eb  /data/app/org.adblockplus.libadblockplus.android.webviewapp-1/lib/arm/libadblockplus-jni.so (_ZN11AdblockPlus8JsEngine12TriggerEventERKNSt3__112basic_stringIcNS1_11char_traitsIcEENS1_9allocatorIcEEEERNS1_6vectorINS1_10shared_ptrINS_7JsValueEEENS5_ISD_EEEE+34)
03-20 12:46:56.211 5169-5169/? A/DEBUG:     #12 pc 001b67f3  /data/app/org.adblockplus.libadblockplus.android.webviewapp-1/lib/arm/libadblockplus-jni.so
03-20 12:46:56.211 5169-5169/? A/DEBUG:     #13 pc 001e1694  /data/app/org.adblockplus.libadblockplus.android.webviewapp-1/lib/arm/libadblockplus-jni.so (_ZN2v88internal25FunctionCallbackArguments4CallEPFNS_6HandleINS_5ValueEEERKNS_9ArgumentsEE+368)
03-20 12:46:56.211 5169-5169/? A/DEBUG:     #14 pc 001ef8d0  /data/app/org.adblockplus.libadblockplus.android.webviewapp-1/lib/arm/libadblockplus-jni.so
03-20 12:46:56.211 5169-5169/? A/DEBUG:     #15 pc 00000b78  <anonymous:20a0a000>

[sergz]

Yes, it tries to call an empty callback. I would not say that it's a bug if the parameter were not marked as optional.

I am going to add the check whether the callback is not an empty function before calling of the it.

[asmirnov]

Yes, i've been thinking about it but i rather expect to see that the argument is required than not so see that the argument is optional.
A question of agreement though.

I can see ​https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java#l95 method with no argument so i assume it's expected use case.
That's why i decided that the argument of c++ class is meant to be optional and this is actually a bug and not just required argument.

[asmirnov]

BTW i've checked with not null callback and it's fired after check is done.
So with not null callback it's working as expected.

[sergz]

There is already ticket #1397.