-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[android] fix offline region download crashes #6293
Conversation
//Register new instance | ||
INSTANCE = new ConnectivityReceiver(context); | ||
context.registerReceiver(INSTANCE, new IntentFilter("android.net.conn.CONNECTIVITY_CHANGE")); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we ever unregister the broadcast receiver?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. I think this should remain active to facilitate background activity. We can unregister when/if MapboxAccountManager is released if needed. Why?
d9bb274
to
7a1c9f9
Compare
@@ -1515,10 +1515,12 @@ void setOfflineRegionObserver(JNIEnv *env, jni::jobject* offlineRegion_, jni::jo | |||
} | |||
|
|||
// Error object | |||
jni::jobject* jerror = &jni::NewObject(*env2, *offlineRegionErrorClass, *offlineRegionErrorConstructorId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you try using jni::PushLocalFrame
here? I'm curious if that alone would have fixed the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried it (again just now). But it doesn't help. Keeps crashing on:
********** Crash dump: **********
Build fingerprint: 'google/bullhead/bullhead:7.0/NRD90M/3085278:user/release-keys'
pid: 8721, tid: 10314, name: DefaultFileSour >>> com.mapbox.mapboxsdk.testapp <<<
signal 6 (SIGABRT), code -6 (SI_TKILL), fault addr --------
Stack frame #00 pc 00049d94 /system/lib/libc.so (tgkill+12)
Stack frame #01 pc 00047533 /system/lib/libc.so (pthread_kill+34)
Stack frame #02 pc 0001d885 /system/lib/libc.so (raise+10)
Stack frame #03 pc 000193d1 /system/lib/libc.so (__libc_android_abort+34)
Stack frame #04 pc 00017014 /system/lib/libc.so (abort+4)
Stack frame #05 pc 0001b87f /system/lib/libc.so (__libc_fatal+22)
Stack frame #06 pc 000195cb /system/lib/libc.so (__assert2+18)
Stack frame #07 pc 003a235f /data/app/com.mapbox.mapboxsdk.testapp-2/lib/arm/libmapbox-gl.so: Routine abort_message at /Volumes/Android/buildbot/src/android/ndk-r12-release/ndk/sources/cxx-stl/llvm-libc++abi/libcxxabi/src/abort_message.cpp:74
Stack frame #08 pc 003a2447 /data/app/com.mapbox.mapboxsdk.testapp-2/lib/arm/libmapbox-gl.so: Routine default_terminate_handler() at /Volumes/Android/buildbot/src/android/ndk-r12-release/ndk/sources/cxx-stl/llvm-libc++abi/libcxxabi/src/cxa_default_handlers.cpp:68
Stack frame #09 pc 003a0a31 /data/app/com.mapbox.mapboxsdk.testapp-2/lib/arm/libmapbox-gl.so: Routine std::__terminate(void (*)()) at /Volumes/Android/buildbot/src/android/ndk-r12-release/ndk/sources/cxx-stl/llvm-libc++abi/libcxxabi/src/cxa_handlers.cpp:68
Stack frame #10 pc 003a034b /data/app/com.mapbox.mapboxsdk.testapp-2/lib/arm/libmapbox-gl.so (__cxa_throw+122): Routine __cxxabiv1::failed_throw(__cxxabiv1::__cxa_exception*) at /Volumes/Android/buildbot/src/android/ndk-r12-release/ndk/sources/cxx-stl/llvm-libc++abi/libcxxabi/src/cxa_exception.cpp:149
Stack frame #11 pc 00144c68 /data/app/com.mapbox.mapboxsdk.testapp-2/lib/arm/libmapbox-gl.so: Routine jni::CheckJavaException(_JNIEnv&) at /Users/ivo/git/mapbox-gl-native/build/android-arm-v7/Debug/../../../mason_packages/headers/jni.hpp/2.0.0/include/jni/errors.hpp:69 (discriminator 2)
Stack frame #12 pc 0014615c /data/app/com.mapbox.mapboxsdk.testapp-2/lib/arm/libmapbox-gl.so: Routine std::__ndk1::__unique_if<mbgl::HTTPRequest>::__unique_single std::__ndk1::make_unique<mbgl::HTTPRequest, _JNIEnv&, mbgl::Resource const&, std::__ndk1::function<void (mbgl::Response)>&>(_JNIEnv&, mbgl::Resource const&, std::__ndk1::function<void (mbgl::Response)>&) at /Users/ivo/git/mapbox-gl-native/mason_packages/osx-x86_64/android-ndk/arm-9-r12b/bin/../lib/gcc/arm-linux-androideabi/4.9.x/../../../../include/c++/4.9.x/memory:3047 (discriminator 34)
Stack frame #13 pc 00152464 /data/app/com.mapbox.mapboxsdk.testapp-2/lib/arm/libmapbox-gl.so: Routine mbgl::OnlineFileSource::Impl::activateRequest(mbgl::OnlineFileRequest*) at /Users/ivo/git/mapbox-gl-native/build/android-arm-v7/Debug/../../../platform/default/online_file_source.cpp:99
Stack frame #14 pc 00152110 /data/app/com.mapbox.mapboxsdk.testapp-2/lib/arm/libmapbox-gl.so: Routine mbgl::OnlineFileSource::Impl::activatePendingRequest() at /Users/ivo/git/mapbox-gl-native/build/android-arm-v7/Debug/../../../platform/default/online_file_source.cpp:117
Stack frame #15 pc 001529f4 /data/app/com.mapbox.mapboxsdk.testapp-2/lib/arm/libmapbox-gl.so: Routine operator() at /Users/ivo/git/mapbox-gl-native/build/android-arm-v7/Debug/../../../platform/default/online_file_source.cpp:101
Stack frame #16 pc 0015289c /data/app/com.mapbox.mapboxsdk.testapp-2/lib/arm/libmapbox-gl.so: Routine _ZNSt6__ndk18__invokeIRZN4mbgl16OnlineFileSource4Impl15activateRequestEPNS1_17OnlineFileRequestEEUlNS1_8ResponseEE_JS6_EEEDTclclsr3std6__ndk1E7forwardIT_Efp_Espclsr3std6__ndk1E7forwardIT0_Efp0_EEEOS9_DpOSA_ at /Users/ivo/git/mapbox-gl-native/mason_packages/osx-x86_64/android-ndk/arm-9-r12b/bin/../lib/gcc/arm-linux-androideabi/4.9.x/../../../../include/c++/4.9.x/__functional_base:413
Stack frame #17 pc 00146364 /data/app/com.mapbox.mapboxsdk.testapp-2/lib/arm/libmapbox-gl.so: Routine mbgl::HTTPRequest::async::{lambda()#1}::operator()() const at /Users/ivo/git/mapbox-gl-native/build/android-arm-v7/Debug/../../../platform/android/src/http_file_source.cpp:46 (discriminator 1)
Stack frame #18 pc 0014229c /data/app/com.mapbox.mapboxsdk.testapp-2/lib/arm/libmapbox-gl.so: Routine mbgl::util::RunLoop::Impl::processRunnables() at /Users/ivo/git/mapbox-gl-native/build/android-arm-v7/Debug/../../../platform/android/src/run_loop.cpp:180
Stack frame #19 pc 00142828 /data/app/com.mapbox.mapboxsdk.testapp-2/lib/arm/libmapbox-gl.so: Routine mbgl::util::RunLoop::run() at /Users/ivo/git/mapbox-gl-native/build/android-arm-v7/Debug/../../../platform/android/src/run_loop.cpp:229 (discriminator 2)
Stack frame #20 pc 0014b8c8 /data/app/com.mapbox.mapboxsdk.testapp-2/lib/arm/libmapbox-gl.so: Routine void mbgl::util::Thread<mbgl::DefaultFileSource::Impl>::run<std::__ndk1::tuple<std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>, std::__ndk1::allocator<char> > const&, unsigned long long&>, 0u, 1u>(std::__ndk1::tuple<std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>, std::__ndk1::allocator<char> > const&, unsigned long long&>&&, std::__ndk1::integer_sequence<unsigned int, 0u, 1u>) at /Users/ivo/git/mapbox-gl-native/build/android-arm-v7/Debug/../../../src/mbgl/util/thread.hpp:111
Stack frame #21 pc 0014b838 /data/app/com.mapbox.mapboxsdk.testapp-2/lib/arm/libmapbox-gl.so: Routine operator() at /Users/ivo/git/mapbox-gl-native/build/android-arm-v7/Debug/../../../src/mbgl/util/thread.hpp:95 (discriminator 1)
Stack frame #22 pc 00047003 /system/lib/libc.so (_ZL15__pthread_startPv+22)
Stack frame #23 pc 00019e1d /system/lib/libc.so (__start_thread+6)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, well, I'm really skeptical about switching everything from PushLocalFrame
/PopLocalFrame
to DeleteLocalRef
. My hunch:
PushLocalFrame
throwingOutOfMemoryError
is a symptom of local references leaking elsewhere. It just happens to be the place where hit the ceiling.- Switching to
DeleteLocalRef
happens to prevent hitting the ceiling, because allocations/deallocations are slightly more granular, but doesn't fix the underlying leak.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jfirebaugh You were right off course, PushLocalFrame was not the issue, my understanding of it was. Found a way to dump the reference table and noticed that the references weren't cleared properly because I didn't store the unique_ptr and the frame got deleted too soon.
Good news is that the local reference table doesn't contain any leaks (in this part). Looks like this during download / offline situations now:
--- reference table dump ---
local reference table dump:
Last 3 entries (of 3):
2: 0x6f827b00 java.lang.Class<android.os.Debug>
1: 0x12f50088 byte[] (25 elements)
0: 0x12c88fa0 com.mapbox.mapboxsdk.offline.OfflineRegion
Summary:
1 of com.mapbox.mapboxsdk.offline.OfflineRegion
1 of java.lang.Class
1 of byte[] (25 elements)
Still running into issues on re-enabling the connection though. Besides the SQLite WAL exceptions, this one is most prevalent:
09-14 12:23:08.038 5269-5269/? A/DEBUG: Abort message: '/Volumes/Android/buildbot/src/android/ndk-r12-release/ndk/sources/cxx-stl/llvm-libc++abi/libcxxabi/src/abort_message.cpp:74: void abort_message(const char *, ...): assertion "terminating with uncaught exception of type std::range_error: wstring_convert: from_bytes error" failed'
********** Crash dump: **********
Build fingerprint: 'google/bullhead/bullhead:7.0/NRD90M/3085278:user/release-keys'
pid: 3666, tid: 3819, name: DefaultFileSour >>> com.mapbox.mapboxsdk.testapp <<<
signal 6 (SIGABRT), code -6 (SI_TKILL), fault addr --------
Stack frame #00 pc 00049d94 /system/lib/libc.so (tgkill+12)
Stack frame #01 pc 00047533 /system/lib/libc.so (pthread_kill+34)
Stack frame #02 pc 0001d885 /system/lib/libc.so (raise+10)
Stack frame #03 pc 000193d1 /system/lib/libc.so (__libc_android_abort+34)
Stack frame #04 pc 00017014 /system/lib/libc.so (abort+4)
Stack frame #05 pc 0001b87f /system/lib/libc.so (__libc_fatal+22)
Stack frame #06 pc 000195cb /system/lib/libc.so (__assert2+18)
Stack frame #07 pc 003a6adf /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so: Routine abort_message at /Volumes/Android/buildbot/src/android/ndk-r12-release/ndk/sources/cxx-stl/llvm-libc++abi/libcxxabi/src/abort_message.cpp:74
Stack frame #08 pc 003a6ba7 /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so: Routine default_terminate_handler() at /Volumes/Android/buildbot/src/android/ndk-r12-release/ndk/sources/cxx-stl/llvm-libc++abi/libcxxabi/src/cxa_default_handlers.cpp:63
Stack frame #09 pc 003a51b1 /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so: Routine std::__terminate(void (*)()) at /Volumes/Android/buildbot/src/android/ndk-r12-release/ndk/sources/cxx-stl/llvm-libc++abi/libcxxabi/src/cxa_handlers.cpp:68
Stack frame #10 pc 003a4acb /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so (__cxa_throw+122): Routine __cxxabiv1::failed_throw(__cxxabiv1::__cxa_exception*) at /Volumes/Android/buildbot/src/android/ndk-r12-release/ndk/sources/cxx-stl/llvm-libc++abi/libcxxabi/src/cxa_exception.cpp:149
Stack frame #11 pc 0004b48c /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so: Routine std::__ndk1::wstring_convert<std::__ndk1::codecvt_utf8_utf16<char16_t, 1114111ul, (std::__ndk1::codecvt_mode)0>, char16_t, std::__ndk1::allocator<char16_t>, std::__ndk1::allocator<char> >::from_bytes(char const*, char const*) at /Users/ivo/git/mapbox-gl-native/mason_packages/osx-x86_64/android-ndk/arm-9-r12b/bin/../lib/gcc/arm-linux-androideabi/4.9.x/../../../../include/c++/4.9.x/locale:3909 (discriminator 1)
Stack frame #12 pc 0004b0c4 /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so: Routine std::__ndk1::wstring_convert<std::__ndk1::codecvt_utf8_utf16<char16_t, 1114111ul, (std::__ndk1::codecvt_mode)0>, char16_t, std::__ndk1::allocator<char16_t>, std::__ndk1::allocator<char> >::from_bytes(std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>, std::__ndk1::allocator<char> > const&) at /Users/ivo/git/mapbox-gl-native/mason_packages/osx-x86_64/android-ndk/arm-9-r12b/bin/../lib/gcc/arm-linux-androideabi/4.9.x/../../../../include/c++/4.9.x/locale:3786 (discriminator 3)
Stack frame #13 pc 00146e74 /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so: Routine jni::Object<jni::StringTag> jni::Make<jni::Object<jni::StringTag>, _JNIEnv&, std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>, std::__ndk1::allocator<char> >&>(_JNIEnv&, std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>, std::__ndk1::allocator<char> >&) at /Users/ivo/git/mapbox-gl-native/build/android-arm-v7/Debug/../../../mason_packages/headers/jni.hpp/2.0.0/include/jni/make.hpp:10 (discriminator 2)
Stack frame #14 pc 001487e0 /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so: Routine std::__ndk1::__unique_if<mbgl::HTTPRequest>::__unique_single std::__ndk1::make_unique<mbgl::HTTPRequest, _JNIEnv&, mbgl::Resource const&, std::__ndk1::function<void (mbgl::Response)>&>(_JNIEnv&, mbgl::Resource const&, std::__ndk1::function<void (mbgl::Response)>&) at /Users/ivo/git/mapbox-gl-native/mason_packages/osx-x86_64/android-ndk/arm-9-r12b/bin/../lib/gcc/arm-linux-androideabi/4.9.x/../../../../include/c++/4.9.x/memory:3047 (discriminator 46)
Stack frame #15 pc 00155fb8 /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so: Routine mbgl::OnlineFileSource::Impl::activateRequest(mbgl::OnlineFileRequest*) at /Users/ivo/git/mapbox-gl-native/build/android-arm-v7/Debug/../../../platform/default/online_file_source.cpp:101
Stack frame #16 pc 00155c64 /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so: Routine mbgl::OnlineFileSource::Impl::activatePendingRequest() at /Users/ivo/git/mapbox-gl-native/build/android-arm-v7/Debug/../../../platform/default/online_file_source.cpp:119
Stack frame #17 pc 00156548 /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so: Routine operator() at /Users/ivo/git/mapbox-gl-native/build/android-arm-v7/Debug/../../../platform/default/online_file_source.cpp:103
Stack frame #18 pc 001563f0 /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so: Routine _ZNSt6__ndk18__invokeIRZN4mbgl16OnlineFileSource4Impl15activateRequestEPNS1_17OnlineFileRequestEEUlNS1_8ResponseEE_JS6_EEEDTclclsr3std6__ndk1E7forwardIT_Efp_Espclsr3std6__ndk1E7forwardIT0_Efp0_EEEOS9_DpOSA_ at /Users/ivo/git/mapbox-gl-native/mason_packages/osx-x86_64/android-ndk/arm-9-r12b/bin/../lib/gcc/arm-linux-androideabi/4.9.x/../../../../include/c++/4.9.x/__functional_base:413
Stack frame #19 pc 001489e8 /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so: Routine mbgl::HTTPRequest::async::{lambda()#1}::operator()() const at /Users/ivo/git/mapbox-gl-native/build/android-arm-v7/Debug/../../../platform/android/src/http_file_source.cpp:47 (discriminator 1)
Stack frame #20 pc 001444a0 /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so: Routine mbgl::util::RunLoop::Impl::processRunnables() at /Users/ivo/git/mapbox-gl-native/build/android-arm-v7/Debug/../../../platform/android/src/run_loop.cpp:180
Stack frame #21 pc 00144a2c /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so: Routine mbgl::util::RunLoop::run() at /Users/ivo/git/mapbox-gl-native/build/android-arm-v7/Debug/../../../platform/android/src/run_loop.cpp:229 (discriminator 2)
Stack frame #22 pc 0014e784 /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so: Routine void mbgl::util::Thread<mbgl::DefaultFileSource::Impl>::run<std::__ndk1::tuple<std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>, std::__ndk1::allocator<char> > const&, unsigned long long&>, 0u, 1u>(std::__ndk1::tuple<std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>, std::__ndk1::allocator<char> > const&, unsigned long long&>&&, std::__ndk1::integer_sequence<unsigned int, 0u, 1u>) at /Users/ivo/git/mapbox-gl-native/build/android-arm-v7/Debug/../../../src/mbgl/util/thread.hpp:114
Stack frame #23 pc 0014e6f4 /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so: Routine operator() at /Users/ivo/git/mapbox-gl-native/build/android-arm-v7/Debug/../../../src/mbgl/util/thread.hpp:98 (discriminator 1)
Stack frame #24 pc 00047003 /system/lib/libc.so (_ZL15__pthread_startPv+22)
Stack frame #25 pc 00019e1d /system/lib/libc.so (__start_thread+6)
e16d266
to
eb89202
Compare
864f57b
to
13b0bc3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit comments, looking great otherwise!
|
||
private List<ConnectivityListener> listeners = new CopyOnWriteArrayList<>(); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: enter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
*/ | ||
public interface ConnectivityListener { | ||
|
||
void onNetworkStateChanged(boolean connected); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: javadoc in case of public API interface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
class NativeConnectivityListener implements ConnectivityListener { | ||
|
||
//TODO: Centralize this | ||
static { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: todo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
"finalize", | ||
METHOD(&ConnectivityListener::onConnectivityStateChanged, "nativeOnConnectivityStateChanged") | ||
); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: enter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
13b0bc3
to
21698a5
Compare
@tobrun Thanks for the review, made the requested changes |
assert(pendingRequestsMap.size() == pendingRequestsList.size()); | ||
} | ||
|
||
bool isScheduled(OnlineFileRequest* request) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be isPending
. "Scheduled" means the timer is running -- it's a state that precedes "Pending".
21698a5
to
aece538
Compare
@jfirebaugh Thanks for the review. I applied the requested changes. |
aece538
to
c4420d0
Compare
@ivovandongen 🎉 . |
Fixes #6210
Fixes #5118
Fixes #5827