727959 - Browser crashes after loading start page on Android/x86 (android.view.InflateException: Binary XML file line #28: Error inflating class <unknown>: at android.view.LayoutInflater.createView(LayoutInflater.java:606))

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Thomas G Eaton]

Attachment: logcat_firefox_crash.txt

User Agent: Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.1; WOW64; Trident/4.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; .NET CLR 1.1.4322; InfoPath.2; MS-RTC LM 8; .NET4.0C; .NET4.0E)

Steps to reproduce:

Build Fennec for x86/Android and run it on a tablet


Actual results:

It crashes shortly after loading the start screen


Expected results:

It should have continued to run.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Add symbol name to relocation errors in the linker [checked-in]

Thomas, can you apply this patch, which won't fix your problem, but help identify it more easily, and paste new logs with it applied? Thanks.

Comment 2

[Thomas G Eaton]

Attachment: New log prior to adding debug patch.

This log appears to have the "unknown" value filled in.

Comment 3

[Thomas G Eaton]

Attachment: log after adding debug patch

This log is from after adding glandium's debug patch.

Comment 4

[Thomas G Eaton]

Here's a link to the apk via dropbox if you want to grab it and look at the libs you mentioned earlier.  

http://dl.dropbox.com/u/59285258/fennec-13.0a1.en-US.android-i686.apk

Comment 5

[Mike Hommey [:glandium]]

(In reply to Thomas G Eaton from comment #3)
> Created attachment 598441 [details]
> log after adding debug patch
> 
> This log is from after adding glandium's debug patch.

Unfortunately, there aren't any GeckoLinker logs here. (which is essentially unexpected, you should at least get messages about unhandled dynamic headers)

Comment 6

[Mike Hommey [:glandium]]

If the original log corresponds to that apk, there's something seriously wrong with the system linker, then, because the symbols that resolve to NULL are __wrap_malloc, which is in libmozglue.so, and getnameinfo, which is in libc.so. Are there ICS images for use in a virtual machine available somewhere or do we need to rely on actual x86 tablets for testing?

Comment 7

[Thomas G Eaton]

The original log doesn't correspond to that apk.  I no longer had that apk and wasn't able to pull it from the the device.  I added the "export MOZ_OLD_LINKER=1" to the mozconfig and it fixed the crash issue.  I assumed that you were looking at the initial "Error inflating class" line which was still in the log even though the browser was no longer crashing.  

Would you like me to rebuild without the old linker flag and with your debug patch?  

I'm not aware of any images for use in a virtual machine.  All my testing has been on a tablet or phone.

Comment 8

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Thomas, just wondering which NDK version are you using?  I had the same issue building on the Mac in which I was using the NDK 6b.  mfinkle had told me that I needed to add the "export MOZ_OLD_LINKER=1" in order to not crash.  

There's a fix that was just recently pushed (bug 720621) that might be related?

Comment 9

[Mike Hommey [:glandium]]

> There's a fix that was just recently pushed (bug 720621) that might be
> related?

That bug leads to "Text relocation not supported" messages. The original log in this bug is about NULL relocations, which is a different problem.

(In reply to Thomas G Eaton from comment #7)
> Would you like me to rebuild without the old linker flag and with your debug
> patch?  

Yes please.

Comment 10

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e18282cd5f61

Comment 11

[Thomas G Eaton]

Attachment: Logcat with error and debug patch

Here is a link to the apk that corresponds to this logcat. http://dl.dropbox.com/u/59285258/fennec-13.0a1.en-US.android-i686.apk

Comment 12

[Mike Hommey [:glandium]]

So, the failing relocation in both cases (mozsqlite and xul) is for __register_frame_info_bases. This symbol comes from the CRT object files, and is found nowhere in the libs provided by the NDK. Where is this symbol defined in system libraries?

Comment 13

[Thomas G Eaton]

I'm not certain, but is it defined in libgcc?

$ grep -ir register_frame_info_bases *
Binary file android_ndk/android-ndk-r6b/platforms/android-9/arch-x86/usr/lib/crtbegin_static.o matches
Binary file android_ndk/android-ndk-r6b/platforms/android-9/arch-x86/usr/lib/crtbegin_so.o matches
Binary file android_ndk/android-ndk-r6b/platforms/android-9/arch-x86/usr/lib/crtbegin_dynamic.o matches
Binary file android_ndk/android-ndk-r6b/sources/cxx-stl/stlport/libs/x86/libstlport_shared.so matches
Binary file android_ndk/android-ndk-r6b/toolchains/x86-4.4.3/prebuilt/linux-x86/lib/gcc/i686-android-linux/4.4.3/crtbeginS.o matches
Binary file android_ndk/android-ndk-r6b/toolchains/x86-4.4.3/prebuilt/linux-x86/lib/gcc/i686-android-linux/4.4.3/crtbeginT.o matches
Binary file android_ndk/android-ndk-r6b/toolchains/x86-4.4.3/prebuilt/linux-x86/lib/gcc/i686-android-linux/4.4.3/libgcc.a matches
Binary file android_ndk/android-ndk-r6b/toolchains/x86-4.4.3/prebuilt/linux-x86/lib/gcc/i686-android-linux/4.4.3/crtbegin.o matches

Comment 14

[Mike Hommey [:glandium]]

Ah I missed it in libgcc.a. So you need to figure out why libgcc.a is (apparently) not linked in the libraries.

Comment 15

[Richard Newman [:rnewman]]

Attachment: Another log

Clean build from current inbound. Installed over earlier Fennec build from inbound (which at some point in the past little while started exhibiting this problem).

Figured I'd post a log.

Comment 16

[Richard Newman [:rnewman]]

Uninstalling Fennec on the device, and installing the exact same APK again: everything works.

(I should point out that I was getting the exact same error as the original report, but no crash: instead the Bookmarks view was dying, but the app stayed up and responsive.)

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e18282cd5f61

Comment 19

[Mike Hommey [:glandium]]

Attachment: Don't error out when missing symbol for PLT relocations is weak [checked-in]

This is what is happening:
On x86 android NDK, crtbegin_so.o contains a frame_dummy function that calls __register_frame_info_bases. The reference to this symbol is weak. That function is defined in libgcc.a. ld is called with both crtbegin_so.o and libgcc.a, and when linking in that case, it throws away libgcc.a's definition of the function and declares the symbol as undefined. This is actually a feature. I don't know why, but it's the way it works (see further below).

Anyways, the result is that whatever we do, with the current content of the NDK, we can only end up with a relocation for a weak __register_frame_info_bases. However, as far as I can see, frame_dummy is called from nowhere, so, that the relocation ends up writing a NULL pointer in the PLT is not going to be a runtime problem.

On the other hand, on more general terms, there are legitimate uses of weak symbols that can end up with the same scheme, and that shouldn't fail either. So what we actually need to do is not to fail on weak symbol resolution ending up in a NULL, even in the PLT. This should allow the android x86 build to go further in the startup process.

For reference, the linking "problem" can be reproduced with the following:
$ echo "int b() __attribute__((weak)); int a() { return b(); }" > a.c
$ echo "int b() { return 42; }" > b.c
$ gcc -o a.o -c a.c -fPIC
$ gcc -o b.o -c b.c -fPIC
$ ar cr b.a b.o
$ ld -shared -o foo.so a.o b.o
$ objdump -T foo.so | grep b$
0000000000000260 g    DF .text	000000000000000b b
$ ld -shared -o bar.so a.o b.a
$ objdump -T bar.so | grep b$
0000000000000000  w   D  *UND*	0000000000000000 b

Comment 20

[Mike Hommey [:glandium]]

Looking much further in crtbegin_so.o, it appears that frame_dummy *is* called, as a static initializer. BUT, fortunately, it does something like:
  if (__register_frame_info_bases)
    __register_frame_info_bases(args);

so in practice, this shouldn't be a problem.

Comment 21

[Mike Hommey [:glandium]]

Looking even much further, it appears that the state of definition of __register_frame_info_bases in the various c++ shared libraries in NDK r7b matches support for exceptions, which is where frame info registration is important. So all in all, since we're not using exceptions and linking against stlport, which doesn't support them anyways, we're just fine without this call.

Comment 22

[Thomas G Eaton]

Attachment: Logcat crash with latest patch

Comment 23

[(dormant account)]

Comment on attachment 599907 [details] [diff] [review]
Don't error out when missing symbol for PLT relocations is weak [checked-in]

For nitpicky linker stuff, Nathan is a better reviewer

Comment 24

[Mike Hommey [:glandium]]

(In reply to Thomas G Eaton from comment #22)
> Created attachment 600202 [details]
> Logcat crash with latest patch

Can you update to the latest mozilla-central (or inbound), add a #define MOZ_DEBUG_LINKER in mozglue/linker/Logging.h, and try again? (log will be significantly more verbose)

Comment 25

[Nathan Froyd [:froydnj]]

Comment on attachment 599907 [details] [diff] [review]
Don't error out when missing symbol for PLT relocations is weak [checked-in]

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

Works for me.

Comment 26

[Aaron Train [:aaronmt]]

I still often get this on Nightly (02/29).

Comment 27

[Doug Turner (:dougt)]

Mike, you ready to land?

Comment 28

[Mike Hommey [:glandium]]

(In reply to Doug Turner (:dougt) from comment #27)
> Mike, you ready to land?

Apparently, this is not enough to fix the linker issue. I'm waiting for Thomas to respond to comment 24.

Comment 29

[Thomas G Eaton]

Sorry for the delay the last week.  I'll be able to build and capture the logs with the more verbose logging tomorrow.

Comment 30

[Thomas G Eaton]

I wasn't able to get my dev machine up and working today to get this done.  I'll be out of the office for the rest of the week.  I should be able to provide the logs next week.  There is a tablet device in Mountain View where this issue could be replicated if progress needs to be made on this bug prior to Monday.  Sorry for the delay.

Comment 31

[Mike Hommey [:glandium]]

Comment on attachment 599907 [details] [diff] [review]
Don't error out when missing symbol for PLT relocations is weak [checked-in]

https://hg.mozilla.org/integration/mozilla-inbound/rev/973003f8b502

Comment 32

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/973003f8b502

Comment 33

[Thomas G Eaton]

Attachment: Logcat with MOZ_DEBUG_LINKER defined

Comment 34

[Mike Hommey [:glandium]]

(In reply to Thomas G Eaton from comment #33)
> Created attachment 605203 [details]
> Logcat with MOZ_DEBUG_LINKER defined

You need to apply the patches from this bug, too :)

Comment 35

[Thomas G Eaton]

Attachment: Added the rest of the patch for this log

Sorry, I only added the patch from early in the bug and missed that latest one.  Here's an updated logcat with the patch from comment 31.

Comment 36

[Mike Hommey [:glandium]]

Thanks. That very much looks like a toolchain bug. I guess we'll have to work around it. Could you attach the libmozsqlite3.so file from that build?

Comment 37

[Thomas G Eaton]

Attachment: libmozsqlite3.so from the build that generated the last log

Comment 38

[Mike Hommey [:glandium]]

Attachment: tentative workaround

Can you try this patch and see if you go further?

Comment 39

[Thomas G Eaton]

It loads with no issues with that patch and I am able to do a google image search with no issues.

Comment 40

[Mike Hommey [:glandium]]

Attachment: Ignore 0xffffffff entries the x86 Android NDK puts in .{init,fini}_array

Comment 41

[(dormant account)]

Comment on attachment 605512 [details] [diff] [review]
Ignore 0xffffffff entries the x86 Android NDK puts in .{init,fini}_array

Feels like this should spit out an adb log message about linker being busted

Comment 42

[Mike Hommey [:glandium]]

> Feels like this should spit out an adb log message about linker being busted

I'm not convinced it's worth polluting logs with something we already know.

Comment 43

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8133f0c6fc4e

Comment 44

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/8133f0c6fc4e