Closed Bug 1096266 Opened 10 years ago Closed 9 years ago

[bluetooth2] Settings crash after calling Bluetooth v2 api disable

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

Product:
Firefox OS Graveyard
Component:
Bluetooth
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: shawnjohnjr, Assigned: jaliu)

Details

Attachments

(1 file, 4 obsolete files)

Avoid to create Bluetooth profiles on content process since HFP manager need permission to create setting lock. (v3), r=shuang, f=btian
9.53 KB, patch
Details | Diff | Splinter Review 
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
Assignee: nobody → joliu
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);
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.
Take the bug since Jocelyn is on vacation.
Assignee: joliu → jaliu
Status: NEW → ASSIGNED
(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.
Attachment #8540470 - Flags: feedback+
Can you confirm you tested all profiles get disconnected?
Flags: needinfo?(jaliu)
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.
Attachment #8540545 - Flags: feedback?(btian) → feedback+
- revise previous patch based on comment 8
Attachment #8540545 - Attachment is obsolete: true
Attachment #8540545 - Flags: review?(shuang)
Attachment #8540577 - Flags: review?(shuang)
(In reply to Shawn Huang [:shawnjohnjr] from comment #7)
> Can you confirm you tested all profiles get disconnected?

Got it.
Flags: needinfo?(jaliu)
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.
Attachment #8540577 - Flags: review?(shuang)
- revise previous patch based on #comment 11.
Attachment #8540577 - Attachment is obsolete: true
Attachment #8540687 - Flags: review?(shuang)
(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 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
Attachment #8540687 - Flags: review?(shuang) → review+
- add reviewer's name to commit message 
- convert to hg format
Attachment #8540687 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/054216fee88a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: