875251 - [Bluetooth] Crash when turning on Bluetooth in Settings app

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Seem on today's m-i

STR:

  - flash phone and turn on bluetooth

Expected result:

  - bluetooth turns on

Actual result

  - Gecko crashes

This can be reproduced easily. The stack trace is shown below. I suspect that this problem is caused by the patch for bug 868130.

>>>

tdz@barney:~/Projects/mozilla/src/B2G-master-unagi$ ./run-gdb.sh attach 109
Attached; pid = 109
Listening on port 11109
prebuilt/linux-x86/toolchain/arm-linux-androideabi-4.4.x/bin/arm-linux-androideabi-gdb -x /tmp/b2g.gdbinit.tdz /home/tdz/Projects/mozilla/src/B2G-master-unagi/objdir-gecko-debug/dist/bin/b2g
GNU gdb (GDB) 7.1-android-gg2
Copyright (C) 2010 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "--host=i686-linux-gnu --target=arm-elf-linux".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Really redefine built-in command "frame"? (y or n) [answered Y; input not from terminal]
Really redefine built-in command "thread"? (y or n) [answered Y; input not from terminal]
Really redefine built-in command "start"? (y or n) [answered Y; input not from terminal]
Reading symbols from /home/tdz/Projects/mozilla/src/B2G-master-unagi/objdir-gecko-debug/dist/bin/b2g...done.
Remote debugging from host 127.0.0.1
_______________________________________________________________________________
Error while running hook_stop:
Value can't be converted to integer.
arena_run_reg_dalloc (ptr=<value optimized out>, offset=<value optimized out>) at /home/tdz/Projects/mozilla/src/mozilla-inbound/bug-830290/memory/mozjemalloc/jemalloc.c:3326
3326			regind = size_invs[(size >> QUANTUM_2POW_MIN) - 3] * diff;
gdb> c
[New Thread 109.344]
[New Thread 109.256]

Program received signal SIGSEGV, Segmentation fault.
_______________________________________________________________________________
Error while running hook_stop:
Value can't be converted to integer.
0x422a4b7a in AutoCheckRequestDepth (this=0xbea9d47c, cx=0x48d8fc00) at /home/tdz/Projects/mozilla/src/mozilla-inbound/bug-830290/js/src/jscntxt.cpp:1479
1479	    JS_ASSERT(cx->runtime->requestDepth || cx->runtime->isHeapBusy());
gdb> bt
#0  0x422a4b7a in AutoCheckRequestDepth (this=0xbea9d47c, cx=0x48d8fc00) at /home/tdz/Projects/mozilla/src/mozilla-inbound/bug-830290/js/src/jscntxt.cpp:1479
#1  0x4226a01c in JS_NewArrayObject (cx=0x48d8fc00, length=0x0, vector=0x0) at /home/tdz/Projects/mozilla/src/mozilla-inbound/bug-830290/js/src/jsapi.cpp:4739
#2  0x4156c962 in nsTArrayToJSArray<nsString> (aCx=0xad, aSourceArray=..., aResultArray=<value optimized out>) at ../../../dist/include/nsTArrayHelpers.h:59
#3  0x416ca696 in mozilla::dom::bluetooth::BluetoothAdapter::SetPropertyByValue (this=0x47cc9180, aValue=<value optimized out>) at /home/tdz/Projects/mozilla/src/mozilla-inbound/bug-830290/dom/bluetooth/BluetoothAdapter.cpp:281
#4  0x416ca886 in BluetoothAdapter (this=0x47cc9180, aWindow=<value optimized out>, aValue=...) at /home/tdz/Projects/mozilla/src/mozilla-inbound/bug-830290/dom/bluetooth/BluetoothAdapter.cpp:192
#5  0x416ca984 in mozilla::dom::bluetooth::BluetoothAdapter::Create (aWindow=0x404f4f20, aValue=...) at /home/tdz/Projects/mozilla/src/mozilla-inbound/bug-830290/dom/bluetooth/BluetoothAdapter.cpp:305
#6  0x416c7302 in GetAdapterTask::ParseSuccessfulReply (this=0x4c8f3190, aValue=0xbea9d670) at /home/tdz/Projects/mozilla/src/mozilla-inbound/bug-830290/dom/bluetooth/BluetoothManager.cpp:61
#7  0x416cb90e in mozilla::dom::bluetooth::BluetoothReplyRunnable::Run (this=0x4c8f3190) at /home/tdz/Projects/mozilla/src/mozilla-inbound/bug-830290/dom/bluetooth/BluetoothReplyRunnable.cpp:68
#8  0x41e0d5ee in nsThread::ProcessNextEvent (this=0x40404320, mayWait=<value optimized out>, result=0xbea9d6ef) at /home/tdz/Projects/mozilla/src/mozilla-inbound/bug-830290/xpcom/threads/nsThread.cpp:627
#9  0x41dd3bc0 in NS_ProcessNextEvent (thread=0xad, mayWait=0x0) at /home/tdz/Projects/mozilla/src/B2G-master-unagi/objdir-gecko-debug/xpcom/build/nsThreadUtils.cpp:238
#10 0x41a907b2 in mozilla::ipc::MessagePump::Run (this=0x404024c0, aDelegate=0x404310c0) at /home/tdz/Projects/mozilla/src/mozilla-inbound/bug-830290/ipc/glue/MessagePump.cpp:82
#11 0x41e52c0e in MessageLoop::RunInternal (this=0x404310c0) at /home/tdz/Projects/mozilla/src/mozilla-inbound/bug-830290/ipc/chromium/src/base/message_loop.cc:219
#12 0x41e52c6e in MessageLoop::RunHandler (this=0x404310c0) at /home/tdz/Projects/mozilla/src/mozilla-inbound/bug-830290/ipc/chromium/src/base/message_loop.cc:212
#13 MessageLoop::Run (this=0x404310c0) at /home/tdz/Projects/mozilla/src/mozilla-inbound/bug-830290/ipc/chromium/src/base/message_loop.cc:186
#14 0x41a0a156 in nsBaseAppShell::Run (this=0x44225d00) at /home/tdz/Projects/mozilla/src/mozilla-inbound/bug-830290/widget/xpwidgets/nsBaseAppShell.cpp:163
#15 0x418fb95a in nsAppStartup::Run (this=0x44357e20) at /home/tdz/Projects/mozilla/src/mozilla-inbound/bug-830290/toolkit/components/startup/nsAppStartup.cpp:269
#16 0x40de566a in XREMain::XRE_mainRun (this=0xbea9d984) at /home/tdz/Projects/mozilla/src/mozilla-inbound/bug-830290/toolkit/xre/nsAppRunner.cpp:3872
#17 0x40de8382 in XREMain::XRE_main (this=0xbea9d984, argc=<value optimized out>, argv=<value optimized out>, aAppData=0x222d8) at /home/tdz/Projects/mozilla/src/mozilla-inbound/bug-830290/toolkit/xre/nsAppRunner.cpp:3939
#18 0x40de8532 in XRE_main (argc=0x1, argv=0xbea9fb84, aAppData=0x222d8, aFlags=<value optimized out>) at /home/tdz/Projects/mozilla/src/mozilla-inbound/bug-830290/toolkit/xre/nsAppRunner.cpp:4140
#19 0x00009aee in do_main (argc=0x1, argv=0xbea9fb84) at /home/tdz/Projects/mozilla/src/mozilla-inbound/bug-830290/b2g/app/nsBrowserApp.cpp:168
#20 main (argc=0x1, argv=0xbea9fb84) at /home/tdz/Projects/mozilla/src/mozilla-inbound/bug-830290/b2g/app/nsBrowserApp.cpp:261
gdb>

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Blocker because Bluetooth is not useable.

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

If seens this assertion triggering during startup when Bluetooth was still disabled. So it's certainly not BT-only.

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Thomas Zimmermann [:tzimmermann] from comment #2)
> If seens 
I've  seen...

Comment 4

[Bobby Holley (:bholley)]

Attachment: Push the JSContexts that get used in BluetoothAdapter. v1

Comment 5

[Bobby Holley (:bholley)]

tzimmermann - I'm on PTO today, and don't have time to spin up a b2g build. Can you give this patch a try and make sure it fixes the crash? I can't even build it locally, because bluetooth isn't built on desktop.

The basic idea is that you're not allowed to just grab a random JSContext and pass it to functions - you have to push it first. This was the rule before, but this patch just made it more obvious when we get it wrong.

The fixes for this kind of bug are generally quite simple and straightforward, and always fix real bugs. As such, I'd appreciate if someone could shepherd this into the tree, and if people would avoid backing out bug 868130, because it's a huge set of patches that bitrot extremely quickly (also, backing it out would also require backing out bug 868110, which landed on top of it). Gabor should be able to help with any advice or reviews, and I'll be available by email for the most part (though may not be reading bugmail).

Comment 6

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Sure, I'll give it a try. Sorry for interupting your PTO. Thanks for the quick response.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Thomas Zimmermann [:tzimmermann] from comment #6)
> Sure, I'll give it a try. Sorry for interupting your PTO. Thanks for the
> quick response.

No problem! Sorry for the crashes. :-)

Shoot me email (drop the +bmo from my bugmail) if you have any questions. It'd also be good if we could get test coverage that would have failed here - when doing these Gecko-wide massive refactorings, I end up relying pretty heavily on "whatever is green on try". ;-)

Comment 8

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 753271 [details] [diff] [review]
Push the JSContexts that get used in BluetoothAdapter. v1

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

Looks good, if it builds it should work too. But I found some more similar cases around blue tooth (and some places else in Gecko...). Some follow up would be nice, I need to run now but a few hours later I will come back and can write some followups myself probably, but then I won't be able to review them...

Anyway, if someone is enthusiastic it's quite simple to fix them as Bobby described it, and a quick search for nsContentUtils::GetSafeJSContext() usages and GetNativeContext() usages should help finding the suspicious places quickly. In case of the former, it should be replaced by an AutoSafeJSContext cx; that does the push automatically.

Comment 9

[Thomas Zimmermann [:tzimmermann] [:tdz]]

FYI: The patch alone doesn't fix the problem, gecko just crashes at a different position for the same reason.

Comment 10

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Added AutoPushJSContext in BluetoothDevice.cpp

I also need this patch to make BT work again. I tested this by pairing and transfering a file to the phone.

Comment 11

[Gabor Krizsanits (INACTIVE)]

(In reply to Thomas Zimmermann [:tzimmermann] from comment #10)
> Created attachment 753309 [details] [diff] [review]
> [02] Added AutoPushJSContext in BluetoothDevice.cpp
> 
> I also need this patch to make BT work again. I tested this by pairing and
> transfering a file to the phone.

+    AutoPushJSContext cx(sc->GetNativeContext());
     if (NS_FAILED(nsTArrayToJSArray(sc->GetNativeContext(),

This will solve the problem, but you want to use the cx in the function you call instead of calling GetNativeContext() again.

So: 

     AutoPushJSContext cx(sc->GetNativeContext());
     if (NS_FAILED(nsTArrayToJSArray(cx, ...

both places.

Also could you also do the same in BluetoothManager.cpp? 

And in BluetoothHfpManager.cpp something like : 

-  JSContext* cx = nsContentUtils::GetSafeJSContext();
-  if (!cx) {
-    NS_WARNING("Failed to get JSContext");
-    return NS_OK;
-  }
+  mozilla::AutoSafeJSContext cx;

Then blue tooth will be clean, and I will start writing patches for the rest of gecko. Is there a way to add test for this crash by the way or it needed to be tested manually?

I'm sorry for keeping you busy with all this, but I think this is the fastest way to get it landed...

Comment 12

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Added AutoPushJSContext in Bluetooth

Here is the updated version you asked for. Putting up the first patch for review was more an afterthought.

Comment 13

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 753685 [details] [diff] [review]
[02] Added AutoPushJSContext in Bluetooth

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

Looks good, thanks!

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/projects/birch/rev/ac3784f25f7a
https://hg.mozilla.org/projects/birch/rev/32d6f86c3d7f

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/32d6f86c3d7f
https://hg.mozilla.org/mozilla-central/rev/ac3784f25f7a