Closed Bug 1250694 Opened 4 years ago Closed 4 years ago

User is unable to send or receive files via bluetooth

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.6?, firefox47 fixed, b2g-v2.5 unaffected, b2g-master affected)

RESOLVED FIXED
blocking-b2g 2.6?
Tracking Status
firefox47 --- fixed
b2g-v2.5 --- unaffected
b2g-master --- affected

People

(Reporter: AdamA, Assigned: brsun)

References

()

Details

(Keywords: regression, smoketest, Whiteboard: [2.6-Daily-Testing][Spark])

Attachments

(3 files, 1 obsolete file)

Attached file logcat
Description:
User is unable to send or receive files via bluetooth to a computer or another phone.

Repro Steps:
1) Update a Aries to 20160223155508
2) Connect to a phone or computer via bluetooth
3) Attempt to send a file
4) Observe Transfer cancelled message 

Actual:
Users are unable to transfer files via bluetooth.

Expected:
It is expected that the user can transfer files via bluetooth

Environmental Variables:
Device: Aries 2.6 [Full Flash]
Build ID: 20160223155508
Gaia: 4f0e2a1a42a2d049b6fe8f4f095cdcdf0fd5465c
Gecko: a9e33d8c48b5ca93ca1937eba4220f681a0f05ec
Gonk: a19052e4389c3ae2d8fc3e7a74a475401baacc56
Version: 47.0a1 (2.6)
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:47.0) Gecko/47.0 Firefox/47.0

Repro frequency: 10/10
Link to failed test case: https://moztrap.mozilla.org/manage/case/6071/
See attached: video clip, logcat
blocking-b2g: --- → 2.6?
This issue DOES occur on Flame 2.6.

Environmental Variables:
Device: Flame 2.6 [Full Flash]
BuildID: 20160223075609
Gaia: 4f0e2a1a42a2d049b6fe8f4f095cdcdf0fd5465c
Gecko: a9e33d8c48b5ca93ca1937eba4220f681a0f05ec
Gonk: 8a066f7fa7410e32b58def35f322aa33f03db283
Version: 47.0a1 (2.6) 
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:47.0) Gecko/47.0 Firefox/47.0

Result:
Users are unable to transfer files via bluetooth.
----------------------------------
This issue DOES NOT occur on Aries 2.5.

Environmental Variables:
Device: Aries 2.5 [Full Flash]
BuildID: 20160216002303
Gaia: 28880004bc5cd6e129d0a23b701a389753788ed0
Gecko: 9d519d3bedacb7f5ce467f4c6bef7ff55ff3b0f3
Gonk: a19052e4389c3ae2d8fc3e7a74a475401baacc56
Version: 44.0 (2.5) 
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:44.0) Gecko/44.0 Firefox/44.0

Result:
User is able to send files via bluetooth
Summary: [Aries]User is unable to send or receive files via bluetooth → User is unable to send or receive files via bluetooth
QA Contact: jmercado
The changes for Bug 1222956 seem to have caused this issue.

Mozilla-inbound Regression Window

Last Working 
Environmental Variables:
Device: Flame 2.6
BuildID: 20160221191140
Gaia: 4f0e2a1a42a2d049b6fe8f4f095cdcdf0fd5465c
Gecko: e4345d1fa4657d41612472dcbf952dcc5fde3dd3
Version: 47.0a1 (2.6) 
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:47.0) Gecko/47.0 Firefox/47.0

First Broken 
Environmental Variables:
Device: Flame 2.6
BuildID: 20160221234458
Gaia: 4f0e2a1a42a2d049b6fe8f4f095cdcdf0fd5465c
Gecko: 807683706e4ed63cd348a3c748bcc028cb4c3d3e
Version: 47.0a1 (2.6) 
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:47.0) Gecko/47.0 Firefox/47.0

Last Working gaia / First Broken gecko - Issue DOES occur
Gaia: 4f0e2a1a42a2d049b6fe8f4f095cdcdf0fd5465c
Gecko: 807683706e4ed63cd348a3c748bcc028cb4c3d3e

First Broken gaia / Last Working gecko - Issue does NOT occur
Gaia: 4f0e2a1a42a2d049b6fe8f4f095cdcdf0fd5465c
Gecko: e4345d1fa4657d41612472dcbf952dcc5fde3dd3

Gecko Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e4345d1fa4657d41612472dcbf952dcc5fde3dd3&tochange=807683706e4ed63cd348a3c748bcc028cb4c3d3e
Blocks: 1222956
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Bruce can you please take a look at this?
Flags: needinfo?(brsun)
Thanks for reporting this issue. I'll take a look first.
Flags: needinfo?(brsun)
Currently |kObexObjectPush| is "00001105-0000-0000-0000-000000000000" instead of "00001105-0000-1000-8000-00805f9b34fb" from the log.

The weird result is due to so called "static initialization order fiasco", and there is no guarantee of the initialization order of |BluetoothUuid::BASE| and |kObexObjectPush| in |BluetoothOppManager.cpp|. One possible solution to prevent such problem is to use |constexpr| declarations to forcibly generate proper values at compile time.

Since there are no viable ways to proper declare |static constexpr BluetoothUuid| data members inside |BluetoothUuid| itself, |static constexpr| member functions are used instead. One |BluetoothUuid| constructor is modified as well to fit the requirement of a |constexpr| constructor.
Attachment #8722925 - Flags: review?(tzimmermann)
Comment on attachment 8722925 [details] [diff] [review]
bug1250694_bluetoothuuid_constexpr.patch

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

Hi!

Thanks a lot for this fix. I ran into such an issue with |BluetoothAddress| before. Sorry that I didn't see this during the review. The new approach should fix the problem once and for all, but please see my comment below.

::: dom/bluetooth/common/BluetoothCommon.cpp
@@ -22,5 @@
>  const BluetoothAddress BluetoothAddress::ALL(0xff, 0xff, 0xff,
>                                               0xff, 0xff, 0xff);
>  
>  const BluetoothAddress BluetoothAddress::LOCAL(0x00, 0x00, 0x00,
>                                                 0xff, 0xff, 0xff);

Maybe we should do something similar for |BluetoothAddress|.

::: dom/bluetooth/common/BluetoothCommon.h
@@ +581,5 @@
>    { }
>  
>    MOZ_IMPLICIT BluetoothUuid(const BluetoothUuid&) = default;
>  
> +  constexpr BluetoothUuid(uint8_t aUuid0, uint8_t aUuid1,

I have some doubts about this. 'constexpr' requires the statement to be evaluable at compile time. Passing actual variables for the parameters would not satisfy this constraint. I'd say you should keep the non-'constexpr' constructor as well.
Attachment #8722925 - Flags: review?(tzimmermann) → review+
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Hi Thomas,

Don't say sorry cause it's my fault to break the file transfer functionality.

> ::: dom/bluetooth/common/BluetoothCommon.cpp
> @@ -22,5 @@
> >  const BluetoothAddress BluetoothAddress::ALL(0xff, 0xff, 0xff,
> >                                               0xff, 0xff, 0xff);
> >  
> >  const BluetoothAddress BluetoothAddress::LOCAL(0x00, 0x00, 0x00,
> >                                                 0xff, 0xff, 0xff);
> 
> Maybe we should do something similar for |BluetoothAddress|.
> 

I can handle this as well.

> ::: dom/bluetooth/common/BluetoothCommon.h
> @@ +581,5 @@
> >    { }
> >  
> >    MOZ_IMPLICIT BluetoothUuid(const BluetoothUuid&) = default;
> >  
> > +  constexpr BluetoothUuid(uint8_t aUuid0, uint8_t aUuid1,
> 
> I have some doubts about this. 'constexpr' requires the statement to be
> evaluable at compile time. Passing actual variables for the parameters would
> not satisfy this constraint. I'd say you should keep the non-'constexpr'
> constructor as well.

From the explanation of 'constexpr'[1], this keyword just provides the possibility for the function to be evaluated at compile time, depending on whether the passing parameters can be evaluated at compile time. It is not compilable by declaring two constructors with the same arguments lists but with different 'constexpr' declarations only.

[1] http://en.cppreference.com/w/cpp/language/constexpr
> From the explanation of 'constexpr'[1], this keyword just provides the
> possibility for the function to be evaluated at compile time
> [1] http://en.cppreference.com/w/cpp/language/constexpr

Sorry, I was reading the section on variables.
It seems visual studio 2013[1][2] on windows doesn't like 'constexpr'. I'd better to find another way to solve this problem in order to make codes compilable on all platforms.

[1] https://msdn.microsoft.com/en-us/library/hh567368.aspx
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Windows_Prerequisites
'constexpr' is not supported by Visual Studio 2013[1] on Windows. Uniform initialization for member arrays in the constructor is also not supported by it[2][3], either. So it seems we cannot workaround the portability by simply using |MOZ_CONSTEXPR| for all platforms.

Static functions are used instead in this patch.

[1] https://msdn.microsoft.com/en-us/library/hh567368.aspx
[2] http://stackoverflow.com/questions/19877757/workaround-for-error-c2536-cannot-specify-explicit-initializer-for-arrays-in-vi
[3] https://connect.microsoft.com/VisualStudio/feedback/details/792161/constructor-initializer-list-does-not-support-braced-init-list-form
Attachment #8722925 - Attachment is obsolete: true
Attachment #8726098 - Flags: review?(tzimmermann)
Comment on attachment 8726098 [details] [diff] [review]
bug1250694_bluetoothaddress_bluetoothuuid_static_function.patch

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

Hi

I don't use the MS compiler, but I didn't here a lot of good about it. :( r+ with the comment addressed.

::: dom/bluetooth/common/BluetoothCommon.cpp
@@ +18,5 @@
>  
> +const BluetoothAddress& BluetoothAddress::ANY()
> +{
> +  static const BluetoothAddress sAddress = BluetoothAddress(0x00, 0x00, 0x00,
> +                                                            0x00, 0x00, 0x00);

Simply construct without the assignment?
Attachment #8726098 - Flags: review?(tzimmermann) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef9e220beb2d1502c0f60cb9e1ac6351e8a9c0fb
Bug 1250694: Use functions to return static const BluetoothAddress and BluetoothUuid; r=tzimmermann
Fix ICS build break.
Attachment #8726664 - Flags: review?(tzimmermann)
Attachment #8726664 - Flags: review?(tzimmermann)
Update: ICS build break due to the commit ef9e220beb2d. Back it out and resubmit the patch again.
https://hg.mozilla.org/mozilla-central/rev/ef9e220beb2d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.