814783 - terminate_string16 is called twice, causing segfault when an app exits

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Dave Hylands [:dhylands]]

I set a breakpoint in the terminate_string16 function. It should be called exactly once.

It turns out that it gets called twice, and the second time causes a segfault.

Breakpoint 2, android::terminate_string16 () at frameworks/base/libs/utils/String16.cpp:55
55	    SharedBuffer::bufferFromData(gEmptyString)->release();
(gdb) bt
#0  android::terminate_string16 () at frameworks/base/libs/utils/String16.cpp:55
#1  0x40ebd346 in ~LibUtilsFirstStatics (this=0x41e78338, __in_chrg=<value optimized out>) at /home/work/B2G-otoro/mozilla-inbound/widget/gonk/libui/Static.cpp:41
#2  0x400dfc34 in __cxa_finalize (dso=0x0) at bionic/libc/stdlib/atexit.c:158
#3  0x400dffd0 in exit (status=0) at bionic/libc/stdlib/exit.c:57
#4  0x400dffd0 in exit (status=0) at bionic/libc/stdlib/exit.c:57
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
(gdb) c
Continuing.

Breakpoint 2, android::terminate_string16 () at frameworks/base/libs/utils/String16.cpp:55
55	    SharedBuffer::bufferFromData(gEmptyString)->release();
(gdb) bt
#0  android::terminate_string16 () at frameworks/base/libs/utils/String16.cpp:55
#1  0x422803e0 in ~LibUtilsFirstStatics (this=0x4228f278, __in_chrg=<value optimized out>) at frameworks/base/libs/utils/Static.cpp:38
#2  0x400dfc34 in __cxa_finalize (dso=0x0) at bionic/libc/stdlib/atexit.c:158
#3  0x400dffd0 in exit (status=0) at bionic/libc/stdlib/exit.c:57
#4  0x400dffd0 in exit (status=0) at bionic/libc/stdlib/exit.c:57
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
(gdb) c
Continuing.

Program received signal SIGSEGV, Segmentation fault.
android::SharedBuffer::release (this=0xfffffff0, flags=0) at frameworks/base/include/utils/SharedBuffer.h:139
139	    return (mRefs == 1);

The first time its called is from /home/work/B2G-otoro/mozilla-inbound/widget/gonk/libui/Static.cpp and the second time its called is from frameworks/base/libs/utils/Static.cpp

It seems to me that the copy of Static.cpp in libui is superfolous and should be removed. 

I also noticed that we have an almost identical version of String8.cpp, Tokenizer.cpp, and Unicode.cpp in the libui directory as well.

The other solution is to duplicate String16.cpp so that the local copy of String16.cpp's terminate_string16 gets called rather than calling android's version twice.

Comment 1

[Michael Wu [:mwu]]

We basically duplicate anything necessary here so we're never dependent on whatever version Android gives us. I'm not a fan of it but that's the goal here.

Comment 2

[Dave Hylands [:dhylands]]

So then the problem seems to be that we didn't duplicate String16.cpp but call functions from it.

Comment 3

[Dave Hylands [:dhylands]]

Attachment: Add tweaked version of String16.cpp

This adds a version of String16.cpp with similar tweaks that were done to String8.cpp

Now when Statics.cpp calls terminate_string16 it will call the verion from this file rather than android's version.

Comment 4

[Michael Wu [:mwu]]

(In reply to Dave Hylands [:dhylands] from comment #3)
> Created attachment 685941 [details] [diff] [review]
> Add tweaked version of String16.cpp
> 
> This adds a version of String16.cpp with similar tweaks that were done to
> String8.cpp
> 

??

Comment 5

[Dave Hylands [:dhylands]]

(In reply to Michael Wu [:mwu] from comment #4)
> (In reply to Dave Hylands [:dhylands] from comment #3)
> > Created attachment 685941 [details] [diff] [review]
> > Add tweaked version of String16.cpp
> > 
> > This adds a version of String16.cpp with similar tweaks that were done to
> > String8.cpp
> > 
> 
> ??

I compared String8.cpp from libui with the one from android. It had done things like:

#include "String8.h" instead of #include <String8.h>
#include "utils_Log.h" instead of #include <utils/Log.h>

Change LOG_ASSERT to ALOG_ASSERT

Those kinds of tweaks

Comment 6

[Michael Wu [:mwu]]

I didn't see that in the patch

Comment 7

[Dave Hylands [:dhylands]]

Attachment: Add tweaked version of String16.cpp - v2

I think I got the new file into the patch this time

Comment 8

[Dave Hylands [:dhylands]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/83c8b42da6ed

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/83c8b42da6ed

Comment 10

[Dave Hylands [:dhylands]]

When I discovered this bug, the crash in bug 816671 was one of the segfaults I also encountered as a related problem. At the time I was only seeing the crash in debug builds, so I didn't nom it. But since we are in fact seeing the crash in bug 816671, I'm noming this one.

This particular patch is B2G-only.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2ad6d5ca2fac
https://hg.mozilla.org/releases/mozilla-beta/rev/a3a65d32f567