1127670 - [Bluetooth][API v2] Sometimes, Settings app is killed during pairing process.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[[e-mail], [e-mail]]

Since I'm working on implementation of pairing flow, I sometime see Settings app is killed during pairing process.

STD:
1. Launch Settings app and go into Bluetooth panel.
2. Turn Bluetooth on and try to pair with a remote device.

Expected:
Settings app should not be killed.

Actual:
Sometimes, Settings app is killed during pairing process.

Comment 1

[[e-mail], [e-mail]]

======= log =======
I/Gecko   ( 1608): [Parent 1608] WARNING: pipe error (182): Connection reset by peer: file ../../../gecko/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 456
I/Gecko   ( 1608): ############ ErrorPage.js
I/GeckoDump( 1608): Crash reporter : Can't fetch app.reportCrashes. Exception: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: chrome://b2g/content/shell.js :: shell_reportCrash :: line 142"  data: no]
V/WLAN_PSA(  214): NL MSG, len[048], NL type[0x11] WNI type[0x5050] len[028]
D/btif_config_util( 1608): btif_config_save_file(L188): in file name:/data/mis

Comment 2

[Ben Tian (inactive)]

Jamin will take a look first. Assign owner to him.

Comment 3

[Jocelyn Liu [:jocelyn] [:joliu]]

I don't have a clear STR, but I also encounter settings killed when using pair API.

Program received signal SIGSEGV, Segmentation fault.
mozilla::dom::bluetooth::BluetoothClassOfDevice::Equals (this=this@entry=0x0, aValue=5898764) at ../../../../yrliou-gecko-dev/dom/bluetooth2/BluetoothClassOfDevice.cpp:67
67                mMinorDeviceClass == GET_MINOR_DEVICE_CLASS(aValue));
(gdb) bt
#0  mozilla::dom::bluetooth::BluetoothClassOfDevice::Equals (this=this@entry=0x0, aValue=5898764) at ../../../../yrliou-gecko-dev/dom/bluetooth2/BluetoothClassOfDevice.cpp:67
#1  0xb54e574c in mozilla::dom::bluetooth::BluetoothDevice::IsDeviceAttributeChanged (this=this@entry=0xb0fb4c40, aType=aType@entry=mozilla::dom::Cod, aValue=...) at ../../../../yrliou-gecko-dev/dom/bluetooth2/BluetoothDevice.cpp:235
#2  0xb54e5f70 in mozilla::dom::bluetooth::BluetoothDevice::HandlePropertyChanged (this=this@entry=0xb0fb4c40, aValue=...) at ../../../../yrliou-gecko-dev/dom/bluetooth2/BluetoothDevice.cpp:276
#3  0xb54e60b8 in mozilla::dom::bluetooth::BluetoothDevice::Notify (this=0xb0fb4c40, aData=...) at ../../../../yrliou-gecko-dev/dom/bluetooth2/BluetoothDevice.cpp:205
#4  0xb54ecda0 in Broadcast (aParam=..., this=<optimized out>) at ../../dist/include/mozilla/Observer.h:72
#5  mozilla::dom::bluetooth::BluetoothService::DistributeSignal (this=this@entry=0xb0f7be50, aSignal=...) at ../../../../yrliou-gecko-dev/dom/bluetooth2/BluetoothService.cpp:351
#6  0xb5509e6e in mozilla::dom::bluetooth::BluetoothChild::RecvNotify (this=<optimized out>, aSignal=...) at ../../../../yrliou-gecko-dev/dom/bluetooth2/ipc/BluetoothChild.cpp:79
#7  0xb4979f12 in mozilla::dom::bluetooth::PBluetoothChild::OnMessageReceived (this=0xb0f7bee0, __msg=...) at PBluetoothChild.cpp:360
#8  0xb49c2ce2 in mozilla::dom::PContentChild::OnMessageReceived (this=0xb3855c18, __msg=...) at PContentChild.cpp:4777
#9  0xb489cdd4 in mozilla::ipc::MessageChannel::DispatchAsyncMessage (this=0xb3855c48, aMsg=...) at ../../../../yrliou-gecko-dev/ipc/glue/MessageChannel.cpp:1231
#10 0xb48a2348 in mozilla::ipc::MessageChannel::DispatchMessage (this=this@entry=0xb3855c48, aMsg=...) at ../../../../yrliou-gecko-dev/ipc/glue/MessageChannel.cpp:1158
#11 0xb48a6de2 in mozilla::ipc::MessageChannel::OnMaybeDequeueOne (this=0xb3855c48) at ../../../../yrliou-gecko-dev/ipc/glue/MessageChannel.cpp:1142
#12 0xb469e974 in DispatchToMethod<FdWatcher, void (FdWatcher::*)()> (method=(void (FdWatcher::*)(FdWatcher * const)) 0xb48a6d4d <mozilla::ipc::MessageChannel::OnMaybeDequeueOne()>, obj=<optimized out>, arg=...)
    at ../../../../yrliou-gecko-dev/ipc/chromium/src/base/tuple.h:383
#13 RunnableMethod<FdWatcher, void (FdWatcher::*)(), Tuple0>::Run (this=<optimized out>) at ../../../../yrliou-gecko-dev/ipc/chromium/src/base/task.h:310
#14 0xb489ddc2 in Run (this=<optimized out>) at ../../dist/include/mozilla/ipc/MessageChannel.h:437
#15 mozilla::ipc::MessageChannel::DequeueTask::Run (this=<optimized out>) at ../../dist/include/mozilla/ipc/MessageChannel.h:454
#16 0xb488e2ec in MessageLoop::RunTask (this=0xbece7f60, task=0xb065a2e0) at ../../../../yrliou-gecko-dev/ipc/chromium/src/base/message_loop.cc:361
#17 0xb4891422 in MessageLoop::DeferOrRunPendingTask (this=<optimized out>, pending_task=...) at ../../../../yrliou-gecko-dev/ipc/chromium/src/base/message_loop.cc:369
#18 0xb48931d8 in DoWork (this=<optimized out>) at ../../../../yrliou-gecko-dev/ipc/chromium/src/base/message_loop.cc:447
#19 MessageLoop::DoWork (this=0xbece7f60) at ../../../../yrliou-gecko-dev/ipc/chromium/src/base/message_loop.cc:426
#20 0xb489b422 in mozilla::ipc::DoWorkRunnable::Run (this=<optimized out>) at ../../../../yrliou-gecko-dev/ipc/glue/MessagePump.cpp:233
#21 0xb46d8378 in nsThread::ProcessNextEvent (this=0xb3868110, aMayWait=<optimized out>, aResult=0xbece7e07) at ../../../../yrliou-gecko-dev/xpcom/threads/nsThread.cpp:855
#22 0xb46ed554 in NS_ProcessNextEvent (aThread=0xb3868110, aMayWait=aMayWait@entry=true) at /home/jocelyn/mozilla/yrliou-gecko-dev/xpcom/glue/nsThreadUtils.cpp:265
#23 0xb48a39ce in mozilla::ipc::MessagePump::Run (this=0xb3801dc0, aDelegate=0xbece7f60) at ../../../../yrliou-gecko-dev/ipc/glue/MessagePump.cpp:140
#24 0xb488f2d8 in MessageLoop::RunInternal (this=this@entry=0xbece7f60) at ../../../../yrliou-gecko-dev/ipc/chromium/src/base/message_loop.cc:233
#25 0xb488f2f2 in RunHandler (this=0xbece7f60) at ../../../../yrliou-gecko-dev/ipc/chromium/src/base/message_loop.cc:226
#26 MessageLoop::Run (this=0xbece7f60) at ../../../../yrliou-gecko-dev/ipc/chromium/src/base/message_loop.cc:200
#27 0xb551433a in nsBaseAppShell::Run (this=0xb23f61c0) at ../../../yrliou-gecko-dev/widget/nsBaseAppShell.cpp:164
#28 0xb596ce40 in XRE_RunAppShell () at ../../../../yrliou-gecko-dev/toolkit/xre/nsEmbedFunctions.cpp:738
#29 0xb48a3ab2 in mozilla::ipc::MessagePumpForChildProcess::Run (this=0xb3801dc0, aDelegate=0xbece7f60) at ../../../../yrliou-gecko-dev/ipc/glue/MessagePump.cpp:272
#30 0xb488f2d8 in MessageLoop::RunInternal (this=this@entry=0xbece7f60) at ../../../../yrliou-gecko-dev/ipc/chromium/src/base/message_loop.cc:233
#31 0xb488f2f2 in RunHandler (this=0xbece7f60) at ../../../../yrliou-gecko-dev/ipc/chromium/src/base/message_loop.cc:226
#32 MessageLoop::Run (this=this@entry=0xbece7f60) at ../../../../yrliou-gecko-dev/ipc/chromium/src/base/message_loop.cc:200
#33 0xb596cd82 in XRE_InitChildProcess (aArgc=<optimized out>, aArgv=<optimized out>, aGMPLoader=<optimized out>) at ../../../../yrliou-gecko-dev/toolkit/xre/nsEmbedFunctions.cpp:575
#34 0xb6fb8e14 in content_process_main (argc=7, argv=0xbece8a54) at ../../../../yrliou-gecko-dev/ipc/app/../contentproc/plugin-container.cpp:211
#35 0xb6eed4a4 in __libc_init (raw_args=0xbece8a50, onexit=<optimized out>, slingshot=0xb6fb8e75 <main(int, char**)>, structors=<optimized out>) at bionic/libc/bionic/libc_init_dynamic.cpp:112
#36 0xb6fb8cf4 in _start ()

Comment 4

[Jocelyn Liu [:jocelyn] [:joliu]]

We create a temporary device instance during pairing[1].

We will hit the crash if the destructor of BluetoothDevice is called before we finish handling PropertyChanged signal.

In this case, mCod is already null at #1  0xb54e574c in mozilla::dom::bluetooth::BluetoothDevice::IsDeviceAttributeChanged (this=this@entry=0xb0fb4c40, aType=aType@entry=mozilla::dom::Cod, aValue=...) at ../../../../yrliou-gecko-dev/dom/bluetooth2/BluetoothDevice.cpp:235.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/BluetoothPairingListener.cpp#108-109

I am thinking of providing the device name directly to gaia in the pairing event instead of creating a temporary BluetoothDevice since name is the only property that gaia is using.

Comment 5

[Jocelyn Liu [:jocelyn] [:joliu]]

Steal this bug. ;)

Comment 6

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1127670 - Replace device property of BluetoothPairingEvent with device name.

Hi Ben,

This patch is to replace device attribute of BluetoothPairingEvent with the device name.
In this approach, we could avoid create a temp device in gecko during pairing but still provide enough information to bluetooth app.
Please help to review my patch.
I will update the wiki page after this solution is r+ from you.

Thanks,
Jocelyn

Comment 7

[Ben Tian (inactive)]

Comment on attachment 8568446 [details] [diff] [review]
Bug 1127670 - Replace device property of BluetoothPairingEvent with device name.

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

r=me with comment addressed.

::: dom/webidl/BluetoothPairingEvent.webidl
@@ +8,5 @@
>   Constructor(DOMString type,
>               optional BluetoothPairingEventInit eventInitDict)]
>  interface BluetoothPairingEvent : Event
>  {
> +  readonly attribute DOMString              name;

Rename to |deviceName| to be clearer.

@@ +14,5 @@
>  };
>  
>  dictionary BluetoothPairingEventInit : EventInit
>  {
> +  required DOMString              name;

Ditto.

Comment 8

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1127670 - Replace device property of BluetoothPairingEvent with device name. r=btian

Hi Boris,

This patch replaces the device attribute with device name (which is the only information of the remote device that Gaia needed) in BluetoothPairingEvent.webidl to avoid create a redundant temporary device instance in gecko.
Could you help to review this webidl change?

Thanks,
Jocelyn

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8568471 [details] [diff] [review]
Bug 1127670 - Replace device property of BluetoothPairingEvent with device name. r=btian

This is OK as far as it goes, but I'd like to understand comment 4.  Is someone currently working with a BluetoothDevice after its destructor has run?  If so, who is this someone and why are they not keeping the BluetoothDevice alive?

Comment 10

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Not doing reviews right now from comment #9)
> Comment on attachment 8568471 [details] [diff] [review]
> Bug 1127670 - Replace device property of BluetoothPairingEvent with device
> name. r=btian
> 
> This is OK as far as it goes, but I'd like to understand comment 4.  Is
> someone currently working with a BluetoothDevice after its destructor has
> run?  If so, who is this someone and why are they not keeping the
> BluetoothDevice alive?

What I saw is it's still handling BluetoothSignal start from |BluetoothDevice::Notify|[1].
However, we do unregister the signal observer in our destructor.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/BluetoothDevice.cpp#199 
[2] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/BluetoothDevice.cpp#104

Comment 11

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1127670 - Replace device property of BluetoothPairingEvent with device name. r=btian, r=bz

Comment 12

[Jocelyn Liu [:jocelyn] [:joliu]]

No try server link since bluetooth2 won't be built.

Comment 13

[Jocelyn Liu [:jocelyn] [:joliu]]

Remove checkin-needed since this patch has already been checked into b2g-inbound.
https://hg.mozilla.org/integration/b2g-inbound/rev/f0c3ace8bab5

Comment 14

[Boris Zbarsky [:bzbarsky]]

> However, we do unregister the signal observer in our destructor.

Then why was there a problem?

In case it wasn't clear, my review was conditional on a clear explanation of why there was a problem.  It's a bit unfortunate that the patch was checked in without that happening, but I would like to see such an explanation still, because it sounds to me like there is a more fundamental problem here that needs fixing.

If it would help, please attach a stack that shows the crash you saw?

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 16

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Not doing reviews right now from comment #14)
> > However, we do unregister the signal observer in our destructor.
> 
> Then why was there a problem?
> 
> In case it wasn't clear, my review was conditional on a clear explanation of
> why there was a problem.  It's a bit unfortunate that the patch was checked
> in without that happening, but I would like to see such an explanation
> still, because it sounds to me like there is a more fundamental problem here
> that needs fixing.
> 

Sorry for misunderstanding your review comments...
And you're right, there is still a problem there needs fixing.
I found out that I could still somehow reproduce the bug by disable/enable BT during pairing sometimes,
which means other BluetoothDevices not created by PairingListener can also hit this crash.

I dig into this problem more today, my observation is the refcount of |BluetoothClassOfDevice| somehow drop to 0 during pairing (not every time), so it is deleted and destructed.
But this didn't make sense since we have |BluetoothDevice| keeps a reference to the |BluetoothClassOfDevice|, and it is not destructed yet when this happens.
It still listens to bluetoothsignals and handles them, then hit the crash.
I will keep digging though I don't have a clue why this could happen at this moment.

For the landed patch, I think it's still a right thing to do since we didn't expect Gaia to use this temporary device(like calling methods in |BluetoothDevice|.
But for this bug, it only avoid partial cases by not creating devices for |PairingListener|.

Again, I'm sorry for misunderstanding your comments and landed a patch which didn't actually solve this bug.
I can reopen this bug and attach/review/land a new patch,
or open a new bug to solve this and make this old one as the bug to not giving a temp device to application since we don't expect them to use this device's functions.
What do you think?

Comment 17

[Boris Zbarsky [:bzbarsky]]

> I will keep digging though I don't have a clue why this could happen at this moment.

OK.  Please file a new bug tracking this?

> For the landed patch, I think it's still a right thing to do

Yes, I agree.  I did say that above.  ;)

> What do you think?

New bug is better.