[Buri] FFOS crash in mozilla::dom::bluetooth::BluetoothNamedValue*

RESOLVED FIXED

Status

--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: marcia, Assigned: echou)

Tracking

({crash})

unspecified
ARM
Gonk (Firefox OS)
crash

Firefox Tracking Flags

(blocking-b2g:-)

Details

(Whiteboard: [b2g-crash], crash signature)

Attachments

(1 attachment, 5 obsolete attachments)

This bug was filed from the Socorro interface and is 
report bp-298b6b0e-3379-4eb0-9dfb-9fdc22130822 .
 ============================================================= 

Found while running on Buri and doing some bluetooth pairing. At the time of the crash the device was idle.

STR:
1. Turn on bluetooth on the Buri device.
2. Attempt to pair with the Mac desktop
3. After several timeouts, the device crashed. I was purposely letting the "Pair" dialog on the phone sit until it timed out.

Frame	Module	Signature	Source
0	libxul.so	mozilla::dom::bluetooth::BluetoothNamedValue* nsTArray_Impl<mozilla::dom::bluetooth::BluetoothNamedValue, nsTArrayInfallibleAllocator>::AppendElement<mozilla::dom::bluetooth::BluetoothNamedValue>(mozilla::dom::bluetooth::BluetoothNamedValue const&)	/builds/slave/b2g_m-cen_hamachi_ntly-0000000/build/objdir-gecko/dom/bluetooth/../../dist/include/nsTSubstring.h
1	libxul.so	BluetoothArrayOfDevicePropertiesRunnable::Run()	dom/bluetooth/linux/BluetoothDBusService.cpp
2	libxul.so	nsThread::ProcessNextEvent(bool, bool*)	xpcom/threads/nsThread.cpp
3	libxul.so	NS_ProcessNextEvent(nsIThread*, bool)	/builds/slave/b2g_m-cen_hamachi_ntly-0000000/build/objdir-gecko/xpcom/build/nsThreadUtils.cpp
4	libxul.so	nsThread::ThreadFunc(void*)	xpcom/threads/nsThread.cpp
5	libnss3.so	_pt_root	nsprpub/pr/src/pthreads/ptthread.c
6	libc.so	__thread_entry	bionic/libc/bionic/pthread.c
7	libc.so	pthread_create	bionic/libc/bionic/pthread.c
Two ideas:

 - UnpackPropertiesMessage at BluetoothDBusService.cpp:731 returns true in the case of an error, and
 - GetPropertiesInternal might return true, even if the received message was NULL.
(Assignee)

Comment 2

5 years ago
Thanks for reporting this.

The code around the crash point is:

  for (uint32_t i = 0; i < mDeviceAddresses.Length(); i++) {
    BluetoothValue v;
    nsString objectPath = GetObjectPathFromAddress(sAdapterPath, mDeviceAddresses[i]);

    if (!GetPropertiesInternal(objectPath, DBUS_DEVICE_IFACE, v)) {
      errorStr.AssignLiteral("Getting properties failed!");
      DispatchBluetoothReply(mRunnable, values, errorStr);
      return NS_OK;
    }

    v.get_ArrayOfBluetoothNamedValue().AppendElement(
      BluetoothNamedValue(NS_LITERAL_STRING("Path"), objectPath)
    );  ==============> Crash

The crash occurred because the type of 'v' was not TArrayOfBluetoothNamedValue but we still tried to call v.get_ArrayOfBluetoothNamedValue().

So the problem becomes: why the out parameter v was not assigned any value but function GetPropertiesInternal() still returned true? This is unreasonable and needs to be fixed.
Assignee: nobody → echou
(Assignee)

Comment 3

5 years ago
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #1)
> Two ideas
>
>  - UnpackPropertiesMessage at BluetoothDBusService.cpp:731 returns true in
> the case of an error, and
>  - GetPropertiesInternal might return true, even if the received message was
> NULL.

I think the first guess is correct. I'll make a patch to fix it.
> The crash occurred because the type of 'v' was not
> TArrayOfBluetoothNamedValue but we still tried to call
> v.get_ArrayOfBluetoothNamedValue().
> 
> So the problem becomes: why the out parameter v was not assigned any value
> but function GetPropertiesInternal() still returned true? This is
> unreasonable and needs to be fixed.

Yup, that's what I was talking about in comment 1. These are the two things I noticed when looking at GetPropertiesInternal. I'm currently trying to somehow reproduce the issue.
(In reply to Eric Chou [:ericchou] [:echou] from comment #3)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #1)
> > Two ideas
> >
> >  - UnpackPropertiesMessage at BluetoothDBusService.cpp:731 returns true in
> > the case of an error, and
> >  - GetPropertiesInternal might return true, even if the received message was
> > NULL.
> 
> I think the first guess is correct. I'll make a patch to fix it.

Just triggered them explicitly and both cases lead to a crash.
(Assignee)

Comment 6

5 years ago
Created attachment 794586 [details] [diff] [review]
Patch 1: v1: UnpackPropertiesMessage() should return false if there's an error

This patch fixes problem (1) mentioned by Thomas in comment 1.
Attachment #794586 - Flags: review?(gyeh)
Attachment #794586 - Flags: feedback?(tzimmermann)
Comment on attachment 794586 [details] [diff] [review]
Patch 1: v1: UnpackPropertiesMessage() should return false if there's an error

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

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ -799,2 @@
>                                              DBUS_TYPE_INVALID);
>  

When msg is NULL, a crash happens within IsDBusMessageError, which is called by UnpackPropertiesMessage. We need to return false before this point.
Attachment #794586 - Flags: feedback?(tzimmermann) → feedback-
(Assignee)

Comment 8

5 years ago
Created attachment 794601 [details] [diff] [review]
patch 2: v1: GetPropertiesInternal() should return false when the received message was NULL

This patch fixes problem (2) mentioned by Thomas in comment 1.
Attachment #794601 - Flags: review?(gyeh)
Attachment #794601 - Flags: feedback?(tzimmermann)
(Assignee)

Comment 9

5 years ago
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #7)
> Comment on attachment 794586 [details] [diff] [review]
> Patch 1: v1: UnpackPropertiesMessage() should return false if there's an
> error
> 
> Review of attachment 794586 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/linux/BluetoothDBusService.cpp
> @@ -799,2 @@
> >                                              DBUS_TYPE_INVALID);
> >  
> 
> When msg is NULL, a crash happens within IsDBusMessageError, which is called
> by UnpackPropertiesMessage. We need to return false before this point.

Yeah, I know. That's why I said this patch would only fix problem (1) you've mentioned in comment 1. Please see my 2nd patch. Thanks!
(Assignee)

Comment 10

5 years ago
Created attachment 794605 [details] [diff] [review]
patch 2: v2: GetPropertiesInternal() should return false when the received message was NULL

* Added an assertion in UnpackPropertiesMessage() to ensure aMsg is valid.
Attachment #794601 - Attachment is obsolete: true
Attachment #794601 - Flags: review?(gyeh)
Attachment #794601 - Flags: feedback?(tzimmermann)
Attachment #794605 - Flags: review?(gyeh)
Attachment #794605 - Flags: feedback?(tzimmermann)
Comment on attachment 794605 [details] [diff] [review]
patch 2: v2: GetPropertiesInternal() should return false when the received message was NULL

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

The first patch touched GetPropertiesInternal, so I though you're attempting to fix the second problem as well. I was wondering about the comment though.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +804,4 @@
>  
>    bool success = UnpackPropertiesMessage(msg, &err, aValue, aIface);
>  
>    if (msg) {

The condition can be removed.
Attachment #794605 - Flags: feedback?(tzimmermann) → feedback+
Comment on attachment 794586 [details] [diff] [review]
Patch 1: v1: UnpackPropertiesMessage() should return false if there's an error

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

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +1826,5 @@
>      nsString objectPath = v.get_nsString();
>      v = InfallibleTArray<BluetoothNamedValue>();
>      if (!GetPropertiesInternal(objectPath, DBUS_ADAPTER_IFACE, v)) {
> +      replyError = NS_ConvertUTF8toUTF16("GetPropertiesInternal: failed");
> +      DispatchBluetoothReply(mRunnable, v, replyError);

I couldn't find these lines in my code base. Did I miss something?

@@ +2021,5 @@
>  
>        if (!GetPropertiesInternal(objectPath, DBUS_DEVICE_IFACE, v)) {
> +        // The target device may have been removed, so continue checking the
> +        // next device object.
> +        continue;

Nice catch!
Comment on attachment 794605 [details] [diff] [review]
patch 2: v2: GetPropertiesInternal() should return false when the received message was NULL

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

Looks great.
Attachment #794605 - Flags: review?(gyeh) → review+
(Assignee)

Comment 14

5 years ago
Hi Thomas,

> 
> ::: dom/bluetooth/linux/BluetoothDBusService.cpp
> @@ +804,4 @@
> >  
> >    bool success = UnpackPropertiesMessage(msg, &err, aValue, aIface);
> >  
> >    if (msg) {
> 
> The condition can be removed.

I think we should still keep this condition check since msg is possible to be non-null. Make sense?
(Assignee)

Comment 15

5 years ago
Created attachment 795814 [details] [diff] [review]
pacth 1: v2: UnpackPropertiesMessage() should return false if there's an error

Hi Thomas,

I took your advice to merge previous two simple patches. Please take a look. Thanks.

Hi Gina,

The part of code you mentioned is missing because patches need to be rebased. Please review again. Thanks.
Attachment #794586 - Attachment is obsolete: true
Attachment #794605 - Attachment is obsolete: true
Attachment #794586 - Flags: review?(gyeh)
Attachment #795814 - Flags: review?(gyeh)
Attachment #795814 - Flags: feedback?(tzimmermann)
(Assignee)

Comment 16

5 years ago
(In reply to Eric Chou [:ericchou] [:echou] from comment #14)
> Hi Thomas,
> 
> > 
> > ::: dom/bluetooth/linux/BluetoothDBusService.cpp
> > @@ +804,4 @@
> > >  
> > >    bool success = UnpackPropertiesMessage(msg, &err, aValue, aIface);
> > >  
> > >    if (msg) {
> > 
> > The condition can be removed.
> 
> I think we should still keep this condition check since msg is possible to
> be non-null. Make sense?

And NOW I know what you mean! Thanks for Gina's hint!

I'll update the patch later. :P
(Assignee)

Comment 17

5 years ago
Created attachment 795823 [details] [diff] [review]
patch 1: v3: UnpackPropertiesMessage() should return false if there's an error

* Remove the condition check
Attachment #795814 - Attachment is obsolete: true
Attachment #795814 - Flags: review?(gyeh)
Attachment #795814 - Flags: feedback?(tzimmermann)
Attachment #795823 - Flags: review?(gyeh)
Attachment #795823 - Flags: feedback?(tzimmermann)
(Assignee)

Comment 18

5 years ago
Created attachment 795827 [details] [diff] [review]
patch 1: v4: UnpackPropertiesMessage() should return false if there's an error

* Forgot to remove another condition check ;)
Attachment #795823 - Attachment is obsolete: true
Attachment #795823 - Flags: review?(gyeh)
Attachment #795823 - Flags: feedback?(tzimmermann)
Attachment #795827 - Flags: review?(gyeh)
Attachment #795827 - Flags: feedback?(tzimmermann)
Comment on attachment 795827 [details] [diff] [review]
patch 1: v4: UnpackPropertiesMessage() should return false if there's an error

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

Perfect! Thanks for your effort.
Attachment #795827 - Flags: review?(gyeh) → review+
Attachment #795827 - Flags: feedback?(tzimmermann) → feedback+
(Assignee)

Comment 21

5 years ago
Nominate as leo+ to prevent this from happening in b2g18.
blocking-b2g: --- → leo?
https://hg.mozilla.org/mozilla-central/rev/df51e3968d00
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Thanks for the fix and I think we will get this out in future release.Adding verifyme to verify this.

For leo, not a blocker because I don't think step 3 in the STR is a common-enough scenario.
blocking-b2g: leo? → -
Keywords: verifyme

Comment 24

5 years ago
Verified Fixed On latest 1.4, 1.3, and 1.2 using Buri.

Environmental Variables:
Device: Buri 1.4 MOZ
BuildID: 20140103040201
Gaia: 83cc63f728489a24256731adf558354bb2012a59
Gecko: 49d2fce9a86c
Version: 29.0a1
Firmware Version: v1.2_20131115

Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20140103004001
Gaia: ae7d05689b6b9ac4ec6182217dfdef06be28e886
Gecko: d9226a660d52
Version: 28.0a2
Firmware Version: v1.2_20131115

Environmental Variables:
Device: Buri 1.2 MOZ
BuildID: 20140103004001
Gaia: 2b116456d8a3ed3e9741b370d628f225c58587da
Gecko: 9cbbf14a0f69
Version: 26.0
Firmware Version: v1.2_20131115
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.