Android Traceroute #572

ssawchenko · 2018-05-10T23:48:48Z

Ok so, a looooooong time in the making. While I am certain there are still many issues with this, I was hoping to start a code review / round of testing to give me a good sanity check on what may be remaining with this feature.

Note, like... 18k files changed due to me removing a redundant boost library and creating an External-Code-Android folder to shelter Android from some problematic libraries. It may be easier to pull the code down and review it in an IDE rather than the diff window here due to that.

Summary from the initial ticket:

I was able to use https://android.googlesource.com/platform/external/iputils/+/master/tracepath.c as a bit of a guide to get ICMP packets sending (albeit over UDP) with incrementing TTLs and then read out the intermediate IPs at each hop. I believe this pretty much solidifies the validity of running a traceroute/path like function programatically in android.

Known issues:

Random crashes on Android 8.0. (see git issue for more details)

* Removed hard coded netinet folder, now using platform dependent libs * Added basic ping to MapControllerWrapper JNI

* Moving to separate include dir as the netinet library iOS is using was causing conflicts with new Android traceroute attempts

…ions etc...

…g results

…estinationAddressWithTTL result

…rom currently selected node

* Had previously deleted netinet for an Android attempt; adding back to project fixed build issue.

abey · 2018-05-30T19:46:30Z

Tried to connect to the same bright spot in Romania(?). Android and iOS shows different data for some reason. Android doesn't seem to display the yellow line to destination

iOS

Android

ssawchenko · 2018-05-30T21:48:52Z

@nbrooke I have noticed some differences in the resulting path on iOS and Android at times, however, I have been comparing my Android trace with both a traceroute on my laptop and using a traceroute Android app I found in the play store, and my results have been almost spot on in comparison with those.

apike · 2018-06-14T17:27:09Z

Testing this build it seems to work pretty well – we should get this wrapped up and released soon, so action plan:

@ssawchenko to fix up the commented code and straightforward code review comments
It sounds like Shayla needs to chat with Nigel about a couple of the more "C++y" issues here (memory leaks in particular) and decide whether it's worth getting some of Nigel's time to help fix them.

nbrooke · 2018-06-19T20:06:33Z

Android-NDK/jniapi.cpp

+    //free up the strings
+    jenv->DeleteLocalRef(from);
+    //oh, we need to free this too
+    jenv->DeleteLocalRef(probeWrapperClass);


Based on the local / global ref docs I found when validating the probeWrapper behaviour (https://docs.oracle.com/javase/6/docs/technotes/guides/jni/spec/design.html#wp1242), I don't think these calls are required. These should be freed automatically when this function returns.

* MapControllerWrapper and TracerouteUtil singletons; can now check isRunning to see if traceroute is running * Do not start a new trace until the previous has finished * Correctly clean up Timers for previous traceroutes * Do not reuse probeWrapper * More efficient string building

…r trace started

ssawchenko · 2018-07-03T19:20:09Z

@abey Would you be able to run another round of testing on https://app.nevercode.io/#/project/4e922557-91dd-4635-8a56-bcbff3f9b239/build/cd2d9744-078f-4d62-973f-d1fc1c343429

@nbrooke Could you check the revisions I have made based on your suggestions?

abey · 2018-07-03T21:55:18Z

@ssawchenko Some comments:

If you tap the clock(?) icon to select the time range and tap any nodes on the globe, it will crash the app.
Android doesn't seem to have a lot of "ASN Hops" compared to iOS, I think this is related to differences in path finding in iOS and Android.
There are lines below a few labels in the iOS app (screenshot), I think we should add them in Android as well. Also, the text size seems to be larger on Android vis-a-vis iOS.

apike · 2018-07-05T22:24:48Z

@abey The crash blocks merging this but the other items don't really, can you break those other two into separate issues?

abey · 2018-07-05T22:30:59Z

Filed #576 and #577 for the other issues.

apike · 2018-07-11T23:23:03Z

I think this is currently waiting on the crash fix Abey found above.

ssawchenko · 2018-07-11T23:30:34Z

Will investigate crash first thing tomorrow.

ssawchenko · 2018-07-12T22:52:19Z

@nbrooke @abey I have made the following changes:

Fixed the crash occurring when nodes were selected in timeline mode.
The destination node is now always added as the "end" if we have not reached it; traceroute highlights on iOS and Android should look much more similar now. (may fix Pathfinding is different on Android vis-a-vis iOS #576?)

@abey Would you be able to give this one more round of testing?
https://app.nevercode.io/#/project/4e922557-91dd-4635-8a56-bcbff3f9b239/build/528de277-ce09-4d65-87ea-8940ac914c65

abey · 2018-07-12T23:20:04Z

@ssawchenko Looks good. 👍

Just one thing though. I think the time-out value is different in android and iOS. After the last IP Hop, android seems to take a lot more time compared to iOS. For example: I did a trace-route to the same location where android took 9264 ms while iOS took 3971 ms. I tried it with a few other locations and the trend was same.
Filed issues #578 , #579 & #580.

ssawchenko · 2018-07-12T23:41:48Z

@abey I think there may be a difference in how each platform handles the hop timeout. Looks like on Android we are waiting one second per packet request, and then sending out a max of 3 probes. So we will be waiting at least the 3 seconds extra there when we reach an IP that is not returning us a response.

@apike I could spend more time digging into where this time is being spent exactly, however, my gut says it's not a super important issue at the moment. Thoughts?

apike · 2018-07-19T21:46:48Z

Sounds like this is good to merge!

ssawchenko · 2018-07-20T17:54:42Z

Not going to lie, squashing and merging that number of file changes makes me a little nervous XD

nbrooke · 2018-07-20T17:56:21Z

@ssawchenko Yeah, this seems like a change that might be worth doing an old fashioned regular merge on just to be on the safe side.

ssawchenko · 2018-07-26T00:37:55Z

Discussed. Going to squash to reduce verbosity of commit history.

ssawchenko added 27 commits May 8, 2018 12:39

Basic ICMP Ping working

dc1a549

* Removed hard coded netinet folder, now using platform dependent libs * Added basic ping to MapControllerWrapper JNI

Now reading response ip

3f27d87

Removed redundant boost library; renamed to External-Code-Android

0bfd8f6

* Moving to separate include dir as the netinet library iOS is using was causing conflicts with new Android traceroute attempts

temp commit

1662e43

temp

8ab47e8

Attempting to fix looping logic; adding more comments

4f59088

Moved tracepath code into separate tracepath.cpp/h files

a8c72ae

temp

2f98378

Traceroute UI support; added trace route view with list, map translat…

d8591f9

…ions etc...

Temp commit, attempting to getprobeDestinationAddressWithTTL returnin…

908b10b

…g results

Created ProbeWrapper to be passed back as mapControllerWrapper.probeD…

24ea0c2

…estinationAddressWithTTL result

Moved trace code into NodePopup

b013432

Added highlightRoute to JNI; still in progress

cc0a10b

Process hops in order

b91fbc0

Updated getIPsFromASN; traceroute now attempting to report on trace f…

8719ccd

…rom currently selected node

Clear lines, better error handling of unknown ASNs

088c68b

Attempting to pass back hop trip time from tracepath.cpp

68a9430

CommonCallback onFailure attempts to report network loss issues

f53a460

Fixed up Xcode project

d20c57b

* Had previously deleted netinet for an Android attempt; adding back to project fixed build issue.

Fixed issue where traceroute UI was getting reset at incorrect times

2879f3b

Fixed up build issues with latest NDK

3afe002

Elapsed time seems to work, small format changes

a608f02

Fixed crash if ASP IP was not found

de462d4

Fixed issue where trace could get stuck in a loop

a320625

Small code cleanup

f083186

Removed old MK files

78ab365

Added style for brand_button; traceroute button now styled

a8ff4d6

ssawchenko self-assigned this May 10, 2018

ssawchenko requested a review from nbrooke May 10, 2018 23:52

apike self-requested a review May 24, 2018 18:03

nbrooke reviewed Jun 19, 2018

View reviewed changes

ssawchenko added 7 commits June 27, 2018 17:34

Removed commented code

5b94ba7

Removed commented code; ported isInvalidOrPrivate method from iOS.

e8b84ab

Remove unused file

0f3d3e8

Fixed typo in isInvalidOrPrivate

b1ffd81

Removed traceroute destination global variables from jniapp.cpp

20485c7

Fixed crash that would occur if Node popup window closed quickly afte…

220a41e

…r trace started

nbrooke approved these changes Jul 3, 2018

View reviewed changes

nbrooke removed their assignment Jul 3, 2018

abey mentioned this pull request Jul 5, 2018

Pathfinding is different on Android vis-a-vis iOS #576

Open

ssawchenko added 2 commits July 12, 2018 15:14

Fixed crash when NodePopup showing on timeline (ie. no traceroute)

8031a17

Final destination highlight now added (to match iOS); small code cleanup

dd0313e

ssawchenko merged commit 7984db0 into master Jul 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Android Traceroute #572

Android Traceroute #572

ssawchenko commented May 10, 2018 •

edited

Loading

abey commented May 30, 2018 •

edited

Loading

ssawchenko commented May 30, 2018

apike commented Jun 14, 2018

nbrooke Jun 19, 2018

ssawchenko commented Jul 3, 2018

abey commented Jul 3, 2018 •

edited

Loading

apike commented Jul 5, 2018

abey commented Jul 5, 2018

apike commented Jul 11, 2018

ssawchenko commented Jul 11, 2018

ssawchenko commented Jul 12, 2018 •

edited

Loading

abey commented Jul 12, 2018 •

edited

Loading

ssawchenko commented Jul 12, 2018

apike commented Jul 19, 2018

ssawchenko commented Jul 20, 2018

nbrooke commented Jul 20, 2018

ssawchenko commented Jul 26, 2018

Android Traceroute #572

Android Traceroute #572

Conversation

ssawchenko commented May 10, 2018 • edited Loading

abey commented May 30, 2018 • edited Loading

ssawchenko commented May 30, 2018

apike commented Jun 14, 2018

nbrooke Jun 19, 2018

Choose a reason for hiding this comment

ssawchenko commented Jul 3, 2018

abey commented Jul 3, 2018 • edited Loading

apike commented Jul 5, 2018

abey commented Jul 5, 2018

apike commented Jul 11, 2018

ssawchenko commented Jul 11, 2018

ssawchenko commented Jul 12, 2018 • edited Loading

abey commented Jul 12, 2018 • edited Loading

ssawchenko commented Jul 12, 2018

apike commented Jul 19, 2018

ssawchenko commented Jul 20, 2018

nbrooke commented Jul 20, 2018

ssawchenko commented Jul 26, 2018

ssawchenko commented May 10, 2018 •

edited

Loading

abey commented May 30, 2018 •

edited

Loading

abey commented Jul 3, 2018 •

edited

Loading

ssawchenko commented Jul 12, 2018 •

edited

Loading

abey commented Jul 12, 2018 •

edited

Loading