1096266 - [bluetooth2] Settings crash after calling Bluetooth v2 api disable

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Program received signal SIGTERM, Terminated.
write () at bionic/libc/arch-arm/syscalls/write.S:14
14          b       __set_errno
(gdb) bt
#0  write () at bionic/libc/arch-arm/syscalls/write.S:14
#1  0xb4f0ef5c in base::MessagePumpLibevent::ScheduleWork (this=0xb6b01f10) at ../../../../../hg/mctry/ipc/chromium/src/base/message_pump_libevent.cc:363
#2  0xb4f1680a in MessageLoop::PostTask_Helper (this=this@entry=0xb3e2cba0, from_here=..., task=task@entry=0xb1904880, delay_ms=delay_ms@entry=0, nestable=nestable@entry=true)
    at ../../../../../hg/mctry/ipc/chromium/src/base/message_loop.cc:332
#3  0xb4f1682e in MessageLoop::PostTask (this=this@entry=0xb3e2cba0, from_here=..., task=task@entry=0xb1904880) at ../../../../../hg/mctry/ipc/chromium/src/base/message_loop.cc:266
#4  0xb4f1fbd2 in mozilla::ipc::ProcessLink::SendMessage (this=<optimized out>, msg=0xb1904840) at ../../../../../hg/mctry/ipc/glue/MessageLink.cpp:197
#5  0xb4f22124 in mozilla::ipc::MessageChannel::Send (this=this@entry=0xb6b55448, aMsg=aMsg@entry=0xb1904840) at ../../../../../hg/mctry/ipc/glue/MessageChannel.cpp:520
#6  0xb4f63214 in mozilla::dom::PContentChild::SendAsyncMessage (this=this@entry=0xb6b55418, aMessage=..., aData=..., aCpows=..., aPrincipal=...) at PContentChild.cpp:3769
#7  0xb524ed9a in ChildProcessMessageManagerCallback::DoSendAsyncMessage (this=<optimized out>, aCx=0xb6b311b0, aMessage=..., aData=..., aCpows=..., aPrincipal=0xb6b2b5c8)
    at ../../../../../hg/mctry/dom/base/nsFrameMessageManager.cpp:1767
#8  0xb52400c4 in nsFrameMessageManager::DispatchAsyncMessageInternal (this=this@entry=0xb3ae35e0, aCx=aCx@entry=0xb6b311b0, aMessage=..., aData=..., aCpows=aCpows@entry=..., 
    aPrincipal=aPrincipal@entry=0xb6b2b5c8) at ../../../../../hg/mctry/dom/base/nsFrameMessageManager.cpp:676
#9  0xb5255446 in nsFrameMessageManager::DispatchAsyncMessage (this=0xb3ae35e0, aMessageName=..., aJSON=..., aObjects=..., aPrincipal=0xb6b2b5c8, aCx=0xb6b311b0, aArgc=4 '\004')
    at ../../../../../hg/mctry/dom/base/nsFrameMessageManager.cpp:707
#10 0xb4dd2c76 in NS_InvokeByIndex (that=<optimized out>, methodIndex=<optimized out>, paramCount=<optimized out>, params=<optimized out>)
    at ../../../../../../../../hg/mctry/xpcom/reflect/xptcall/md/unix/xptcinvoke_arm.cpp:164
#11 0xb5031d5e in Invoke (this=0xbedf44c8) at ../../../../../../hg/mctry/js/xpconnect/src/XPCWrappedNative.cpp:2394
#12 Call (this=0xbedf44c8) at ../../../../../../hg/mctry/js/xpconnect/src/XPCWrappedNative.cpp:1746
#13 XPCWrappedNative::CallMethod (ccx=..., mode=mode@entry=XPCWrappedNative::CALL_METHOD) at ../../../../../../hg/mctry/js/xpconnect/src/XPCWrappedNative.cpp:1713
#14 0xb50321ca in XPC_WN_CallMethod (cx=0xb6b311b0, argc=4, vp=0xb3dca148) at ../../../../../../hg/mctry/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1250
#15 0xb5f6950e in CallJSNative (args=..., native=0xb50320d9 <XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*)>, cx=0xb6b311b0) at ../../../../../hg/mctry/js/src/jscntxtinlines.h:231
#16 js::Invoke (cx=0xb6b311b0, args=..., construct=js::NO_CONSTRUCT) at ../../../../../hg/mctry/js/src/vm/Interpreter.cpp:482
#17 0xb5f63e16 in Interpret (cx=0xb6b311b0, state=...) at ../../../../../hg/mctry/js/src/vm/Interpreter.cpp:2517
#18 0xb5f69286 in js::RunScript (cx=cx@entry=0xb6b311b0, state=...) at ../../../../../hg/mctry/js/src/vm/Interpreter.cpp:432
#19 0xb5f694ae in js::Invoke (cx=cx@entry=0xb6b311b0, args=..., construct=construct@entry=js::NO_CONSTRUCT) at ../../../../../hg/mctry/js/src/vm/Interpreter.cpp:501
#20 0xb5f69af0 in js::Invoke (cx=cx@entry=0xb6b311b0, thisv=..., fval=..., argc=argc@entry=1, argv=argv@entry=0xbedf55c8, rval=rval@entry=...) at ../../../../../hg/mctry/js/src/vm/Interpreter.cpp:538
#21 0xb5ee3134 in JS_CallFunctionValue (cx=cx@entry=0xb6b311b0, obj=..., obj@entry=..., fval=..., fval@entry=..., args=..., rval=rval@entry=...) at ../../../../../hg/mctry/js/src/jsapi.cpp:5001
#22 0xb503396c in nsXPCWrappedJSClass::CallMethod (this=0xb14c8460, wrapper=<optimized out>, methodIndex=<optimized out>, info_=0xb6bdd770, nativeParams=0xbedf5668)
    at ../../../../../../hg/mctry/js/xpconnect/src/XPCWrappedJSClass.cpp:1187
#23 0xb502609c in CallMethod (params=<optimized out>, info=0xb6bdd770, methodIndex=3, this=0xb1591d40) at ../../../../../../hg/mctry/js/xpconnect/src/XPCWrappedJS.cpp:532
#24 nsXPCWrappedJS::CallMethod (this=0xb1591d40, methodIndex=<optimized out>, info=0xb6bdd770, params=<optimized out>) at ../../../../../../hg/mctry/js/xpconnect/src/XPCWrappedJS.cpp:522
#25 0xb4dd34fe in PrepareAndDispatch (self=0xb14c6660, methodIndex=<optimized out>, args=<optimized out>) at ../../../../../../../../hg/mctry/xpcom/reflect/xptcall/md/unix/xptcstubs_arm.cpp:93
#26 0xb4dd2cc0 in SharedStub () from /home/jocelyn/mozilla/flame-kk/objdir-gecko/toolkit/library/libxul.so
#27 0xb57af0fa in mozilla::dom::bluetooth::BluetoothHfpManager::Init (this=this@entry=0xb1422ca0) at ../../../../../hg/mctry/dom/bluetooth2/bluedroid/hfp/BluetoothHfpManager.cpp:245
#28 0xb57afa6c in Get () at ../../../../../hg/mctry/dom/bluetooth2/bluedroid/hfp/BluetoothHfpManager.cpp:486
#29 mozilla::dom::bluetooth::BluetoothHfpManager::Get () at ../../../../../hg/mctry/dom/bluetooth2/bluedroid/hfp/BluetoothHfpManager.cpp:472
#30 0xb579de7a in mozilla::dom::bluetooth::BluetoothService::StopBluetooth (this=0xb3d3d250, aIsStartup=aIsStartup@entry=216, aRunnable=0xb0ca3d30)
    at ../../../../../hg/mctry/dom/bluetooth2/BluetoothService.cpp:405
#31 0xb579dfdc in mozilla::dom::bluetooth::BluetoothService::StartStopBluetooth (this=this@entry=0xb3d3d250, aStart=<optimized out>, aIsStartup=aIsStartup@entry=false, aRunnable=<optimized out>)
    at ../../../../../hg/mctry/dom/bluetooth2/BluetoothService.cpp:469

Comment 1

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Per discussion with Jocelyn, she found SIGTERM was been received after calling "settings->createLock".

  nsCOMPtr<nsISettingsServiceLock> settingsLock;
  nsresult rv = settings->CreateLock(nullptr, getter_AddRefs(settingsLock));
  NS_ENSURE_SUCCESS(rv, false);

  nsRefPtr<GetVolumeTask> callback = new GetVolumeTask();
  rv = settingsLock->Get(AUDIO_VOLUME_BT_SCO_ID, callback);
  NS_ENSURE_SUCCESS(rv, false);

Comment 2

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

It looks like aftering calling |settings->CreateLock|, Settings crash. But it really doesn't seem to be related to bluetooth. I will take a look further, somehow it blocks gaia new Bluetooth Settings developement.

Comment 3

[Jamin Liu [:jaliu][:Jamin]]

Take the bug since Jocelyn is on vacation.

Comment 4

[Jamin Liu [:jaliu][:Jamin]]

Attachment: (draft) Avoid to create Bluetooth profiles on content process since HFP manager need permission to create setting lock.

Comment 5

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Shawn Huang [:shawnjohnjr] from comment #2)
> It looks like aftering calling |settings->CreateLock|, Settings crash. But
> it really doesn't seem to be related to bluetooth. I will take a look
> further, somehow it blocks gaia new Bluetooth Settings developement.

It seems that the setting lock can't be created on content process.
I made a draft patch #attachment 8540470 [details] [diff] [review] to handle BT profiles related operations on chrome process and the settings app wouldn't crash after BT disabled.

Comment 6

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Avoid to create Bluetooth profiles on content process since HFP manager need permission to create setting lock. (v1)

Comment 7

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Can you confirm you tested all profiles get disconnected?

Comment 8

[Ben Tian (inactive)]

Comment on attachment 8540545 [details] [diff] [review]
Avoid to create Bluetooth profiles on content process since HFP manager need permission to create setting lock. (v1)

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

f=me with comment addressed. Thanks.

::: dom/bluetooth2/BluetoothService.cpp
@@ +407,5 @@
>     * Please see bug 892392 for more information.
>     */
>    if (aIsStartup || IsEnabled()) {
> +    // Switch Bluetooth off. If any Bluetooth profile is still connected, it
> +    // would be disconnected during this proess.

typo: process

Suggest to simplify as: // Any connected Bluetooth profile would be disconnected.

::: dom/bluetooth2/bluedroid/BluetoothUtils.h
@@ +76,5 @@
>                             const nsAString& aDeviceAddress,
>                             bool aStatus);
>  
> +/**
> + * Test whether this function is running at b2g porcess.

typo: process

@@ +78,5 @@
>  
> +/**
> + * Test whether this function is running at b2g porcess.
> + *
> + * @return returns whether the function is running at b2g porcess.

Revise @return as following:

  @return true if the function is running at b2g process, false otherwise.

::: dom/bluetooth2/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +224,5 @@
>  BluetoothHfpManager::Init()
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  // The function must run at b2g process since it would access SettingsService.

nit: Suggest to move this line forward to group assertions together.

  // The function must run at b2g process since it accesses SettingsService.
  MOZ_ASSERT(IsMainProcess());
  MOZ_ASSERT(NS_IsMainThread());

::: dom/bluetooth2/bluez/BluetoothHfpManager.cpp
@@ +418,5 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  // The function must run at b2g process since it would access SettingsService.
> +  MOZ_ASSERT(IsMainProcess());

Ditto.

Comment 9

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Avoid to create Bluetooth profiles on content process since HFP manager need permission to create setting lock. (v2), f=btian

- revise previous patch based on comment 8

Comment 10

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Shawn Huang [:shawnjohnjr] from comment #7)
> Can you confirm you tested all profiles get disconnected?

Got it.

Comment 11

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8540577 [details] [diff] [review]
Avoid to create Bluetooth profiles on content process since HFP manager need permission to create setting lock. (v2), f=btian

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

::: dom/bluetooth2/bluez/BluetoothDBusService.cpp
@@ +166,5 @@
> +    if (profile->IsConnected()) {
> +      profile->Disconnect(nullptr);
> +    } else {
> +      profile->Reset();
> +    }

Disconnection of profiles shall be done after 'if (!IsEnabled())' check.

Comment 12

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Avoid to create Bluetooth profiles on content process since HFP manager need permission to create setting lock. (v3), f=btian

- revise previous patch based on #comment 11.

Comment 13

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Shawn Huang [:shawnjohnjr] from comment #7)
> Can you confirm you tested all profiles get disconnected?

Since bluetooth app haven't support profile connection yet, I tested it by checking Reset() function instead.
As we expected, BluetoothHfpManager, BluetoothHidManager and BluetoothA2DPManager called Reset() during BluetoothAdapter.disable() process.

Comment 14

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8540687 [details] [diff] [review]
Avoid to create Bluetooth profiles on content process since HFP manager need permission to create setting lock. (v3), f=btian

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

LGTM. r=me

Comment 15

[Jamin Liu [:jaliu][:Jamin]]

It looks fine on treeherder.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=38d8070c6f14

Comment 16

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Avoid to create Bluetooth profiles on content process since HFP manager need permission to create setting lock. (v3), r=shuang, f=btian

- add reviewer's name to commit message 
- convert to hg format

Comment 17

[Ben Tian (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/rev/054216fee88a

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/054216fee88a