812700 - SCTP code doesn't yet build on Android

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Description

[Dan Mosedale (:dmosedale, :dmose)]

A bunch of the files in netwerk/sctp blow up when built for Android.  Discussion with jesup suggests there are likely problems with, at the very least, the include files.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Note that the patch in bug 750869 landing is required to get far enough to see this behavior, so if it hasn't yet landed, it belongs first the patch queue.

Comment 2

[Gian-Carlo Pascutto [:gcp]]

I got a bunch of small fixes for bionic oddities, but then ran into this:
netwerk/sctp/src/netinet/sctp_bsd_addr.c requires getifaddr but bionic libc doesn't supply it at all. We'll probably have to ship our own implementation.

Comment 3

[Gian-Carlo Pascutto [:gcp]]

Android has an implementation here, but it's partly C++:
libcore/luni/src/main/native/ifaddrs-android.h

Comment 4

[Gian-Carlo Pascutto [:gcp]]

pthread_rwlock is also missing in Android API Level <= 8. But IIRC we already require 9 elsewhere so I'll just bump the android-version= in mozconfig.

Comment 5

[Gian-Carlo Pascutto [:gcp]]

media/webrtc/signaling/src/sipcc is trying to use IPC message queues, which don't exist in Android at all. It looks like the darwin implementation works with basic pthread, so I'll try to swap that in on Android.

Comment 6

[Gian-Carlo Pascutto [:gcp]]

Attachment: WIP. Fixes for Android WebRTC build

Patches so far. I accidentally folded with bug 750869 so this is the only thing you need on top of m-c.

Comment 7

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. Fix SCTP code

Cleaned up and unfolded, but still needs to go on try to see if I didn't break other platforms.

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 683941 [details] [diff] [review]
Patch 1. Fix SCTP code

Review of attachment 683941 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/sctp/src/user_inpcb.h
@@ +35,5 @@
>  
>  #include <user_route.h> /* was <net/route.h> */
> +#if defined(__Userspace_os_Linux)
> +#include <arpa/inet.h>
> +#include <netinet/in.h>

The include structure is fragile, and has to work on both modern and ancient (ok, RHEL5) linux versions used on the builders.  Definitely needs Try and also Fedora/Ubuntu tests.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #4)
> pthread_rwlock is also missing in Android API Level <= 8. But IIRC we
> already require 9 elsewhere so I'll just bump the android-version= in
> mozconfig.

https://bugzilla.mozilla.org/show_bug.cgi?id=750869#c8 seems to suggest that we don't want to bump the API if we can avoid it.  IIRC, the WebRTC code has its own threading implementation?  If that's true, I wonder if we could take advantage of that instead...

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

(Assuming, of course, that it doesn't just wrap pthreads).

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

I believe it does wrap pthreads on linux, sorry.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Good times.

To get SCTP build against ndk r5 also requires turning off IPv6 support (i.e. commenting out -DINET6 in netwerk/sctp/src/Makefile.in).  What percentage of potential users are likely to want to use WebRTC over IPv6 on Android, I have no idea. I could certainly imagine coming back around to IPv6 later, though.

Ted, https://bugzilla.mozilla.org/show_bug.cgi?id=750869#c8 is a bit vague on how painful it would be to accept a version bump, in the sense that it's not clear whether we'd just have to live without some automated testing for a while, or if that could be fixed, or ....

Comment 13

[Gian-Carlo Pascutto [:gcp]]

One of you pointed out on IRC that we have our own generic rwlock somewhere for platforms that don't have it, so the pthread side of this shouldn't be all that much of a problem.

Bumping the minimum required version would mean leaving users behind. For fundamental reasons this may be unavoidable eventually, but just a rwlock isn't a very good reason.

Comment 14

[(not currently active) Ted Mielczarek]

(In reply to Dan Mosedale (:dmose) from comment #12)
> Ted, https://bugzilla.mozilla.org/show_bug.cgi?id=750869#c8 is a bit vague
> on how painful it would be to accept a version bump, in the sense that it's
> not clear whether we'd just have to live without some automated testing for
> a while, or if that could be fixed, or ....

It's a non-starter at the moment. We're just getting pandaboards online for automated testing, but we don't have enough capacity to replace the Tegras. (In addition, this would need signoff from someone in Product, since we'd be leaving a bunch of users behind.)

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Ok, for the moment let's turn off IPV6 on Android and keep going, and come back to that later (file a bug).

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Patch 1, v3. Fix SCTP code to build.

Turns out that even with the target platform version set to 9, the compile still blows up not finding IPv6 headers when building against NDK r5c, which is what the automated builders are using, according to <https://wiki.mozilla.org/Mobile/Fennec/Android#Install_Android_NDK_2>.  So here's a new patch rev that turns off IPv6 on Android for now.

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Patch 1, v4. Fix SCTP code to build.

Fix include structure fragility noted by jesup in comment 8.  Next steps: try server runs and review.

Comment 18

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1, v5. Fix SCTP code to build.

Updated. Later patches use Userspace_os_Android, so we'll do it here too. It avoids some hairy situations where Android needs extra includes that will end up conflicting on real Linux.

Try of the entire stack:
https://tbpl.mozilla.org/?tree=Try&rev=c0c108719370

Comment 19

[(not currently active) Ted Mielczarek]

Comment on attachment 688262 [details] [diff] [review]
Patch 1, v5. Fix SCTP code to build.

Review of attachment 688262 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/sctp/datachannel/DataChannel.cpp
@@ +374,5 @@
>    struct sockaddr_conn addr;
>  
>    memset(&addr, 0, sizeof(addr));
>    addr.sconn_family = AF_CONN;
> +#if !defined(__Userspace_os_Linux) && !defined(__Userspace_os_Windows) && !defined(__Userspace_os_Android)

What actually uses this code at this point, just Mac? Maybe this should just be a test for Mac...

::: netwerk/sctp/datachannel/Makefile.in
@@ +57,5 @@
>  ifeq ($(OS_TARGET),FreeBSD)
>  DEFINES += -D__Userspace_os_FreeBSD=1
>  else
>  #default_fallback; probably doesn't work
>  DEFINES += -D__Userspace_os_$(OS_TARGET)=1

Seems like we could just use this version except for Windows, which would get rid of this horrible chain of ifeqs.

::: netwerk/sctp/src/Makefile.in
@@ +71,5 @@
>    -I$(topsrcdir)/xpcom/ds \
>    $(NULL)
>  
> +# Android NDK r5c, used on the builders at the time of this writing, doesn't
> +# have the headers we need for IPv6

Hm, we don't have a Makefile var for the NDK version, do we?

@@ +107,5 @@
>  DEFINES += -D__Userspace_os_Linux=1
>  else
> +ifeq ($(OS_TARGET),Android)
> +DEFINES += -D__Userspace_os_Linux=1
> +else

You can fold these two together, like:
ifeq (,$(filter-out Linux Android,$(OS_TARGET))
DEFINES += ...

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Patch 1, v6. Fix SCTP code to build.

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Patch 1, v7. Fix SCTP code to build.

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #19)

v7 of the patch folds in all of Ted's comments other than this one:
 
> ::: netwerk/sctp/src/Makefile.in
> @@ +71,5 @@
> >    -I$(topsrcdir)/xpcom/ds \
> >    $(NULL)
> >  
> > +# Android NDK r5c, used on the builders at the time of this writing, doesn't
> > +# have the headers we need for IPv6
> 
> Hm, we don't have a Makefile var for the NDK version, do we?

It seems pretty clear from reading android.mk that we don't.  :/

Comment 23

[Dan Mosedale (:dmosedale, :dmose)]

Hmm, current patch doesn't actually build; I'm gonna need to poke at this a bit more.

Comment 24

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Patch 1, v8. Fix SCTP code to build.

OK, here's a version that does work.  I suspect (but don't know for sure) that the problems I was hitting were an interaction with changes to the code in mozilla-inbound.

After discussion with Jesup, he suggested not having a separate __Userspace_os_Android, and instead just treating Android as __Userspace_os_Linux, and conditionalizing on the ANDROID symbol in the few cases where we need to differentiate.

So here's a patch that does that.

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Patch 1, v8. Fix SCTP code to build.

Let's try that again; I forgot to do a final qrefresh before uploading.  But this time for sure!

Comment 26

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Patch 1, v9. Fix SCTP code to build.

Turns out that since we're letting of __Userspace_os_Android, I had to partially undo one of Ted's suggested cleanups.  Sure would be nice to someday just kill the extra __Userspace_os crud, as it seems like it just adds unnecessary complexity.

Comment 27

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 690247 [details] [diff] [review]
Patch 1, v9. Fix SCTP code to build.

Review of attachment 690247 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/sctp/src/LocalArray.h
@@ +1,2 @@
> +/*
> + * Copyright (C) 2009 The Android Open Source Project

To import new 3rd party code requires emailing <[e-mail]>, a la <http://www.mozilla.org/MPL/license-policy.html>.  Given that this is a known-compatible license, my hope is that we'll get quick approval here.  If you could do, that'd be great.

::: netwerk/sctp/src/user_recv_thread.c
@@ +444,5 @@
>  	ExitThread(0);
>  #else
>  	pthread_exit(NULL);
>  #endif
> +	return NULL;

Since these are going to effect cross-platform code, I'm curious to know what the reasoning behind adding the |return NULL|s is.  Please explain.

::: netwerk/sctp/src/user_socketvar.h
@@ +36,5 @@
>  #if defined(__Userspace_os_Darwin)
>  #include <sys/types.h>
>  #include <unistd.h>
>  #endif
> +#if defined(ANDROID)

The fact that we need to include a Coda filesystem header in order to get a typedef that really wants to live in <sys/types.h> is just bizarre.  Can you add a comment here describing what's going on?

Comment 28

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1, v10. Fix SCTP code to build.

>If you could do, that'd be great.

I already got approval for adding APL 2.0 licensed Android code previously (bug 729457), just sent licensing@ a note for this bug too.

Bug 818843 might have a slightly simpler getifaddr fix, but it doesn't specify where the code came from, is missing copyright attribution and I couldn't find it in my Android tree (which is admittedly not entirely up to date). We can consider switching to that one if that bug lands.

>Since these are going to effect cross-platform code, I'm curious to know what 
>the reasoning behind adding the |return NULL|s is.  Please explain.

Android GCC won't compile a function that is defined returning void* without them. But you'll never actually get there, as pthread_exit (or equivalent) will not return.

>Can you add a comment here describing what's going on?

OK.

Comment 29

[Dan Mosedale (:dmosedale, :dmose)]

Gerv also commented in private email indicating that the licensing folks weren't tracking APL2/Android code on a per-directory basis, so we're fine to land that stuff here and elsewhere.

Comment 30

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 690411 [details] [diff] [review]
Patch 1, v10. Fix SCTP code to build.

r=dmose; carrying forward feedback+ from ted.

Comment 31

[Dan Mosedale (:dmosedale, :dmose)]

Tryserver run currently in-progress here: https://tbpl.mozilla.org/?tree=Try&rev=111902d19a12

Comment 32

[Dan Mosedale (:dmosedale, :dmose)]

Landed on mozilla-inbound:

<https://hg.mozilla.org/integration/mozilla-inbound/rev/226ffef01c84>

Comment 33

[Dan Mosedale (:dmosedale, :dmose)]

Setting in-testsuite-, as this is a build patch: the compiler is the test suite.

Comment 34

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/226ffef01c84