1002391 - [Flame] Could not send image/video via NFC

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Alison Shiue]

Attachment: nfc_send_image.log

Device    Flame
Gaia      265e1ac4ee71ad6190335c974bfce33f783edfce
Gecko     https://hg.mozilla.org/mozilla-central/rev/ea25086073c8
BuildID   20140427160202
Version   31.0a1


STR:
1. launch "settings" app for 2 phones
2. turn on "NFC" for 2 phones
3. open an image on phone A 
4. tap 2 phones
5. swipe shrinking UI on phone A

Expected result:
phone B receive image shared by phone A

Actual result:
phone B could not receive image

Comment 1

[Ken Chang[:kenkai]]

We are going to use flame as a reference device.

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

After discussed with Eric, this is a Bluetooth bug.
re-assign to him.

Comment 3

[Ken Chang[:kenkai]]

It is a blocker for NFC 2.0 release.

Comment 4

[Mishra]

Device    Nexus 4
BuildID   20140516110816
Version   32.0a1
Git Commit info 2014-05-05 16:59:23 d33e4f15

I tested above on nexus 4 and the issue is not present

Comment 5

[Eric Chou [:ericchou] [:echou]]

(In reply to Mishra from comment #4)
> Device    Nexus 4
> BuildID   20140516110816
> Version   32.0a1
> Git Commit info 2014-05-05 16:59:23 d33e4f15
> 
> I tested above on nexus 4 and the issue is not present

Yes, this is BlueZ specific bug so it won't be reproducible on Nexus 4/5 which use Bluedroid as bt stack.

Thanks for your test. :)

Comment 6

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

update ETA to 6/9 per discussion with Eric.

Comment 7

[Dimi Lee [:dimi]]

There is a javascript "let is a reserved identifier" error in ndef_utils.js when trying to send file

Comment 8

[Eric Chou [:ericchou] [:echou]]

I've been working on this these two days and I've already had a solution. I'll provide a patch tomorrow to fix this.

Comment 9

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 1: v1: Get the latest SDP records of the target

Tested on my Flame.

Comment 10

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

Comment on attachment 8435555 [details] [diff] [review]
patch 1: v1: Get the latest SDP records of the target

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

It looks good to me. In general, I think we need to consider if the remote is using rfcomm secured socket with MITM in the long term, for non-MITM case, I'm ok with it :)

::: dom/bluetooth/bluez/BluetoothDBusService.cpp
@@ +3647,5 @@
>      MOZ_ASSERT(!sAdapterPath.IsEmpty());
>  
> +    // We first guess that the device doesn't exist at all. So we use BlueZ
> +    // API "CreateDevice" to create an object path for the BluetoothDevice
> +    // object.

nits:Can we add something like "Update SDP records before calling DBUS API 'CreateDevice for NFC use case" to make comments easy to understand? In a normal use case, we don't need to do 'CreateDevice' before query SDP records before pairing procedure always performed earlier.

@@ +3648,5 @@
>  
> +    // We first guess that the device doesn't exist at all. So we use BlueZ
> +    // API "CreateDevice" to create an object path for the BluetoothDevice
> +    // object.
> +    NS_ConvertUTF16toUTF8 address(mDeviceAddress);

nits:Please validate mDeviceAddress in the beginning.

@@ +3679,5 @@
> +      // SDP records then we have to do "DiscoverServices"
> +      BT_LOGR("%s", NS_ConvertUTF16toUTF8(errorString).get());
> +
> +      OnUpdateSdpRecordsRunnable* r =
> +        static_cast<OnUpdateSdpRecordsRunnable*>(aData);

OnUpdateSdpRecordsRunnable shall be moved outside if-block, right?

@@ +3686,5 @@
> +      r->GetDeviceAddress(deviceAddress);
> +
> +      const nsString objectPath =
> +        GetObjectPathFromAddress(sAdapterPath, deviceAddress);
> +

We need to add if-guard to make sure Bluetooth is not in the middle of turning on/off. Otherwise, I'm afraid that we might see Bug 1015810. I think they are the similar scenario.

Comment 11

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 1: v2: Get the latest SDP records of the target

* Patch updated.

Comment 12

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

Comment on attachment 8435680 [details] [diff] [review]
patch 1: v2: Get the latest SDP records of the target

LGTM, thanks.

Comment 13

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

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #10)
> Comment on attachment 8435555 [details] [diff] [review]
> patch 1: v1: Get the latest SDP records of the target
> 
> Review of attachment 8435555 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 
> We need to add if-guard to make sure Bluetooth is not in the middle of
> turning on/off. Otherwise, I'm afraid that we might see Bug 1015810. I think
> they are the similar scenario.
My bad, Bug 1015810 won't happen, because now we handle things in I/O thread.

Comment 14

[Eric Chou [:ericchou] [:echou]]

https://hg.mozilla.org/integration/b2g-inbound/rev/03e02a8a1677

Comment 15

[Eric Chou [:ericchou] [:echou]]

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #12)
> Comment on attachment 8435680 [details] [diff] [review]
> patch 1: v2: Get the latest SDP records of the target
> 
> LGTM, thanks.

Thanks! :D

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/03e02a8a1677

Comment 17

[Alison Shiue]

Verified on
Device    Flame (Nexus 4)
Gaia      2e5636e852a9354a5f8072b179cf16f72647cfd6
Gecko     https://hg.mozilla.org/mozilla-central/rev/8bd92dc9ef59
BuildID   20140608160201
Version   32.0a1

Comment 18

[Martin Shuman [:Marty]]

Verified fixed. Issue does not occur in latest Flame 2.0.
NFC is able to properly transfer image and video files.

Environmental Variables:
Device: Flame 2.0
BuildID: 20140910000203
Gaia: 3f4c635106c5364228782d12b1cb76b0c105b971
Gecko: 02a5b9234c13
Version: 32.0 (2.0)
Firmware: V123
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0