Closed Bug 949772 Opened 10 years ago Closed 10 years ago

MOZ_ASSERT(arr.Length() == 1) in mozilla::dom::bluetooth::BluetoothAdapter::Notify

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

x86
macOS
defect
Not set
normal

Tracking

(blocking-b2g:-)

RESOLVED FIXED
blocking-b2g -

People

(Reporter: gwagner, Assigned: echou)

Details

Attachments

(2 files, 3 obsolete files)

Seen on nexus4 with current gecko/gaia trunk and debug gecko build.

STR:
start music app
Enable/disable nfc, bluetooth a few times:


Program received signal SIGSEGV, Segmentation fault.
0xb554985a in mozilla::dom::bluetooth::BluetoothAdapter::Notify (this=0xb1fd3c00, aData=...) at ../../../dom/bluetooth/BluetoothAdapter.cpp:321
321	    MOZ_ASSERT(arr.Length() == 1);
(gdb) bt
#0  0xb554985a in mozilla::dom::bluetooth::BluetoothAdapter::Notify (this=0xb1fd3c00, aData=...) at ../../../dom/bluetooth/BluetoothAdapter.cpp:321
#1  0xb554dedc in Broadcast (aParam=<optimized out>, this=<optimized out>) at ../../dist/include/mozilla/Observer.h:67
#2  mozilla::dom::bluetooth::BluetoothService::DistributeSignal (this=<optimized out>, aSignal=...) at ../../../dom/bluetooth/BluetoothService.cpp:458
#3  0xb555051a in mozilla::dom::bluetooth::BluetoothChild::RecvNotify (this=<optimized out>, aSignal=<optimized out>)
    at ../../../dom/bluetooth/ipc/BluetoothChild.cpp:78
#4  0xb4fb84d2 in mozilla::dom::bluetooth::PBluetoothChild::OnMessageReceived (this=0xb1f43af0, __msg=...) at PBluetoothChild.cpp:364
#5  0xb4fdd6ae in mozilla::dom::PContentChild::OnMessageReceived (this=0xb3e44818, __msg=...) at PContentChild.cpp:3155
#6  0xb4f9405c in mozilla::ipc::MessageChannel::DispatchAsyncMessage (this=0xb3e44848, aMsg=...) at ../../../ipc/glue/MessageChannel.cpp:986
#7  0xb4f96834 in mozilla::ipc::MessageChannel::OnMaybeDequeueOne (this=0xb3e44848) at ../../../ipc/glue/MessageChannel.cpp:887
#8  0xb4f93954 in DispatchToMethod<mozilla::ipc::MessageChannel, void (mozilla::ipc::MessageChannel::*)()> (method=
    (void (mozilla::ipc::MessageChannel::*)(mozilla::ipc::MessageChannel * const)) 0xb4f9679d <mozilla::ipc::MessageChannel::OnMaybeDequeueOne()>, 
    obj=<optimized out>, arg=<optimized out>) at ../../../ipc/chromium/src/base/tuple.h:383
#9  RunnableMethod<mozilla::ipc::MessageChannel, void (mozilla::ipc::MessageChannel::*)(), Tuple0>::Run (this=<optimized out>)
    at ../../../ipc/chromium/src/base/task.h:307
#10 0xb4f944c0 in Run (this=<optimized out>) at ../../dist/include/mozilla/ipc/MessageChannel.h:441
#11 mozilla::ipc::MessageChannel::DequeueTask::Run (this=<optimized out>) at ../../dist/include/mozilla/ipc/MessageChannel.h:458
#12 0xb4f8a318 in MessageLoop::RunTask (this=0xbe95af20, task=0xb2058900) at ../../../ipc/chromium/src/base/message_loop.cc:344
#13 0xb4f8ac56 in MessageLoop::DeferOrRunPendingTask (this=<optimized out>, pending_task=<optimized out>) at ../../../ipc/chromium/src/base/message_loop.cc:352
#14 0xb4f8bcfa in DoWork (this=<optimized out>) at ../../../ipc/chromium/src/base/message_loop.cc:452
#15 MessageLoop::DoWork (this=0xbe95af20) at ../../../ipc/chromium/src/base/message_loop.cc:431
#16 0xb4f9788e in mozilla::ipc::DoWorkRunnable::Run (this=<optimized out>) at ../../../ipc/glue/MessagePump.cpp:226
#17 0xb4e0cadc in ProcessNextEvent (result=0xbe95adc7, mayWait=<optimized out>, this=0xb3e2e800) at ../../../xpcom/threads/nsThread.cpp:634
#18 nsThread::ProcessNextEvent (this=0xb3e2e800, mayWait=<optimized out>, result=0xbe95adc7) at ../../../xpcom/threads/nsThread.cpp:567
#19 0xb4dcb0cc in NS_ProcessNextEvent (thread=0xb3e2e800, mayWait=<optimized out>) at ../../../xpcom/glue/nsThreadUtils.cpp:263
Thanks for reporting. I will check later.
Steal!
Assignee: nobody → echou
I was unable to reproduce, but I did find something wrong and I believe that would be the root cause which make this occur. The patch is coming.
Attachment #8347952 - Flags: review?(gyeh)
Attachment #8347952 - Flags: feedback?(btian)
Comment on attachment 8347952 [details] [diff] [review]
patch 1: v1: event PropertyChanged should only contains one element

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

LGTM.
Attachment #8347952 - Flags: feedback?(btian) → feedback+
Comment on attachment 8347952 [details] [diff] [review]
patch 1: v1: event PropertyChanged should only contains one element

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

How about handling multiple |PropertyChanged| sequentially in BluetoothAdapter? When backend returns several pairs of name and value at one time, we just need to broadcast the signal once and let each instance of BluetoothAdapter to update its own values.
* Updated based on Gina's comment.
Attachment #8347952 - Attachment is obsolete: true
Attachment #8347952 - Flags: review?(gyeh)
Attachment #8347967 - Flags: review?(gyeh)
Attachment #8347967 - Flags: feedback?(btian)
Comment on attachment 8347967 [details] [diff] [review]
patch 1: v2: Make BluetoothAdpater be able to deal with multiple properties in one PropertyChanged event

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

Nothing to pick on. :)
Attachment #8347967 - Flags: review?(gyeh) → review+
Comment on attachment 8347967 [details] [diff] [review]
patch 1: v2: Make BluetoothAdpater be able to deal with multiple properties in one PropertyChanged event

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

f=me with nit addressed.

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +321,2 @@
>  
> +    for (uint32_t i = 0; i < propertyCount; ++i) {

You can check i < arr.length() directly.
Attachment #8347967 - Flags: feedback?(btian) → feedback+
Nominate as 1.3+ since 1.3 would be affected.
blocking-b2g: --- → 1.3?
> ::: dom/bluetooth/BluetoothAdapter.cpp
> @@ +321,2 @@
> >  
> > +    for (uint32_t i = 0; i < propertyCount; ++i) {
> 
> You can check i < arr.length() directly.

Creating a variable must be more efficient(or at least won't be less efficient) than getting unchanged length by calling length() every time.
* Per offline discussion with Gina, we should notify registering BluetoothDevice objects as well as what we do to BluetoothAdapter.
Attachment #8348549 - Flags: review?(gyeh)
(In reply to Eric Chou [:ericchou] [:echou] from comment #13)
> Created attachment 8348549 [details] [diff] [review]
> patch 2: v1: Fire PropertyChanged to registering BluetoothDevice objects
> 
> * Per offline discussion with Gina, we should notify registering
> BluetoothDevice objects as well as what we do to BluetoothAdapter.

In addition, 2 callback functions are renamed in this patch since they are just callbacks to setter and getter, 'Changed' shouldn't be necessary.
triage: not blocking release as there seem to be no major user impact. please work on trunk and ask for approval to land in 1.3
blocking-b2g: 1.3? → -
Comment on attachment 8348549 [details] [diff] [review]
patch 2: v1: Fire PropertyChanged to registering BluetoothDevice objects

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

r+ with nits addressed.

::: dom/bluetooth/bluedroid/gonk/BluetoothServiceBluedroid.cpp
@@ +385,1 @@
>   * following conditions:

nit: Please merge them into one line.

@@ +427,4 @@
>      }
>    }
>  
> +  // Update to registering BluetoothDevice objects

nit: registered

@@ +429,5 @@
>  
> +  // Update to registering BluetoothDevice objects
> +  BluetoothValue value(props);
> +  BluetoothSignal signal(NS_LITERAL_STRING("PropertyChanged"),
> +                         remoteDeviceBdAddress, value);

nit: You can use anonymous object for |value| if you don't need it after creating BluetoothSignal.
Attachment #8348549 - Flags: review?(gyeh) → review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: