Skip to content
This repository has been archived by the owner on Jul 3, 2020. It is now read-only.

android_glue::add_sender: Sender error #239

Closed
OptimisticPeach opened this issue Sep 1, 2019 · 7 comments
Closed

android_glue::add_sender: Sender error #239

OptimisticPeach opened this issue Sep 1, 2019 · 7 comments

Comments

@OptimisticPeach
Copy link
Contributor

OptimisticPeach commented Sep 1, 2019

I get a panic when using the android_glue::add_sender api:

thread '<unnamed>' panicked at 'assertion failed: (*self.data.get()).is_none()', src/libstd/sync/mpsc/oneshot.rs:90:13

I would like to provide a larger backtrace, but it is quickly followed by a SIGABRT from the system:

--------- beginning of crash
F libc    : Fatal signal 6 (SIGABRT), code -6 (SI_TKILL) in tid 9521 (ust.doze_widget), pid 9505 (ust.doze_widget)
V threaded_app: Resume: 0x7b5b135280
I crash_dump64: obtaining output fd from tombstoned, type: kDebuggerdTombstone
I /system/bin/tombstoned: received crash request for pid 9521
I crash_dump64: performing dump of process 9505 (target tid = 9521)
F DEBUG   : *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***
F DEBUG   : Build fingerprint: 'google/walleye/walleye:9/PQ3A.190801.002/5670241:user/release-keys'
F DEBUG   : Revision: 'MP1'
F DEBUG   : ABI: 'arm64'
F DEBUG   : pid: 9505, tid: 9521, name: ust.doze_widget  >>> rust.doze_widget <<<
F DEBUG   : signal 6 (SIGABRT), code -6 (SI_TKILL), fault addr --------
F DEBUG   :     x0  0000000000000000  x1  0000000000002531  x2  0000000000000006  x3  0000000000000008
F DEBUG   :     x4  0000000000000000  x5  0000000000000000  x6  0000000000000000  x7  0000007b42ec0000
F DEBUG   :     x8  0000000000000083  x9  0000007bdddf89a0  x10 fffffff87ffffbdf  x11 0000000000000001
F DEBUG   :     x12 0000000000000001  x13 0000000000000000  x14 ffffffffffffffff  x15 0000000000000000
F DEBUG   :     x16 0000007bdde312c8  x17 0000007bddd6f2d8  x18 0000007b453ec23a  x19 0000000000002521
F DEBUG   :     x20 0000000000002531  x21 0000000000000083  x22 0000007b4374a7ac  x23 0000007b453ec130
F DEBUG   :     x24 0000007b453ec130  x25 000000000000000d  x26 0000000000000001  x27 0000007b43794a9c
F DEBUG   :     x28 000000000000001c  x29 0000007b453eaf70
F DEBUG   :     sp  0000007b453eaf30  lr  0000007bddd63a90  pc  0000007bddd63abc
F DEBUG   :
F DEBUG   : backtrace:
F DEBUG   :     #00 pc 0000000000021abc  /system/lib64/libc.so (abort+124)
F DEBUG   :     #01 pc 00000000008d1224  /data/app/rust.doze_widget-P-GGnI-kpfm2VhbJLCPJDw==/lib/arm64/libdoze_widget.so (uw_init_context_1+84)
W ActivityManager:   Force finishing activity rust.doze_widget/android.app.NativeActivity

Times and pid (I think) are omitted for brevity.

I have no unsafe code on my side, but while compiling I get

warning: use of deprecated item 'std::mem::uninitialized': use `mem::MaybeUninit` instead
   --> Z:\android\doze_widget\doze_widget\target\android-artifacts\debug\injected-glue\lib.rs:348:61
    |
348 |             let mut source: *mut ffi::android_poll_source = mem::uninitialized();
    |                                                             ^^^^^^^^^^^^^^^^^^

Which might explain it, so I changed it on my local version to use MaybeUninit as follows (Which should probably be done either way):

let mut events = mem::MaybeUninit::uninit();
let mut source: mem::MaybeUninit<*mut ffi::android_poll_source> = mem::MaybeUninit::uninit();

// A `-1` means to block forever, but any other positive value
// specifies the number of milliseconds to block for, before
// returning.
let code = ffi::ALooper_pollAll(-1, ptr::null_mut(), events.as_mut_ptr(),
                                source.as_mut_ptr() as *mut _ as *mut _);
if code == ffi::ALOOPER_POLL_WAKE {
    send_event(Event::Wake)
}

// If the application thread has exited then we need to exit also.
if is_app_thread_terminated().0 {
    // Not sure exactly how to do this, or what might be the proper
    // manner in which to do it.
    //
    // (1) hide ourselves by switching to home screen
    // (2) display message that we have finished
    // (3) do nothing like we are doing now
    //
    // We must keep this thread going so it can service events, else
    // the user will get a locked UI until the system terminates our
    // process. So we continue processing events..
}
let source = source.assume_init();
// Processing the event
if !source.is_null() {
    ((*source).process)(ANDROID_APP, source);
}

Which unfortunately didn't resolve the problem. I'm not well versed on how the event system works.

The panic occurs here, on the docs version of the code and here on github.

@philip-alldredge
Copy link
Contributor

I"m not too familiar with how all the event passing works. I took a quick look and I didn't see anything obviously wrong. Are you using the glue from the master branch?

I agree that the use of mem::uninitialized should be fixed.

@OptimisticPeach
Copy link
Contributor Author

I am, although I cloned pre- #237 and applied the steps you mentioned in #234 to do the navigation bar and fix stuff. It's relatively simple to use mem::MaybeUninit, I'll post a PR tomorrow fixing that.

@mb64
Copy link
Contributor

mb64 commented Sep 2, 2019

The event code hasn't been touched for a while, and it's riddled with raw pointers and such. This smells suspiciously like some bad pointer use.

What's the smallest example you have that runs into this issue?

@OptimisticPeach
Copy link
Contributor Author

OptimisticPeach commented Sep 2, 2019

Ok, so this crashes, but it's not the standard example:

pub fn enable_backtrace() {
    use std::env;
    let key = "RUST_BACKTRACE";
    env::set_var(key, "1");
}

fn main() {
    enable_backtrace();
    write_log("Hey, this is working! ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~This is to make this statement visible~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~");
    let (sender, receiver) = std::sync::mpsc::channel();
    android_glue::add_sender(sender);
    loop {
        for i in receiver.try_iter() {
            write_log(&format!("{:?}", i));
        }
    }
}

The "not the standard example" I mentioned is that I've made the following modifications to cargo-apk:

  • The MaybeUninit stuff mentioned above
  • Added the following to the bottom of ./injected-glue/lib.rs:
    #[no_mangle]
    #[inline(never)]
    #[allow(non_snake_case)]
    pub unsafe extern "C" fn ANativeActivity_onCreate(activity: *mut c_void, saved_state: *mut c_void, saved_state_size: usize) {
        ANativeActivity_onCreate2(activity, saved_state, saved_state_size);
    }
    
    extern {
        #[allow(non_snake_case)]
        fn ANativeActivity_onCreate2(activity: *mut c_void, saved_state: *mut c_void, saved_state_size: usize);
    }
    
  • Renamed ANativeActivity_onCreate in ./native_app_glue/android_native_app_glue.c to ANativeActivity_onCreate2
  • Instead of using C code as suggested in What to replace arm-linux-androideabi with in manifest? #234 for removal of the navigation bars, I inserted a function into ./native_app_glue/android_native_app_glue.h defined as so:
    /**
     * This is the function that is run on startup in the main thread
     * For example, to hide the navigation bars. 
     */
    extern void android_startup(ANativeActivity* activity, int focused);
    
    And called in <./native_app_glue/android_native_app_glue.c>::onWindowFocusChanged:
    static void onWindowFocusChanged(ANativeActivity* activity, int focused) {
        LOGV("WindowFocusChanged: %p -- %d\n", activity, focused);
        android_app_write_cmd((struct android_app*)activity->instance,
                focused ? APP_CMD_GAINED_FOCUS : APP_CMD_LOST_FOCUS);
        android_startup(activity, focused);
    }
    

As far as I can tell these changes shouldn't realistically do anything, so I tried with the master branch:

> cargo install --git "https://github.com/rust-windowing/android-rs-glue.git" cargo-apk -f

And the same results arise.


Although upon further inspection of the cargo apk install log, I'm apparently compiling for debug and not release, so AFAIK invalid pointer arithmetic shouldn't cause compiler-bound UB.

@OptimisticPeach
Copy link
Contributor Author

By installing as follows:

> cargo apk build --release
Compiling injected-glue for aarch64-linux-android
warning: use of deprecated item 'std::mem::uninitialized': use `mem::MaybeUninit` instead
   --> Z:\android\doze_widget\doze_widget\target\android-artifacts\release\injected-glue\lib.rs:298:30
    |
298 |             let mut thread = mem::uninitialized();
    |                              ^^^^^^^^^^^^^^^^^^
    |
    = note: `#[warn(deprecated)]` on by default

warning: use of deprecated item 'std::mem::uninitialized': use `mem::MaybeUninit` instead
   --> Z:\android\doze_widget\doze_widget\target\android-artifacts\release\injected-glue\lib.rs:347:30
    |
347 |             let mut events = mem::uninitialized();
    |                              ^^^^^^^^^^^^^^^^^^

warning: use of deprecated item 'std::mem::uninitialized': use `mem::MaybeUninit` instead
   --> Z:\android\doze_widget\doze_widget\target\android-artifacts\release\injected-glue\lib.rs:348:61
    |
348 |             let mut source: *mut ffi::android_poll_source = mem::uninitialized();
    |                                                             ^^^^^^^^^^^^^^^^^^

   Compiling android_glue v0.2.3
   Compiling doze_widget v0.1.0 (Z:\android\doze_widget\doze_widget)
warning: unused variable: `activity`
 --> src\__cargo_apk_main.tmp:7:38
  |
7 | pub unsafe extern fn android_startup(activity: *const ffi::ANativeActivity, _focused: i32) {
  |                                      ^^^^^^^^ help: consider prefixing with an underscore: `_activity`
  |
  = note: `#[warn(unused_variables)]` on by default

    Finished release [optimized] target(s) in 2.38s
aapt2.exe W 09-02 08:25:22  3672 11924 ApkAssets.cpp:138] resources.arsc in APK 'P:\sdk\platforms\android-29\android.jar' is compressed.
 'lib/arm64-v8a/libdoze_widget.so'...
Verifying alignment of Z:\android\doze_widget\doze_widget\target\android-artifacts\release\apk\doze_widget.apk (4)...
      49 AndroidManifest.xml (OK - compressed)
    1248 res/mipmap-mdpi-v4/ic_launcher.png (OK)
    3272 res/mipmap-hdpi-v4/ic_launcher.png (OK)
    6496 res/mipmap-xhdpi-v4/ic_launcher.png (OK)
   10616 res/mipmap-xxhdpi-v4/ic_launcher.png (OK)
   16716 res/mipmap-xxxhdpi-v4/ic_launcher.png (OK)
   25072 resources.arsc (OK)
   26303 assets/fonts\Ubuntu-R.ttf (OK - compressed)
  198688 lib/arm64-v8a/libdoze_widget.so (OK - compressed)
Verification succesful
> adb install "./target/android-artifacts/release/apk/doze_widget.apk"
Performing Streamed Install
Success

I result in the same error, with the master branch and the code above for main and enable_backtrace.

@philip-alldredge
Copy link
Contributor

@OptimisticPeach
Based on the build log above, The android_glue being used is from crates.io and not the one from master. When using the crates.io version, I am able to reproduce the issue.

Using the android_glue crate from the master branch results in things working as expected.

I assume the crash occurs because the Event enum that is exchanged between android_glue and cargo-apk has changed since the last release.

@OptimisticPeach
Copy link
Contributor Author

Indeed this was the error! Thanks for the help. I'll get onto that PR in a moment.

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

No branches or pull requests

3 participants