821186 - [Gaia::Bluetooth] Remove workaround of using deviceStorage in bluetooth transfer after Bug 811615 is fixed

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Bluetooth, defect, P3)

Comment 1

[Dominic Kuo [:dkuo]]

Since bug 811615 is fixed, we can pass File by web activities normally, it will not turn into Blob again.
So as title, bluetooth transfer should remove the workaround of using deviceStorage, now we can directly receive the File instead of retrieving the File again from deviceStorage.
This will effect bluetooth transfer in System app and Bluetooth app.

Comment 2

[[e-mail], [e-mail]]

We should remove the useless workaround of using deviceStorage to query the file again. It may wastes time and memory.
Could we let the issue be bb+?

Comment 3

[Joe Cheng [:jcheng] (please needinfo)]

Triage: BB+, P3 - to remove unnecessary code

Comment 4

[David Flanagan [:djf]]

I'm happy to review this when you have a patch ready.

Comment 5

[Josh Carpenter [:jcarpenter]]

Ian what is the performance impact to the user of leaving this code in? Slow transfer? Slow initialization time, etc?

Comment 6

[Alex Keybl [:akeybl]]

Given the lack of concrete user impact noted here ("may wastes time and memory"), we'll track for v1.x

Comment 7

[[e-mail], [e-mail]]

Attachment: Remove workaround of using deviceStorage in bluetooth sending file

Hi Eric,
I try to remove the workaround. Then, we can get the filename information from the blob. It works on sending file from unagi to SS galaxy tab. But it don't work on sending file from unagi to unagi. I provide the patch with debug mode on. We could need to investigate the problem together.

Comment 8

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

Comment on attachment 707978 [details]
Remove workaround of using deviceStorage in bluetooth sending file

For the part of passing a blob directly into function sendFile(), looks good. I've cherry-picked your patch and tested, seems to work fine.

Comment 9

[[e-mail], [e-mail]]

Eric,

It doesn't work for me. What gecko version did you test?

Comment 10

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

I was using the latest mozilla-b2g18

Comment 11

[[e-mail], [e-mail]]

I just test the latest mozilla-b2g18 by version 2013-02-21-23-02-03. The sending process is able to run on defaultAdapter.sendFile(). But the file could not be sent.

There are two different status for following test case:

Unagi --> Unagi
Two of the Unagi receive message with "File could not be sent" immediately.

Unagi --> GalaxyTab GT-P6800
Sometimes Galaxy won't receive transferring file request.
Sometimes it works normally.

Comment 12

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

The nightly build 2013-02-23-23-02-04 of b2g-18-unagi works for me. Moreover, just tried latest b2g-18/gecko with latest v1-train/gaia and it worked well, too.

Comment 13

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

Forgot to mention, I used Samsung GS2.

Comment 14

[[e-mail], [e-mail]]

Leave me experiment..
I find out a strange experiment to fail the test case.

Unagi --> GalaxyTab GT-P6800
** Plug-in power for GalaxyTab **

After we send out a file from Unagi to GalaxyTab, GalaxyTab will turn on the screen without any message about the transferring request. Unlock the lock screen for GalaxyTab, there is no any prompt or status icon for out-coming transferring request.

But we unplug the power from GalaxyTab, it will show a prompt and status icon for out-coming transferring request. We can following these message to receive transferring request successfully. I'm not sure that it's a SS' issue.

I also use Nexus 7 and Nexus S to try the reproduces steps.(Nexus 7 --> GalaxyTab, Nexus S --> GalaxyTab) They work normally...(so sad~~)
We should keep to track the strange issue.

Comment 15

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

(In reply to Ian Liu [:ianliu] from comment #14)
> Leave me experiment..
> I find out a strange experiment to fail the test case.
> 
> Unagi --> GalaxyTab GT-P6800
> ** Plug-in power for GalaxyTab **
> 
> After we send out a file from Unagi to GalaxyTab, GalaxyTab will turn on the
> screen without any message about the transferring request. Unlock the lock
> screen for GalaxyTab, there is no any prompt or status icon for out-coming
> transferring request.
> 
> But we unplug the power from GalaxyTab, it will show a prompt and status
> icon for out-coming transferring request. We can following these message to
> receive transferring request successfully. I'm not sure that it's a SS'
> issue.
> 
> I also use Nexus 7 and Nexus S to try the reproduces steps.(Nexus 7 -->
> GalaxyTab, Nexus S --> GalaxyTab) They work normally...(so sad~~)
> We should keep to track the strange issue.

Just investigated more about this. When we apply Ian's patch, the class that target blob belongs to would change to class nsDOMMultipartFile, which differs from the original one, nsDOMFileFile. So, the scenario is:

1. Cheery-pick Ian's patch to current Gaia/v1-train
2. In BluetoothOppManager.cpp, we try to get an nsIInputStream instance from GetInternalStream().
3. nsDOMMultipartFile::GetInternalStream() would be called. In the for-loop of function GetInternalStream(), a blob instance is retrieved from mBlobs and its GetInternalStream() would then be called.
4. The GetInternalStream() of class RemoteBlob in dom/ipc/Blob.cpp would be called. The beginning of the function looks like:

    ActorType* actor = RemoteBlobBase<ActorFlavor>::Actor();
    if (!actor) {
      return NS_ERROR_UNEXPECTED;
    }

Unfortunately, the actor has no value so NS_ERROR_UNEXPECTED is returned. That's why the operation would fail.

Comment 16

[[e-mail], [e-mail]]

Just verify the issue is fixed or not without the workaround. The WIP patch is updated.

Build version: Gaia::master, Gecko::mozilla-central, ID: 20130826190748

Noted the log as following:
========================================================================================
E/GeckoConsole( 1821): Content JS LOG at app://bluetooth.gaiamobile.org/js/transfer.js:280 in bt_connSuccess/<: filepath = /sdcard/DCIM/100MZLLA/IMG_0001.jpg
E/GeckoConsole( 1821): Content JS LOG at app://bluetooth.gaiamobile.org/js/transfer.js:294 in bt_connSuccess/<: blob.name = /sdcard/DCIM/100MZLLA/IMG_0001.jpg
E/GeckoConsole( 1821): Content JS LOG at app://bluetooth.gaiamobile.org/js/transfer.js:295 in bt_connSuccess/<: blob.size = 335330
E/GeckoConsole( 1821): Content JS LOG at app://bluetooth.gaiamobile.org/js/transfer.js:296 in bt_connSuccess/<: blob = [object File]
I/nsVolumeMountLock( 1678): nsVolumeMountLock created for 'sdcard'
I/nsVolume( 1678): nsVolume: sdcard state Mounted @ '/mnt/sdcard' gen 1 locked 1 fake 0 media 1 sharing 0
I/AutoMounter( 1678): UpdateState: umsAvail:1 umsEnabled:1 mode:0 usbCablePluggedIn:1 tryToShare:0
I/AutoMounter( 1678): UpdateState: Volume sdcard is Mounted and inserted @ /mnt/sdcard gen 1 locked 1 sharing y
I/AutoMounter( 1678): UpdateState: umsAvail:1 umsEnabled:1 mode:0 usbCablePluggedIn:1 tryToShare:0
I/AutoMounter( 1678): UpdateState: Volume sdcard is Mounted and inserted @ /mnt/sdcard gen 1 locked 1 sharing y
I/nsVolumeMountLock( 1678): nsVolumeMountLock acquired for 'sdcard' gen 1
E/GeckoConsole( 1821): Content JS LOG at app://bluetooth.gaiamobile.org/js/transfer.js:284 in bt_connSuccess/</getRequest.onsuccess: result = [object File]
========================================================================================

There is no any error in Gaia side. It will need Gecko Bluetooth for investigating the issue. And it might be other issue about passing blob via activity.
https://bugzilla.mozilla.org/show_bug.cgi?id=896176

Comment 17

[Joe Cheng [:jcheng] (please needinfo)]

koi+ as it blocks koi+ bug

Comment 18

[[e-mail], [e-mail]]

Attachment: Pointer to Github pull request 11983.html

Since bug 908432 is landed, passing blob via bluetooth is working for me.
My test case:
* photo, music, video
* single/multiple file(s).

Dominic,

Could you please help to review the patch? Thanks.

Comment 19

[Dominic Kuo [:dkuo]]

Comment on attachment 800633 [details]
Pointer to Github pull request 11983.html

Ian, this a trivial patch that simply remove the workaround, so r+ of course, and please also address the minor issue I commented on github before you land this, thanks.

Comment 20

[[e-mail], [e-mail]]

Dominic, I have added and concerned to the empty array issue. Thanks for your reviewing effort. 

Since the patch is updated and landed, we're able to close the issue now.

gaia/master: a3397464f8c73e650018e88cd0ae8e1b16a9ceb3