Fixes for building with Android NDK r5

RESOLVED FIXED

Status

()

Core
Build Config
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: mbrubeck, Assigned: azakai)

Tracking

({footprint, mobile, perf})

Trunk
All
Android
footprint, mobile, perf
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 13 obsolete attachments)

2.98 KB, patch
ted
: review+
blassey
: feedback+
Details | Diff | Splinter Review
45.12 KB, patch
blassey
: review+
Details | Diff | Splinter Review
23.67 KB, patch
ted
: review+
Details | Diff | Splinter Review
892 bytes, patch
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
Created attachment 495603 [details] [diff] [review]
WIP

The new Android NDK breaks the Mozilla build in several places.

This work-in-progress patch fixes some of the errors, but not all.  See also bug 617074.
(Reporter)

Comment 1

7 years ago
Created attachment 496262 [details] [diff] [review]
WIP 2

This fixes some issues with the "signbit" and "isinf" functions in math.h.

Currently the stlport-enabled build fails in some GCC-specific hash_map code in chromium.
Assignee: nobody → mbrubeck
Attachment #495603 - Attachment is obsolete: true
Created attachment 500755 [details] [diff] [review]
WIP 3

this builds and runs (as long as you disable crash reporter), however it crashes soon after start up. The stack just has memcpy and then garbage.
(Reporter)

Updated

7 years ago
Assignee: mbrubeck → nobody
(In reply to comment #1)
> Created attachment 496262 [details] [diff] [review]
> WIP 2
> 
> This fixes some issues with the "signbit" and "isinf" functions in math.h.

I hit these signbit issues with g++ 4.5 with -std=c++0x. While trying different things to fix it, I hit another "fun" issue that your patch will have too: the current Android SDK used on build bots doesn't support std::signbit:
/builds/slave/try-mb-br-andrd-r7-bld/build/try/js/src/jsvalue.h:94: error: 'std::signbit' has not been declared
Depends on: 640494
Created attachment 523726 [details] [diff] [review]
WIP patch v4

this patch gets us to build and run, but we crash 1-2s after about:home loads
Attachment #496262 - Attachment is obsolete: true
Attachment #500755 - Attachment is obsolete: true
Created attachment 523727 [details] [diff] [review]
nspr patch

nspr patch split out because configure changes make it huge and ugly
if I comment out this line we don't crash on startup:
https://mxr.mozilla.org/mobile-browser/source/chrome/content/browser-ui.js#513

which implies that starting the content process is what's crashing the chrome process
(Reporter)

Updated

6 years ago
Keywords: footprint, perf
Assignee: nobody → azakai
(Assignee)

Comment 7

6 years ago
I installed the new NDK, updated my mozconfig, but it fails on 'C compiler cannot create executables', which happens because build/prebuilt does not exist in the downloaded NDK r5. I don't see anything about creating prebuilt/ in our build docs or in the NDK build docs (everything says "unpack and it's ready to use", basically).

I guess I must be missing something simple here?
(Assignee)

Comment 8

6 years ago
blassey informed me that some additional mozconfig changes are needed,

ac_add_options --with-android-ndk=".../android-ndk-r5"
ac_add_options --with-android-sdk=".../android-sdk-linux_86/platforms/android-8"
ac_add_options --with-android-version=5
ac_add_options --with-android-tools=".../android-sdk-linux_86/tools"
ac_add_options --with-android-toolchain=".../android-ndk-r5/toolchains/arm-linux-androideabi-4.4.3/prebuilt/linux-x86"
ac_add_options --with-android-platform=".../android-ndk-r5/platforms/android-5/arch-arm"
(Assignee)

Comment 9

6 years ago
(In reply to comment #6)
> if I comment out this line we don't crash on startup:
> https://mxr.mozilla.org/mobile-browser/source/chrome/content/browser-ui.
> js#513
> 

The line there is

Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime)
                                 .ensureContentProcess();

Turns out that what is failing is getting that service. More specifically, getting that service leads to getting the AppShell service, which tries to get the prefService, which tries to add an observer in the observerService - which fails. The failure is due to "Using observer service off the main thread!". Which seems very odd.
(Assignee)

Comment 10

6 years ago
It looks like there is a problem with TLS.

We set the TLS to mark the main thread in nsThreadManager::Init, then when we start the cycle collector thread in nsCycleCollector_startup, the CC thread mark its TLS, after which the TLS on the main thread is corrupted (1299198128, looks random). As a result, we always think we are not the main thread, even when we are, and everything breaks.

Looking in the r5 docs, I see that OVERVIEW.html says

> At the moment, thread-local storage defined through the __thread compiler keyword is not supported by the Bionic C library and dynamic linker.

There is no corresponding file in r4 to compare to.
(Assignee)

Comment 11

6 years ago
Created attachment 532081 [details] [diff] [review]
do not use TLS

blassey found that this was a configure issue. His patch here makes configure not make the mistake of thinking TLS will work.

After that, fennec loads, but the child process is in a frozen state. The cause is apparently many of these:

I/Gecko   ( 6004): WARNING: NS_ENSURE_TRUE(mMainThread) failed: file /home/alon/Dev/mozilla-central/xpcom/threads/nsThreadManager.cpp, line 280

I/Gecko   ( 6004): WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0xC1F30001: file /home/alon/Dev/android/fennec/xpcom/build/nsThreadUtils.cpp, line 173


- NS_DispatchToMainThread fails, since NS_ENSURE_TRUE(mMainThread, NS_ERROR_NOT_INITIALIZED); fails in NS_GetMainThread. So nothing is dispatched and the child process is a zombie. Looks like even with no TLS, something went very wrong in initializing nsThreadManager.
(Assignee)

Comment 12

6 years ago
The parent process sets up the threadManager, and things like NS_GetMainThread succeed. Then it forks the child, but in XRE_InitChildProcess calling NS_GetMainThread fails, since mMainThread is not initialized as mentioned before. Which seems odd as a fork of a process in which that worked ok.

However, we do some tricky things with how we fork and launch the child process, and I am not sure I follow all the steps. Does anyone know, is it possible to not get an identical copy in the forked child for some reason?
(Assignee)

Comment 13

6 years ago
Further checks show that fork() is not broken - right after the fork, we do have the proper values in memory in the child process.

However, after we fork we do an execvp(), which AFAIK loads a new binary in place of the old one ("The exec() family of functions replaces the current process image with a new process image" the docs say). After we do that, we have the uninitialized values in memory (so, the threadManager is no longer initialized, and things fail). This seems expected though - we are calling LaunchApp after all, and it looks like it just uses fork() as an intermediate tool. So I guess I do not understand how things worked for us before.

Did we rely on some trick with execvp to keep part of ourselves in memory?
(Assignee)

Comment 14

6 years ago
Some poking around shows we do need to initialize XPCOM in the child process. So that it wasn't happening was a problem. Which was caused by the crashreporter stuff in ContentChild.cpp, commenting that out lets things proceed a little further.

The symptoms remain the same, though - frozen unresponsive child process, even with XPCOM and so forth set up properly. Plenty of odd warnings and assertions though, including lots of 'potential deadlock' stuff. Not sure how much of that is expected.

Side issue: I think that crashreporter code in ContentChild.cpp should be enclosed in #ifdef MOZ_CRASHREPORTER? Or is it intentionally run even without the crashreporter being enabled?
(Assignee)

Comment 15

6 years ago
I worked around the deadlock, which did not fix anything. I also compared the warnings and assertions to linux desktop, and they all seem unrelated to the problem.
(Assignee)

Comment 16

6 years ago
Created attachment 532394 [details] [diff] [review]
do not send crash reports without crashreporter

Patch to not try to send crash reports if crash reporter is not enabled.

With this patch and the other 3, plus a clobber build (apparently necessary...), things work ok with NDK5!
(Assignee)

Comment 17

6 years ago
Created attachment 532773 [details] [diff] [review]
Part 5: crashreporter fixes

With this patch, we build with crashreporter enabled.

I had to hack in two changes inside google-breakpad code, which I am assuming is not a good thing. I don't immediately see an alternative, though. (Perhaps newer version of breakpad have these fixes anyhow?)
Attachment #532773 - Flags: feedback?(blassey.bugs)
(Assignee)

Comment 18

6 years ago
blassey tells me that ndk5 builds are slower than ndk4 ones. As an initial step to investigating this, I compared the build commands the ndks generate. The differences are:

1. ndk5 adds an -Include for stlport
2. ndk4 has |-mfpu=vfp|
(In reply to comment #18)

> 2. ndk4 has |-mfpu=vfp|

IIRC, this made a big change in performance when Vlad switched to use it.
(Assignee)

Comment 20

6 years ago
tl;dr looks like NDK5 gives us 15% faster SunSpider and 12% faster V8.

I ran some benchmarks on SunSpider and V8 (not Kraken, since it takes too long to load over wifi), on a Galaxy S:

                     SS      V8
NDK4               2.705   0.505
NDK5               3.110   0.560
NDK5 mfpu=vfp      3.115   0.570
NDK5 config mfpu   3.055   0.565

Run on dromaeo.com. Results are runs/sec so more is better. Each number is the average of two full runs.

Seems to be no difference in the NDK5 variants. The first is just plain, the second is when hacking -mfpu=vfp into the build commands (NDK4 had it by default, NDK5 not, hence the interest in what it does). The last is a configure.in fix from blassey that is related to the mfpu stuff and looks like the correct thing to do (fixes *-android* to *android*, like the other configure.in fix we have).

There are some significant changes in .apk size:

NDK4             15.25MB
NDK5             15.88MB
NDK5 mfpu=vfp    20.69MB (!)
NDK5 config mfpu 15.88MB

So the apparently right option (the last) means we increase the APK size by 4% (also the same amount by which libxul.so increased). Note that I had to do

ac_add_options --disable-elf-hack

for the NDK5 builds. Can this perhaps be related to the size increase?

Comment 21

6 years ago
(In reply to comment #20)
> ac_add_options --disable-elf-hack
> 
> for the NDK5 builds. Can this perhaps be related to the size increase?

Likely. Do a NDK4 build without elf hack to compare.
(In reply to comment #20)
> Note that I had to do
> 
> ac_add_options --disable-elf-hack
> 
> for the NDK5 builds.

What kind of problem did you have?
(Assignee)

Comment 23

6 years ago
(In reply to comment #22)
> (In reply to comment #20)
> > Note that I had to do
> > 
> > ac_add_options --disable-elf-hack
> > 
> > for the NDK5 builds.
> 
> What kind of problem did you have?

Hmm I had that on my laptop earlier while figuring out other build problems. I can't reproduce it now on the build machine with the latest patches.

Ok, a build with the ELF hack is 15.70MB, which is 3% bigger than NDK4 (compared to 4% bigger without the ELF hack). So a slight improvement but not all of it. Maybe worth it though.
(Assignee)

Comment 24

6 years ago
Created attachment 533132 [details] [diff] [review]
Part 3: configure.in fixes

Updated configure.in patch. Fixes for both TLS and mfpu stuff.
Attachment #532081 - Attachment is obsolete: true
(In reply to comment #23)
> Ok, a build with the ELF hack is 15.70MB, which is 3% bigger than NDK4
> (compared to 4% bigger without the ELF hack). So a slight improvement but
> not all of it. Maybe worth it though.

Could you attach libxul.so or put it somewhere? It might be a case of the new toolchain making it difficult for elfhack to do its job.
(Assignee)

Comment 26

6 years ago
(In reply to comment #25)
> (In reply to comment #23)
> > Ok, a build with the ELF hack is 15.70MB, which is 3% bigger than NDK4
> > (compared to 4% bigger without the ELF hack). So a slight improvement but
> > not all of it. Maybe worth it though.
> 
> Could you attach libxul.so or put it somewhere? It might be a case of the
> new toolchain making it difficult for elfhack to do its job.

Sure, here: http://www.syntensity.com/static/staging/libxul.so.tar.bz2
Created attachment 533410 [details] [diff] [review]
patch to turn on vfp and fix building with ndk4

this patch fixes building with ndkr4 and turns vfp back on
Created attachment 533417 [details] [diff] [review]
rolled up patch
Attachment #523726 - Attachment is obsolete: true
Attachment #532394 - Attachment is obsolete: true
Attachment #533132 - Attachment is obsolete: true
Attachment #533410 - Attachment is obsolete: true
Created attachment 533420 [details] [diff] [review]
rolled up patch
Attachment #533417 - Attachment is obsolete: true
Created attachment 533422 [details] [diff] [review]
rolled up patch
Attachment #533420 - Attachment is obsolete: true
Created attachment 533427 [details] [diff] [review]
rolled up patch
Attachment #533422 - Attachment is obsolete: true
Attachment #533427 - Flags: review?(ted.mielczarek)
Attachment #523727 - Flags: review?(ted.mielczarek)
Attachment #532773 - Flags: review?(ted.mielczarek)
Attachment #532773 - Flags: feedback?(blassey.bugs)
Attachment #532773 - Flags: feedback+
(Assignee)

Updated

6 years ago
Blocks: 658074
Depends on: 658246
(In reply to comment #26)
> (In reply to comment #25)
> > (In reply to comment #23)
> > > Ok, a build with the ELF hack is 15.70MB, which is 3% bigger than NDK4
> > > (compared to 4% bigger without the ELF hack). So a slight improvement but
> > > not all of it. Maybe worth it though.
> > 
> > Could you attach libxul.so or put it somewhere? It might be a case of the
> > new toolchain making it difficult for elfhack to do its job.
> 
> Sure, here: http://www.syntensity.com/static/staging/libxul.so.tar.bz2

There are around 300KB that could be stripped with elfhack, with R_ARM_ABS32 relocations not being supported (filed bug 658246). On the other hand, the reason why so much of these relocations are there is that libstdc++ is dynamically linked. I thought Android devices didn't come with a bundled libstdc++ ?
Ndk5 introduces libstdc++. You can either link to it statically or dynamically. I if you link to it dynamically you need to package the .so with your app. We are linking statically.
(In reply to comment #33)
> Ndk5 introduces libstdc++. You can either link to it statically or
> dynamically. I if you link to it dynamically you need to package the .so
> with your app. We are linking statically.

In the libxul.so azakai linked to, it is not linked statically.
(Assignee)

Updated

6 years ago
Attachment #532394 - Attachment is obsolete: false
(Assignee)

Updated

6 years ago
Attachment #532394 - Flags: review?(blassey.bugs)
(Assignee)

Updated

6 years ago
Attachment #532394 - Attachment is obsolete: true
Attachment #532394 - Flags: review?(blassey.bugs)
Attachment #532394 - Flags: review+
(Assignee)

Comment 35

6 years ago
Pushed the "do not send crash reports without crashreporter enabled on android" bit,

http://hg.mozilla.org/mozilla-central/rev/1f693cd5a613
Comment on attachment 523727 [details] [diff] [review]
nspr patch

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

::: nsprpub/build/autoconf/config.sub
@@ +1690,5 @@
>  				vendor=stratus
>  				;;
> +			*android*)
> +				vendor=linux-
> +				;;

We generally pull this whole file from upstream. I'm okay with taking local changes since we don't update it all that often, but can you try to submit these changes upstream?

::: nsprpub/configure.in
@@ +164,5 @@
> +    ;;
> +esac
> +
> +case "$target" in
> +*android*)

Can this be -*android*) (does that match everything you care about?)

@@ +1050,4 @@
>      RESOLVE_LINK_SYMBOLS=1
>      ;;
>          
> +*android*)

Same question here.
Attachment #523727 - Flags: review?(ted.mielczarek) → review+
(In reply to comment #36)
> Comment on attachment 523727 [details] [diff] [review] [review]
> nspr patch
> 
> Review of attachment 523727 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: nsprpub/build/autoconf/config.sub
> We generally pull this whole file from upstream. I'm okay with taking local
> changes since we don't update it all that often, but can you try to submit
> these changes upstream?

do you know where exactly to submit the patch?


> > +case "$target" in
> > +*android*)
> 
> Can this be -*android*) (does that match everything you care about?)
*-*android* should work. another option would be *-android*|*-linuxandroid*
(In reply to comment #37)
> > We generally pull this whole file from upstream. I'm okay with taking local
> > changes since we don't update it all that often, but can you try to submit
> > these changes upstream?
> 
> do you know where exactly to submit the patch?

Contact details are in the file header.

> *-*android* should work. another option would be *-android*|*-linuxandroid*

Either of those sound fine.
Comment on attachment 532773 [details] [diff] [review]
Part 5: crashreporter fixes

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

::: toolkit/crashreporter/Makefile.in
@@ +100,5 @@
>  
>  ifeq ($(OS_TARGET),Android)
>  DIRS += fileid
> +# NDK5 workarounds
> +DEFINES += -D_STLP_CONST_CONSTRUCTOR_BUG -D_STLP_NO_MEMBER_TEMPLATES

Can we just put these in configure instead of having to stick them in a couple of random Makefiles?

::: toolkit/crashreporter/google-breakpad/src/client/linux/handler/exception_handler.cc
@@ +98,5 @@
>  #include "client/linux/minidump_writer/minidump_writer.h"
>  #include "common/linux/guid_creator.h"
>  #include "common/linux/eintr_wrapper.h"
>  
> +#include "linux/sched.h"

These need to go upstream, I can land them for you. If you can provide me a patch against Breakpad SVN, that'd be even easier:
http://code.google.com/p/google-breakpad/source/browse/

You have tested that these don't break the desktop Linux build, right?
Attachment #532773 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 533427 [details] [diff] [review]
rolled up patch

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

There are some things here that don't quite make sense to me. Why are these changes needed now with NDK r5 and not with previous versions?

::: configure.in
@@ +281,5 @@
> +    ;;
> +esac
> +
> +case "$target" in
> +*android*)

Same comment as with the NSPR patch.

@@ +1248,4 @@
>          darwin*)      OS_ARCH=Darwin OS_TARGET=Darwin ;;
>      esac
>      case "${target}" in
> +        *android*) OS_ARCH=Linux OS_TARGET=Android ;;

And here, and all the following places in this file.

::: db/sqlite3/src/sqlite3.c
@@ +25868,4 @@
>  ** If you know that your system does support fdatasync() correctly,
>  ** then simply compile with -Dfdatasync=fdatasync
>  */
> +#if (!defined(fdatasync) && !defined(__linux__)) || defined(ANDROID)

This needs to be upstreamed.

::: ipc/chromium/src/base/hash_tables.h
@@ +19,4 @@
>  
>  #include "base/string16.h"
>  
> +#if defined(COMPILER_MSVC) || defined(ANDROID) && defined(_STLP_STD_NAME)

I think this could use extra parentheses on the right-hand side of the ||.

::: js/src/assembler/wtf/Platform.h
@@ +329,4 @@
>  /* PLATFORM(LINUX) */
>  /* Operating system level dependencies for Linux-like systems that */
>  /* should be used regardless of operating environment */
> +#if defined(__linux__) && !defined(ANDROID)

I don't understand this, what's the reasoning here?

::: js/src/configure.in
@@ +272,5 @@
> +    ;;
> +esac
> +
> +case "$target" in
> +*android*)

Same comments about the pattern here as elsewhere.

::: js/src/ctypes/libffi/config.sub
@@ +126,4 @@
>    nto-qnx* | linux-gnu* | linux-dietlibc | linux-newlib* | linux-uclibc* | \
>    uclinux-uclibc* | uclinux-gnu* | kfreebsd*-gnu* | knetbsd*-gnu* | netbsd*-gnu* | \
>    kopensolaris*-gnu* | \
> +  storm-chaos* | os2-emx* | rtmk-nova* | wince-winmo*)

This has a bunch of extra changes, what's up with that?

::: netwerk/base/src/Makefile.in
@@ +139,4 @@
>  include $(topsrcdir)/ipc/chromium/chromium-config.mk
>  include $(topsrcdir)/config/rules.mk
>  
> +nsURLParsers.$(OBJ_SUFFIX):MOZ_OPTIMIZE_FLAGS=

What's the purpose of this? This needs an explanatory comment.

::: toolkit/xre/nsSigHandlers.cpp
@@ +60,4 @@
>  #include <sys/resource.h>
>  #include <unistd.h>
>  #include <stdlib.h> // atoi
> +#if defined(__amd64__) ||  defined(__i386__) // no arm impl

Surely this should just be #ifndef __arm__ ?
Attachment #533427 - Flags: review?(ted.mielczarek) → review-
> > +#if (!defined(fdatasync) && !defined(__linux__)) || defined(ANDROID)
> 
and 
> >  /* Operating system level dependencies for Linux-like systems that */
> >  /* should be used regardless of operating environment */
> > +#if defined(__linux__) && !defined(ANDROID)
> 
> I don't understand this, what's the reasoning here?

the new toolchain (arm-linuxandroid-eabi) defines __linux__ where the old toolchain didn't. In these two places we tested for __linux__ and assumed not android which isn't true with the new toolchain

> ::: js/src/ctypes/libffi/config.sub
> @@ +126,4 @@
> >    nto-qnx* | linux-gnu* | linux-dietlibc | linux-newlib* | linux-uclibc* | \
> >    uclinux-uclibc* | uclinux-gnu* | kfreebsd*-gnu* | knetbsd*-gnu* | netbsd*-gnu* | \
> >    kopensolaris*-gnu* | \
> > +  storm-chaos* | os2-emx* | rtmk-nova* | wince-winmo*)
> 
> This has a bunch of extra changes, what's up with that?

I simply copied the config.sub from elsewhere in the tree. This copy was apparently a few versions behind

> 
> ::: netwerk/base/src/Makefile.in
> @@ +139,4 @@
> >  include $(topsrcdir)/ipc/chromium/chromium-config.mk
> >  include $(topsrcdir)/config/rules.mk
> >  
> > +nsURLParsers.$(OBJ_SUFFIX):MOZ_OPTIMIZE_FLAGS=
> 
> What's the purpose of this? This needs an explanatory comment.
I believe gcc was crashing when compiling that file with opt flags. I'll retest to make sure its still necessary and add a comment if it is.

> 
> ::: toolkit/xre/nsSigHandlers.cpp
> @@ +60,4 @@
> >  #include <sys/resource.h>
> >  #include <unistd.h>
> >  #include <stdlib.h> // atoi
> > +#if defined(__amd64__) ||  defined(__i386__) // no arm impl
> 
> Surely this should just be #ifndef __arm__ ?

sure
Created attachment 534984 [details] [diff] [review]
nspr patch

updated nspr patch based on comments, carrying r+
Attachment #523727 - Attachment is obsolete: true
Attachment #534984 - Flags: review+
Created attachment 534985 [details] [diff] [review]
patch

updated based on review
Attachment #533427 - Attachment is obsolete: true
Attachment #534985 - Flags: review?(ted.mielczarek)
pushed to try http://tbpl.mozilla.org/?tree=Try&rev=c7dc12714194
Comment on attachment 534985 [details] [diff] [review]
patch

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

::: netwerk/base/src/Makefile.in
@@ +141,5 @@
>  include $(topsrcdir)/config/rules.mk
>  
> +ifeq ($(OS_TARGET),Android)
> +# this works around a "branch out of range" error when compiling this file opt
> +nsURLParsers.$(OBJ_SUFFIX):MOZ_OPTIMIZE_FLAGS=

Nit: can you stick a space after the colon here? Makes it slightly more readable.

::: toolkit/xre/nsSigHandlers.cpp
@@ +190,4 @@
>    *mxcsr &= ~SSE_STATUS_FLAGS; /* clear all pending SSE exceptions */
>  #endif
>  #endif
> +#if defined(LINUX) && (defined(__amd64__) ||  defined(__i386__))

This should probably be && !defined(__arm__) as well, then.
Attachment #534985 - Flags: review?(ted.mielczarek) → review+
pushed http://hg.mozilla.org/mozilla-central/rev/b854ffeef0d1

Ted said he'll push the NSPR changes to NSPR's CVS.

Wan-Teh, can we get a tag so we can update NSPR in mozilla-central to pick this up?
Landed in NSPR CVS:
Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.304; previous revision: 1.303
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.306; previous revision: 1.305
done
Checking in build/autoconf/config.sub;
/cvsroot/mozilla/nsprpub/build/autoconf/config.sub,v  <--  config.sub
new revision: 1.19; previous revision: 1.18
done

Comment 48

6 years ago
Ted: you can create a NSPR_4_8_9_BETA2 CVS tag and push it to mozilla-central.
Okay, I have tagged NSPR tip with that. blassey: feel free to push that tag to mozilla-central.

Comment 50

6 years ago
blassey: FYI, the NSPR_4_8_9_BETA2 tag contains the fixes for the following bugs:
- bug 617115 (this bug)
- bug 647820
- bug 564851
- Bug 650474 (just the last patch in the bug)
pushed nspr patch as part of http://hg.mozilla.org/mozilla-central/rev/b20b6d500c05
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 52

6 years ago
Reopening since we still have the 'Part 5' patch (crashreporter stuff) to finish.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 53

6 years ago
(In reply to comment #39)
> Comment on attachment 532773 [details] [diff] [review] [review]
> Part 5: crashreporter fixes
> 
> Review of attachment 532773 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/crashreporter/Makefile.in
> @@ +100,5 @@
> >  
> >  ifeq ($(OS_TARGET),Android)
> >  DIRS += fileid
> > +# NDK5 workarounds
> > +DEFINES += -D_STLP_CONST_CONSTRUCTOR_BUG -D_STLP_NO_MEMBER_TEMPLATES
> 
> Can we just put these in configure instead of having to stick them in a
> couple of random Makefiles?
> 

It looks like the build command for crashreporter succeeds when I put the defines in a -D, but if they appear inside mozilla-config.h, they have no effect, even though that file is -include'ed. Perhaps there is an order issue of some sort I can't figure out.
(Assignee)

Comment 54

6 years ago
Ok, the problem is that having those defines globally will cause files in /ipc/chromium/ to fail.

So, I'd like to go with the "Part 5: crashreporter fixes" patch as-is. ted, is that ok with you?
(Assignee)

Comment 55

6 years ago
Pushed since we want this ASAP. I can push a followup if anything needs to be changed.

http://hg.mozilla.org/mozilla-central/rev/02a5505b965b

I'll prepare a patch for upstream crashreporter against their svn.
(Assignee)

Comment 56

6 years ago
Created attachment 535681 [details] [diff] [review]
breakpad patch against upstream svn

ted, here is a patch against upstream breakpad svn.

I didn't test it since I'm not sure how, but it should work.
(Assignee)

Comment 57

6 years ago
Marking closed since all done here, except for the upstream patch.
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Okay, unfortunate that you need to do those defines that way, but that's fine. I pushed the Breakpad patch upstream, thanks for making that easy for me!
http://code.google.com/p/google-breakpad/source/detail?r=790
(Reporter)

Updated

6 years ago
Depends on: 661463
(Reporter)

Updated

6 years ago
Depends on: 663214
Blocks: 670315
(In reply to Ted Mielczarek [:ted, :luser] from comment #40)
> ::: db/sqlite3/src/sqlite3.c
> @@ +25868,4 @@
> >  ** If you know that your system does support fdatasync() correctly,
> >  ** then simply compile with -Dfdatasync=fdatasync
> >  */
> > +#if (!defined(fdatasync) && !defined(__linux__)) || defined(ANDROID)
> 
> This needs to be upstreamed.
In the future, this needs to be upstreamed *BEFORE* we take it.  Changing SQLite without telling the person who has to maintain the code that wraps it is *not* cool.  You've already complicated the upgrade to a newer version, which is one of the reasons why we don't take changes to SQLite in our tree.
Depends on: 697709
You need to log in before you can comment on or make changes to this bug.