Simplify JNI interface for Mullvad daemon

[dlon]

This reduces the JNI interface for the Mullvad daemon to a start/init function and a stop function. mullvad-jni was also refactored.

Fix DROID-1050.
Fix DES-760.

This change is 

[linear]

DROID-1050 Fix initialization/denationalization of MullvadDaemon

[linear]

DES-760 Rework startup/shutdown jni interface

[Pururun]

⭐ Good job!

Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dlon)

---

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadDaemon.kt line 46 at r1 (raw file):

    suspend fun shutdown() =
        withContext(Dispatchers.IO) {

We should check if withContext can be safely removed or not.

[MarkusPettersson98]

Reviewed 6 of 8 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dlon)

---

mullvad-jni/src/lib.rs line 4 at r1 (raw file):

mod api;
mod classes;

I think HashSet can safely be removed from classes.rs, no? Is it still used on the Android side? @Pururun

Code quote:

mod classes;

---

mullvad-jni/src/lib.rs line 84 at r1 (raw file):

            0
        }
    }

IMHO, I think boxing the DaemonContext is a concern of this function and not start. So if we could both box the DaemonContext and turn it into a raw pointer here that would be great :)

Code quote:

    match start(
        env,
        vpnService,
        rpcSocketPath,
        filesDirectory,
        cacheDirectory,
        apiEndpoint,
    ) {
        Ok(daemon) => Box::into_raw(daemon) as jlong,
        Err(message) => {
            env.throw(message.to_string())
                .expect("Failed to throw exception");
            0
        }
    }

---

mullvad-jni/src/lib.rs line 121 at r1 (raw file):

    let rpc_socket = PathBuf::from(String::from_java(&env, rpc_socket_path));
    let files_dir = PathBuf::from(String::from_java(&env, files_directory));
    let cache_dir = PathBuf::from(String::from_java(&env, cache_directory));

Nit What do you think about DRY-ing a little?

let [rpc_socket, files_dir, cache_dir] = [rpc_socket_path, files_directory, cache_directory]
    .map(|path| String::from_java(&env, path))
    .map(PathBuf::from);

Code quote:

    let rpc_socket = PathBuf::from(String::from_java(&env, rpc_socket_path));
    let files_dir = PathBuf::from(String::from_java(&env, files_directory));
    let cache_dir = PathBuf::from(String::from_java(&env, cache_directory));

[albin-mullvad]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dlon)

---

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadDaemon.kt line 33 at r1 (raw file):

        migrateSplitTunneling.migrate()

        mullvadDaemonHandle = start(

Are there any guarantees for the return value that we can assert on the kotlin side? For instance I assume it shouldn't return zero or negative numbers? We might want to wrap the Long in a value class to help with that.

[albin-mullvad]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dlon)

---

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadDaemon.kt line 33 at r1 (raw file):

Previously, albin-mullvad wrote…

Are there any guarantees for the return value that we can assert on the kotlin side? For instance I assume it shouldn't return zero or negative numbers? We might want to wrap the Long in a value class to help with that.

@Pururun what do you think about doing something like this? 3c7ab1b

[albin-mullvad]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dlon)

---

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadDaemon.kt line 33 at r1 (raw file):

Previously, albin-mullvad wrote…

@Pururun what do you think about doing something like this? 3c7ab1b

The above commit should probably be require(value != 0) rather than require(value > 0) if we decide to keep it, but we're now looking at removing the handle completely instead

[MarkusPettersson98]

Reviewed 5 of 6 files at r2, all commit messages.
Reviewable status: 11 of 12 files reviewed, 6 unresolved discussions (waiting on @dlon and @Pururun)

---

mullvad-jni/src/lib.rs line 26 at r2 (raw file):

const LOG_FILENAME: &str = "daemon.log";

static DAEMON_CONTEXT: Mutex<Option<DaemonContext>> = Mutex::new(None);

Nit Would be a nice with a small doc-comment explaining that DAEMON_CONTEXT is initialized/deinitialized with .._init and .._shutdown respectively 😊

Code quote:

static DAEMON_CONTEXT: Mutex<Option<DaemonContext>> = Mutex::new(None);

---

mullvad-jni/src/lib.rs line 82 at r2 (raw file):

    .map(|daemon| {
        *ctx = Some(daemon);
    })

Nit I don't really like the use of map to access the inner value of a Result in order to only perform a side-effect. Since we're not transforming the data contained in the Result from start, I think a plain match-statement is more suitable here

match start(
    env,
    vpnService,
    rpcSocketPath,
    filesDirectory,
    cacheDirectory,
    apiEndpoint,
) {
    Ok(daemon) => {
        *ctx = Some(daemon);
    }
    Err(err) => {
        env.throw(err.to_string())
            .expect("Failed to throw exception");
    }
};

Code quote:

    .map(|daemon| {
        *ctx = Some(daemon);
    })

[Pururun]

Reviewed all commit messages.
Reviewable status: 11 of 12 files reviewed, 6 unresolved discussions (waiting on @dlon and @MarkusPettersson98)

---

mullvad-jni/src/lib.rs line 4 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

I think HashSet can safely be removed from classes.rs, no? Is it still used on the Android side? @Pururun

I think it is used by the connectivity listener that does some jni communication with the daemon?

[Pururun]

Reviewable status: 11 of 12 files reviewed, 6 unresolved discussions (waiting on @dlon and @MarkusPettersson98)

---

mullvad-jni/src/lib.rs line 4 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

I think it is used by the connectivity listener that does some jni communication with the daemon?

There is not jni communication using HashSet in connectivity listener, so this should be safe to remove.

[albin-mullvad]

Reviewable status: 11 of 12 files reviewed, 5 unresolved discussions (waiting on @dlon, @MarkusPettersson98, and @Pururun)

---

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadDaemon.kt line 33 at r1 (raw file):

Previously, albin-mullvad wrote…

The above commit should probably be require(value != 0) rather than require(value > 0) if we decide to keep it, but we're now looking at removing the handle completely instead

This comment is no longer relevant since we're now going in more of a singleton direction

[albin-mullvad]

Reviewable status: 11 of 12 files reviewed, 6 unresolved discussions (waiting on @dlon, @MarkusPettersson98, and @Pururun)

---

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadDaemon.kt line 30 at r2 (raw file):

        migrateSplitTunneling.migrate()

        init(

nit: I suggest naming something else than init since it can be confused with kotlin init blocks

Code quote:

init

[dlon]

Reviewable status: 8 of 13 files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98 and @Pururun)

---

mullvad-jni/src/lib.rs line 4 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

There is not jni communication using HashSet in connectivity listener, so this should be safe to remove.

Removed.

---

mullvad-jni/src/lib.rs line 84 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

IMHO, I think boxing the DaemonContext is a concern of this function and not start. So if we could both box the DaemonContext and turn it into a raw pointer here that would be great :)

Agreed, but no longer relevant since the handle is gone.

---

mullvad-jni/src/lib.rs line 26 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Nit Would be a nice with a small doc-comment explaining that DAEMON_CONTEXT is initialized/deinitialized with .._init and .._shutdown respectively 😊

Done.

---

mullvad-jni/src/lib.rs line 82 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Nit I don't really like the use of map to access the inner value of a Result in order to only perform a side-effect. Since we're not transforming the data contained in the Result from start, I think a plain match-statement is more suitable here

match start(
    env,
    vpnService,
    rpcSocketPath,
    filesDirectory,
    cacheDirectory,
    apiEndpoint,
) {
    Ok(daemon) => {
        *ctx = Some(daemon);
    }
    Err(err) => {
        env.throw(err.to_string())
            .expect("Failed to throw exception");
    }
};

Done. (Kind of. Doesn't apply anymore.)

[albin-mullvad]

Reviewed 1 of 8 files at r1, 1 of 6 files at r2, 1 of 5 files at r3.
Reviewable status: 9 of 13 files reviewed, 1 unresolved discussion (waiting on @dlon, @MarkusPettersson98, and @Pururun)

---

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadDaemon.kt line 1 at r2 (raw file):

package net.mullvad.mullvadvpn.service

Amazing improvement, good job! 🤩

[Pururun]

Reviewed 4 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dlon)

---

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadDaemon.kt line 46 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

We should check if withContext can be safely removed or not.

No longer relevant

---

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadVpnService.kt line 200 at r3 (raw file):

        // Shutting down the daemon gracefully
        MullvadDaemon.shutdown()

I think this is fine for now, but we should keep in mind if we find any issues in the future that we might want to wait for the daemon to shutdown before destroying the service.

[albin-mullvad]

Android/Kotlin side looks good to me!

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

---

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadVpnService.kt line 200 at r3 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

I think this is fine for now, but we should keep in mind if we find any issues in the future that we might want to wait for the daemon to shutdown before destroying the service.

Yeah, I think this is fine. Let's focus on testing it thourougly before and after merge. Also worth mentioning is that there are regardless no guarantees that the system will let the onDestroy complete if I recall correctly

[Rawa]

👏 great work!

Reviewed 2 of 2 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

---

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadDaemon.kt line 1 at r2 (raw file):

Previously, albin-mullvad wrote…

Amazing improvement, good job! 🤩

Indeed, love it!

[MarkusPettersson98]

So good! 🚀 ⭐

Reviewed 2 of 5 files at r3, 1 of 2 files at r4, 2 of 2 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

---

mullvad-jni/src/lib.rs line 84 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Agreed, but no longer relevant since the handle is gone.

🙌

[albin-mullvad]

Reviewed 2 of 3 files at r8, all commit messages.
Reviewable status: 12 of 13 files reviewed, all discussions resolved (waiting on @MarkusPettersson98 and @Rawa)

[MarkusPettersson98]

Reviewed 1 of 3 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved