Closed Bug 1063066 Opened 10 years ago Closed 10 years ago

[Flame][KK] Bluetooth transfer does not work in Gallery App

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
major

Tracking

(blocking-b2g:2.1+, firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v1.4 unaffected, b2g-v2.0 unaffected, b2g-v2.1 verified, b2g-v2.2 verified)

VERIFIED FIXED
2.1 S5 (26sep)
blocking-b2g 2.1+
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
b2g-v1.4 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: njpark, Assigned: ben.tian)

References

Details

(Keywords: regression, smoketest)

Attachments

(3 files, 8 obsolete files)

105.37 KB, text/plain
Details
34.19 KB, patch
Details | Diff | Splinter Review
15.77 KB, patch
Details | Diff | Splinter Review
Attached file bluetooth.txt
Version Info:
Gaia      a2393917a358a616950d0b02280e03c5c10795b9
Gecko (git commit) 54b65f9f37946f3eeffea00652fc04ff634eecaf
BuildID   20140904101243
Version   34.0a2
ro.build.date   Thu Aug 28 15:16:38 CST 2014
ro.bootloader   L1TC00011660

STR:
Pair bluetooth with Mac.
Go to gallery app, select a picture, and select transfer by bluetooth, and select the Mac

Actual:
The notification says transfer is initiated, but the computer does not seem to receive the file.  It worked properly on the first and second time, but on 3rd time this bug appeared, and cannot transfer files ever since.

Logcat attached
[Blocking Requested - why for this release]:
File transfer functionality broken

This is with 319MB KK build (v166)
blocking-b2g: --- → 2.1?
Howie, looks like this bug is in peripherals team's plate. Can you help to triage this bug? Thanks.
Flags: needinfo?(hochang)
qawanted to check if it's reproducible, thanks.
Flags: needinfo?(hochang)
Keywords: qawanted
Our team does not have the resources to test this bug on a Mac device.  

However, these are my results transferring files to and from a Linux computer. 


Unable to transfer a Gallery image from a v165 Flame device to Linux computer via bluetooth. 
-Files can be received from computer. 
(319 MB memory)

Device: Flame 2.2
Build ID: 20140904183303
Gaia: 5765c62163bcb7fde5ebfd211881117de31a7c46
Gecko: db7212847c14
Version: 35.0a1 (Master)
Firmware Version: v165 
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0
---------------------------------------------------------------------------------
Transferring Gallery images to and from v123 Flame device to a Linux computer via bluetooth is successful.  
(319 MB memory)

Device: Flame 2.2
BuildID: 20140910060915
Gaia: f108c706fae43cd61628babdd9463e7695b2496e
Gecko: 843332cc69af
Version: 35.0a1 (Master) 
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0


Leaving 'qawanted' tag for other testers to review.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Based on the repro w/ Linex computer this does not seem Mac specific


QA-Wanted for a full KK branch check
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
QA Contact: ddixon
All checks performed with a Flame KK base (512 MB memory) and a Linux computer. 

Issue DOES occur in Flame 2.2 build

Device: Flame 2.2
Build ID: 20140904183303
Gaia: 5765c62163bcb7fde5ebfd211881117de31a7c46
Gecko: db7212847c14
Version: 35.0a1 (Master)
Firmware Version: v165 
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0
-----------------------------------------------------------
-----------------------------------------------------------
Flame 2.1 repro is blocked by another issue (Gallery does not open correctly)

(Blocked) 

Device: Flame 2.1
BuildID: 20140908172602
Gaia: c7b55ed3be126b1f5f3d9cdd0277c365a6652d29
Gecko: 956b152e6d21
Version: 34.0a2 (Master) 
Firmware Version: 
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
-----------------------------------------------------------
-----------------------------------------------------------
Issue DOES NOT occur in Flame 2.0, Flame 1.4 KK base image. 

Device: Flame 2.0
BuildID: 20140908131505
Gaia: 59a670d40ad7f5966ec7fafcab0f05009bea97ab
Gecko: de70f9a40834
Version: 32.0 (2.0) 
Firmware Version: 
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0
-----------------------------------------------------------
Device: Flame 1.4 (KK base)
Build ID: 20140814202332
Gaia: 129211661489feb60bbd6772a44081d23b374f17
Gecko: 1483bd406aad0b3b9e3bdb75c5238b787b5a0cd6
Version: 30.0 (1.4)
Firmware Version: 
User Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Keywords: qawanted
QA Whiteboard: [QAnalyst-Triage?]
status-b2g-v2.1: --- → ?
Flags: needinfo?(jmitchell)
Triage: 2.1 blocking decision to be made after Comment 6 Gallery issue is fixed. Hi Duane, do we have the bug number for the Gallery issue? Thanks.
Flags: needinfo?(ddixon)
(In reply to howie [:howie] from comment #7)
> Triage: 2.1 blocking decision to be made after Comment 6 Gallery issue is
> fixed. Hi Duane, do we have the bug number for the Gallery issue? Thanks.

After re-testing the 'blocked' 2.1 build listed in Comment 6, the issue DOES NOT occur.  

The bug I was seeing is similar to Bug 1044114, but I could not get it to happen today.  

I also tested for the blocking issue on the latest 2.1 build (KK base image): 

Device: Flame 2.1
BuildID: 20140910165554
Gaia: d61264cd0c1f797b6be11e33524d8d52983c87e4
Gecko: 1d44dfce2e5b
Version: 34.0a2 (Master) 
Firmware Version: v165
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Overall, the device would have difficulty connecting to my Linux computer via bluetooth.  However, once the connection was made, sending files from the Gallery App was successful.  

Now I am finding the Regression Window here...
Flags: needinfo?(ddixon)
Unable to provide Regression Window. 

Bug occurs in the earliest Flame KK build we have access to. 

Actual Results: Gallery files cannot be sent over Bluetooth because the connection cannot be established. 

Environmental Variables:
Device: Flame Master
BuildID: 20140904171737
Gaia: de59e0c3614dd0061881fe284e9f2d74fa0d1d5d
Gecko: 8703c1895505
Version: 35.0a1 (Master) 
Firmware Version: 
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
We need to see if we can Repro in JB and then we can do a regression window there because we can go back further.


swapping prior nom to 2.2 because 2.1 is determined to be unaffected
blocking-b2g: 2.1? → 2.2?
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
There was some confusion about what this issue actually is.  Duane was seeing an issue where he could not pair devices instead of the issue where a gallery transfers starts but is not received by the target device.  

As this issue only occurs on KK Flame and occurs on the earliest available builds we have, a regression window for this issue cannot be found.

This issue occurs on 2.2 KK Flame, the earliest Central KK Flame available (20140904171737) and 2.1 KK Flame.

Environmental Variables:
Device: Flame 2.2
BuildID: 20140915053010
Gaia: 855be6ade407c26e0596e7306a44deebc3f60933
Gecko: 56cba2986c61
Version: 35.0a1 (2.2) 
Firmware Version: v165
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Environmental Variables:
Device: Flame 2.2
BuildID: 20140904171737
Gaia: de59e0c3614dd0061881fe284e9f2d74fa0d1d5d
Gecko: 8703c1895505
Version: 35.0a1 (2.2) 
Firmware Version: 
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Environmental Variables:
Device: Flame 2.1
BuildID: 20140915073605
Gaia: 944e5b4582c4efa1b67cd33245dbb8f6aa25d09f
Gecko: 2697f51cfe95
Version: 34.0a2 (2.1) 
Firmware Version: v165
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

This issue does not occur on 2.0 KK Flame, 2.2 Flame on the v123 JB base, or the 1.4 Flame KK v165 base image.

Environmental Variables:
Device: Flame 2.0
BuildID: 20140914162307
Gaia: 7edd3b0b9f65c3dde235c732d270e43e055a1254
Gecko: 13e04ab68621
Version: 32.0 (2.0) 
Firmware Version: 
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Environmental Variables:
Device: Flame 2.2
BuildID: 20140915053010
Gaia: 855be6ade407c26e0596e7306a44deebc3f60933
Gecko: 56cba2986c61
Version: 35.0a1 (2.2) 
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
[Blocking Requested - why for this release]: swapping from 2.2 to 2.1 because 2.1 IS affected
blocking-b2g: 2.2? → 2.1?
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][lead-review+]
Triage: regression, blocking.
blocking-b2g: 2.1? → 2.1+
QA has officially moved over to KK as our primary daily testing target, which now bumps this to a smoketest blocker.

Howie - Can you push on the BT team to get fix on this asap? I'd like to get a green KK smoketest as soon as possible.
Flags: needinfo?(hochang)
Keywords: smoketest
Hi Shawn, let's put this as the top priority now, thank you.
Flags: needinfo?(hochang) → needinfo?(shuang)
Attached patch aurora-opp-ugly-fix.patch (obsolete) — Splinter Review
Update:

When bluetooth connects to socket for file transferring, it gets the fd still used by prior listening socket. The reason is |BluetoothSocket::CloseSocket| of listening socket isn't propagated to AcceptWatcher so the socket message watcher is still waiting for socket messages on the fd. And then |BluetoothSocket::ConnectSocket| sometimes gets the same fd and then block for
receiving nothing from fd.

09-22 03:20:25.690   208   208 I GeckoBluetooth: Accept: 
09-22 03:20:25.690   208   560 I GeckoBluetooth: Watch: fd 142
09-22 03:20:25.690   208   560 I GeckoBluetooth: OnFileCanReadWithoutBlocking: fd 142
09-22 03:20:38.500   208   208 I GeckoBluetooth: CloseSocket: 
09-22 03:20:38.500   208   208 I GeckoBluetooth: Connect: 
09-22 03:20:38.500   208   560 I GeckoBluetooth: Watch: fd 142
--

Attach an ugly fix that aborts socket message watcher when socket is closed. Also ni? Thomas for better suggestion to fix.

09-22 04:00:02.640   206   206 I GeckoBluetooth: Accept: 
09-22 04:00:02.640   206   549 I GeckoBluetooth: Watch: fd 154
09-22 04:00:02.640   206   549 I GeckoBluetooth: OnFileCanReadWithoutBlocking: fd 154
09-22 04:00:18.200   206   206 I GeckoBluetooth: CloseSocket: 
09-22 04:00:18.200   206   206 I GeckoBluetooth: Abort: 
09-22 04:00:18.200   206   206 I GeckoBluetooth: Abort: stop socket message watcher
09-22 04:00:18.200   206   549 I GeckoBluetooth: StopWatch: 
09-22 04:00:18.200   206   549 I GeckoBluetooth: OnFileCanReadWithoutBlocking: fd -1
09-22 04:00:18.200   206   549 I GeckoBluetooth: OnFileCanReadWithoutBlocking: stop watcher
09-22 04:00:18.200   206   549 I GeckoBluetooth: Proceed: 
09-22 04:00:18.200   206   206 I GeckoBluetooth: Connect: 
09-22 04:00:18.200   206   549 I GeckoBluetooth: Watch: fd 154
09-22 04:00:18.200   206   206 I GeckoBluetooth: OnError: BluetoothSocketInterface::Accept failed: 1
09-22 04:00:19.340   206   549 I GeckoBluetooth: OnFileCanReadWithoutBlocking: fd 154
09-22 04:00:19.490   206   549 I GeckoBluetooth: OnFileCanReadWithoutBlocking: fd 154
09-22 04:00:19.490   206   549 I GeckoBluetooth: OnFileCanReadWithoutBlocking: stop watcher
09-22 04:00:19.510   206   206 I GeckoBluetooth: OnSocketConnectSuccess: [client]
Flags: needinfo?(tzimmermann)
Attachment #8492920 - Attachment description: aurora-opp-ugly-fix.patch → [aurora-opp-ugly-fix] Stop socket message watcher when listening socket is closed.
Attachment #8492920 - Attachment description: [aurora-opp-ugly-fix] Stop socket message watcher when listening socket is closed. → ugly-fix (v2.1): Stop socket message watcher when listening socket is closed.
Comment on attachment 8492920 [details] [diff] [review]
aurora-opp-ugly-fix.patch

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

Hi Ben,

Thanks for figuring out this problem. I added some comments on how to fix the problem correctly. The idea is to maintain a hash table that maps |BluetoothSocketResultHandler|s to watchers. When a pending connect process needs to be aborted, the respective result handler can be used for looking up the watcher and removing it from the I/O loop. I left some comments to explain how to do that. Let me know if you need more help. Please note that |Connect| is probably also affected.

BTW, you're using an outdated branch. BluetoothInterface.{cpp,h} shouldn't exist in bluedroid/ any longer.

::: dom/bluetooth/bluedroid/BluetoothInterface.cpp
@@ +1822,5 @@
>    void OnFileCanReadWithoutBlocking(int aFd) MOZ_OVERRIDE
>    {
>      BluetoothStatus status;
>  
> +    BT_LOGR("fd %d", aFd);

Please remove all these log statements from the final patch.

@@ +1825,5 @@
>  
> +    BT_LOGR("fd %d", aFd);
> +    if (aFd < 0) {
> +      status = STATUS_FAIL;
> +    } else {

Please remove this branch and see below for how to cleanup correctly.

@@ +1861,5 @@
>        &mWatcher,
>        this);
>    }
>  
> +  void StopWatch()

'StopWatching'

@@ +1864,5 @@
>  
> +  void StopWatch()
> +  {
> +    BT_LOGR();
> +    OnFileCanReadWithoutBlocking(-1);

Call |mWatcher.StopWatchingFileDescriptor()| instead. This will remove the watcher from the I/O loop.

@@ +2128,2 @@
>    : SocketMessageWatcher(aFd)
> +  , mInterface(aInterface)

Not required any longer.

@@ +2164,5 @@
> +  Task* t = new SocketMessageWatcherTask(mWatcher);
> +  XRE_GetIOMessageLoop()->PostTask(FROM_HERE, t);
> +}
> +
> +class StopSocketMessageWatcherTask MOZ_FINAL : public Task

Maybe |DeleteSocketMessageWatcherTask| instead.

@@ +2175,5 @@
> +  }
> +
> +  void Run() MOZ_OVERRIDE
> +  {
> +    mWatcher->StopWatch();

Your class member should be the result handler. Lookup the watcher from the hash table here on the I/O thread. Call |watcher->StopWatching|, then delete |watcher|.

When you create a new SocketMessagWatcher in |Accept| or |Connect| you need to insert it's address into the hash table. This is best done on |SocketMessageWatcher::Watch| after starting to watch the file descriptor. For successful connections, it's necessary to remove  the watcher from the hashtable. That's best done before calling |Proceed| in |SocketMessageWatcher::OnFileCanReadWithoutBlocking|.

@@ +2184,5 @@
> +};
> +
> +
> +void
> +BluetoothSocketInterface::Abort()

Replace this method by |Close(BluetoothSocketResultHandler* aRes)|, which I outlined elsewhere. |Close| should create the task for stopping the watcher and post it to the I/O thread.

@@ +2197,5 @@
>  
>  BluetoothSocketInterface::BluetoothSocketInterface(
>    const btsock_interface_t* aInterface)
>  : mInterface(aInterface)
> +, mWatcher(nullptr)

We cannot store the watcher here because there could be multiple accept operations at the same time (at least in principle). There should instead be a hash table for looking up the watcher. The result handler would be the table's key.

::: dom/bluetooth/bluedroid/BluetoothInterface.h
@@ +66,5 @@
>  
>    void Accept(int aFd, BluetoothSocketResultHandler* aRes);
>  
> +  void Abort();
> +  void ResetWatcher() { mWatcher = nullptr; }

|Abort| and |ResetWatcher| should go away. Instead, I'd like to have a method |Close(BluetoothSocketResultHandler* aRes)|. This gets called by |BluetoothSocket| on the main thread, and removes the watcher (if it exists) on the I/O thread.

::: dom/bluetooth/bluedroid/BluetoothSocket.cpp
@@ +592,5 @@
>    if (!mImpl) {
>      return;
>    }
>  
> +  sBluetoothSocketInterface->Abort();

You need to store a pointer to the last result handler that was executed on a BluetoothSocket. There should only be one at a time, so a simple variable in |BluetoothSocket| should do. At this point, call |sBluetoothSocketInterface::Close| with that result handler. The backend will be able to remove the corresponding watcher from the I/O thread.
Attachment #8492920 - Attachment description: ugly-fix (v2.1): Stop socket message watcher when listening socket is closed. → aurora-opp-ugly-fix.patch
Attachment #8492920 - Flags: feedback-
Flags: needinfo?(tzimmermann)
Attached patch WIP patch on v2.1 (v1) (obsolete) — Splinter Review
The patch uses hash table but meets following crash during stop AcceptWatcher. The crash doesn't happen without hashtable.remove(). Still under investigation.

--
Program received signal SIGBUS, Bus error.
[Switching to Thread 207.539]
0xb4e3b1c4 in add (aVal=1, aPtr=@0x5a5a5a5e: <error reading variable>) at ../../dist/include/mozilla/Atomics.h:980
980	  T operator++() { return Base::Intrinsics::inc(Base::mValue) + 1; }
(gdb) bt
#0  0xb4e3b1c4 in add (aVal=1, aPtr=@0x5a5a5a5e: <error reading variable>) at ../../dist/include/mozilla/Atomics.h:980
#1  inc (aPtr=@0x5a5a5a5e: <error reading variable>) at ../../dist/include/mozilla/Atomics.h:487
#2  mozilla::detail::AtomicBaseIncDec<unsigned int, (mozilla::MemoryOrdering)2>::operator++ (this=0x5a5a5a5e) at ../../dist/include/mozilla/Atomics.h:980
#3  0xb510bd1e in operator++ (this=<optimized out>) at ../../../../dist/include/nsISupportsImpl.h:356
#4  AddRef (this=<optimized out>) at ../../../../../../gecko/v2.1/media/webrtc/signaling/include/CC_Device.h:20
#5  nsRefPtr<CSF::CC_Device>::nsRefPtr (this=0xac9ffa28, aRawPtr=<optimized out>) at ../../../../../../gecko/v2.1/xpcom/base/nsAutoPtr.h:885
#6  0xb564dc34 in BluetoothInterfaceRunnable1 (aArg1=<synthetic pointer>, aMethod=&virtual mozilla::dom::bluetooth::BluetoothSocketResultHandler::OnError(mozilla::dom::bluetooth::BluetoothStatus), aObj=0x5a5a5a5a, this=0xac9ffa20)
    at ../../../../gecko/v2.1/dom/bluetooth/bluedroid/BluetoothInterface.cpp:1125
#7  mozilla::dom::bluetooth::DispatchBluetoothSocketResult (aRes=aRes@entry=0x5a5a5a5a, aMethod=&virtual mozilla::dom::bluetooth::BluetoothSocketResultHandler::Accept(int, nsAString_internal const&, int), 
    aArg1=aArg1@entry=1515870810, aArg2=..., aArg3=1515870810, aStatus=aStatus@entry=mozilla::dom::bluetooth::STATUS_DONE) at ../../../../gecko/v2.1/dom/bluetooth/bluedroid/BluetoothInterface.cpp:1746
#8  0xb564dda0 in mozilla::dom::bluetooth::AcceptWatcher::Proceed (this=0xaf5daa90, aStatus=mozilla::dom::bluetooth::STATUS_DONE) at ../../../../gecko/v2.1/dom/bluetooth/bluedroid/BluetoothInterface.cpp:2141
#9  0xb564bf08 in StopWatching (this=0xaf5daa90) at ../../../../gecko/v2.1/dom/bluetooth/bluedroid/BluetoothInterface.cpp:1864
#10 mozilla::dom::bluetooth::DeleteSocketMessageWatcherTask::Run (this=<optimized out>) at ../../../../gecko/v2.1/dom/bluetooth/bluedroid/BluetoothInterface.cpp:2182
#11 0xb4fb7998 in MessageLoop::RunTask (this=0xb32ffcb0, task=0xac6451a0) at ../../../../gecko/v2.1/ipc/chromium/src/base/message_loop.cc:362
#12 0xb4fb8076 in MessageLoop::DeferOrRunPendingTask (this=<optimized out>, pending_task=...) at ../../../../gecko/v2.1/ipc/chromium/src/base/message_loop.cc:370
#13 0xb4fb90a8 in DoWork (this=<optimized out>) at ../../../../gecko/v2.1/ipc/chromium/src/base/message_loop.cc:448
#14 MessageLoop::DoWork (this=0xb32ffcb0) at ../../../../gecko/v2.1/ipc/chromium/src/base/message_loop.cc:427
#15 0xb4fb025a in base::MessagePumpLibevent::Run (this=0xb6acd040, delegate=0xb32ffcb0) at ../../../../gecko/v2.1/ipc/chromium/src/base/message_pump_libevent.cc:311
#16 0xb4fb7926 in MessageLoop::RunInternal (this=this@entry=0xb32ffcb0) at ../../../../gecko/v2.1/ipc/chromium/src/base/message_loop.cc:234
#17 0xb4fb79d8 in RunHandler (this=0xb32ffcb0) at ../../../../gecko/v2.1/ipc/chromium/src/base/message_loop.cc:227
#18 MessageLoop::Run (this=this@entry=0xb32ffcb0) at ../../../../gecko/v2.1/ipc/chromium/src/base/message_loop.cc:201
#19 0xb4fba534 in base::Thread::ThreadMain (this=0xb6a34580) at ../../../../gecko/v2.1/ipc/chromium/src/base/thread.cc:168
#20 0xb4fb070c in ThreadFunc (closure=<optimized out>) at ../../../../gecko/v2.1/ipc/chromium/src/base/platform_thread_posix.cc:39
#21 0xb6ecf22c in __thread_entry (func=0xb4fb0705 <ThreadFunc(void*)>, arg=0xb6a34580, tls=0xb32ffdd0) at bionic/libc/bionic/pthread_create.cpp:105
#22 0xb6ecf3c4 in pthread_create (thread_out=0xb6a34588, attr=<optimized out>, start_routine=0xb4fb0705 <ThreadFunc(void*)>, arg=0x78) at bionic/libc/bionic/pthread_create.cpp:224
#23 0x00000000 in ?? ()
Assignee: nobody → btian
Comment on attachment 8493574 [details] [diff] [review]
WIP patch on v2.1 (v1)

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

Hi Ben,

See below for some comments on the current WIP patch.

::: dom/bluetooth/bluedroid/BluetoothInterface.cpp
@@ +1821,5 @@
>    virtual void Proceed(BluetoothStatus aStatus) = 0;
>  
> +  virtual void Watch()
> +  {
> +    BT_LOGR("fd %d", mFd);

I suggest to put the result handler into the hash table right here before you start watching. The method |AcceptWatcher::Watch| can then be removed. Having the hash-table setup here will also make sure that |BluetoothSocketInterface::Connect| works correctly.

@@ +1852,1 @@
>        mWatcher.StopWatchingFileDescriptor();

Replace this line by a call to the cleaned up implementation of |StopWatching|. This will cleanup the result handler from the hash table.

@@ +1861,3 @@
>    {
> +    BT_LOGR("fd %d", mFd);
> +    mWatcher.StopWatchingFileDescriptor();

Remove the result handler right before this line.

@@ +1861,4 @@
>    {
> +    BT_LOGR("fd %d", mFd);
> +    mWatcher.StopWatchingFileDescriptor();
> +    Proceed(STATUS_DONE);

I'm not sure if |Proceed| is required for closed connections, but please move the call to |Proceed| out of |StopWatching| and into the calling method.
Hi Thomas,

Thanks for you suggestion but still I'm confused about what to do exactly. Please see my questions below.

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #20)
> ::: dom/bluetooth/bluedroid/BluetoothInterface.cpp
> @@ +1821,5 @@
> >    virtual void Proceed(BluetoothStatus aStatus) = 0;
> >  
> > +  virtual void Watch()
> > +  {
> > +    BT_LOGR("fd %d", mFd);
> 
> I suggest to put the result handler into the hash table right here before
> you start watching. The method |AcceptWatcher::Watch| can then be removed.
> Having the hash-table setup here will also make sure that
> |BluetoothSocketInterface::Connect| works correctly.

Do you suggest to put the hash table inside |SocketMessageWatcher| as a static variable? So the |BluetoothSocketResultHandler| has to be passed into |SocketMessageWatcher| and stored as a member variable, right?
 
> @@ +1852,1 @@
> >        mWatcher.StopWatchingFileDescriptor();
> 
> Replace this line by a call to the cleaned up implementation of
> |StopWatching|. This will cleanup the result handler from the hash table.

Why not trigger |Proceed| to reply socket result? Shouldn't the result handler of closed connection be replied?

> @@ +1861,3 @@
> >    {
> > +    BT_LOGR("fd %d", mFd);
> > +    mWatcher.StopWatchingFileDescriptor();
> 
> Remove the result handler right before this line.

You mean to remove from hash table, right?

> @@ +1861,4 @@
> >    {
> > +    BT_LOGR("fd %d", mFd);
> > +    mWatcher.StopWatchingFileDescriptor();
> > +    Proceed(STATUS_DONE);
> 
> I'm not sure if |Proceed| is required for closed connections, but please
> move the call to |Proceed| out of |StopWatching| and into the calling method.

Why move the call to |Proceed| out of |StopWatching|?
Flags: needinfo?(tzimmermann)
(In reply to Ben Tian [:btian] from comment #19)
> The patch uses hash table but meets following crash during stop
> AcceptWatcher. The crash doesn't happen without hashtable.remove(). Still
> under investigation.

hashtable.Remove() releases both key and value objects so access to key/value object after hash entry removal results in crash. Trying to find a suitable timing to call Remove().
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #20)
> > ::: dom/bluetooth/bluedroid/BluetoothInterface.cpp
> > @@ +1821,5 @@
> > >    virtual void Proceed(BluetoothStatus aStatus) = 0;
> > >  
> > > +  virtual void Watch()
> > > +  {
> > > +    BT_LOGR("fd %d", mFd);
> > 
> > I suggest to put the result handler into the hash table right here before
> > you start watching. The method |AcceptWatcher::Watch| can then be removed.
> > Having the hash-table setup here will also make sure that
> > |BluetoothSocketInterface::Connect| works correctly.
> 
> Do you suggest to put the hash table inside |SocketMessageWatcher| as a
> static variable? So the |BluetoothSocketResultHandler| has to be passed into
> |SocketMessageWatcher| and stored as a member variable, right?

Right, the result handler has to be available in |SocketMessageWatcher::Watch| somehow. There is the member field |mRes| in |AcceptWatcher| and |ConnectWatcher|. Moving this into |SocketMessageWatcher| should do the job AFAICS.

The actual hash table has to be a global variable as it is now, I think. If you can make it static within |SocketMessageWatcher| that would be even better.

>  
> > @@ +1852,1 @@
> > >        mWatcher.StopWatchingFileDescriptor();
> > 
> > Replace this line by a call to the cleaned up implementation of
> > |StopWatching|. This will cleanup the result handler from the hash table.
> 
> Why not trigger |Proceed| to reply socket result? Shouldn't the result
> handler of closed connection be replied?

I mean to replace |mWatcher.StopWatchingFileDescriptor()| by the call to |StopWatching|. This will remove the watcher from the I/O thread and remove the result handler from the hash table. You still have to call |Proceed| in to line below.

> 
> > @@ +1861,3 @@
> > >    {
> > > +    BT_LOGR("fd %d", mFd);
> > > +    mWatcher.StopWatchingFileDescriptor();
> > 
> > Remove the result handler right before this line.
> 
> You mean to remove from hash table, right?

Right.

> > @@ +1861,4 @@
> > >    {
> > > +    BT_LOGR("fd %d", mFd);
> > > +    mWatcher.StopWatchingFileDescriptor();
> > > +    Proceed(STATUS_DONE);
> > 
> > I'm not sure if |Proceed| is required for closed connections, but please
> > move the call to |Proceed| out of |StopWatching| and into the calling method.
> 
> Why move the call to |Proceed| out of |StopWatching|?

1) Cleanliness: 'StopWatching' should only stop watching. ;) And, as the name suggests, it should be the inverse operation to |Watch|. Although the methods |Watch| and |StopWatching| also maintain the content of the hash table, but that more an internal property and well guarded from the rest of the code.

2) The other reason is that |Proceed| receives a status code, which is currently hard-coded in |StopWatching|. Instead of passing the status as argument to |StopWatching|, I think it's better to move |Proceed| out for reasons given in 1).
Flags: needinfo?(tzimmermann)
(In reply to Ben Tian [:btian] from comment #22)
> (In reply to Ben Tian [:btian] from comment #19)
> > The patch uses hash table but meets following crash during stop
> > AcceptWatcher. The crash doesn't happen without hashtable.remove(). Still
> > under investigation.
> 
> hashtable.Remove() releases both key and value objects so access to
> key/value object after hash entry removal results in crash. Trying to find a
> suitable timing to call Remove().

The result handler is ref-counted. Does that help?
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #24)
> > hashtable.Remove() releases both key and value objects so access to
> > key/value object after hash entry removal results in crash. Trying to find a
> > suitable timing to call Remove().
> 
> The result handler is ref-counted. Does that help?

It is but its refptr is stored in non-ref-counted |SocketMessageWatcher| (the value object). I think this might be the problem, no?
Attached patch WIP patch on v2.1 (v2) (obsolete) — Splinter Review
Revise based on Thomas' suggestion. Still meets crash and is investigating.
Attachment #8493574 - Attachment is obsolete: true
> It is but its refptr is stored in non-ref-counted |SocketMessageWatcher|
> (the value object). I think this might be the problem, no?

Ref-counting doesn't depend on the container class.

Just a guess: Maybe the watcher get's deleted when you remove it from the hash table. You could work around that by storing the pointer to the watcher in a wrapper class, which is then inserted into the table. If you don't delete the watcher in the wrapper's destructor, you should be fine.
The patch maintains an nsRefPtr array to keep stopped watchers from be released by hash table, and then remove them from the array to release in |Proceed|.
Attachment #8492920 - Attachment is obsolete: true
Attachment #8494201 - Attachment is obsolete: true
Attachment #8494352 - Flags: review?(tzimmermann)
Hi Ben

(In reply to Ben Tian [:btian] from comment #28)
> Created attachment 8494352 [details] [diff] [review]
> [2.1] Patch 1 (v1): Stop watchers when socket is closed
> 
> The patch maintains an nsRefPtr array to keep stopped watchers from be
> released by hash table, and then remove them from the array to release in
> |Proceed|.

I don't like the approach of having another hash table and ref-counting in |SocketMessageWatcher| because of the overhead. Did you try to wrap the watcher in a trivial container while it's in the original hash table? Did that work?
Flags: needinfo?(btian)
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #29)
> I don't like the approach of having another hash table and ref-counting in
> |SocketMessageWatcher| because of the overhead. Did you try to wrap the
> watcher in a trivial container while it's in the original hash table? Did
> that work?

Not yet. The patch was done before I saw your comment 27. I'll upload another patch based on your suggestion. Thanks.
Flags: needinfo?(btian)
This patch adopts SocketMessageWatcherWrapper based on Thomas' suggestion.
Attachment #8494352 - Attachment is obsolete: true
Attachment #8494352 - Flags: review?(tzimmermann)
Attachment #8494383 - Flags: review?(tzimmermann)
Comment on attachment 8494383 [details] [diff] [review]
[2.1] Patch 1 (v2): Stop watchers when socket is closed

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

Hi Ben,

Thanks for the patch. The comments are mostly about potential cleanups. I'm still confused by your revision. I don't think this is the latest master branch. :/

::: dom/bluetooth/bluedroid/BluetoothInterface.cpp
@@ +1807,5 @@
> +
> +/* |sWatcherHashTable| maps result handlers to corresponding watchers */
> +static nsClassHashtable<nsRefPtrHashKey<BluetoothSocketResultHandler>,
> +                        SocketMessageWatcherWrapper>
> +  sWatcherHashtable;

Maybe call this variable 'sActiveWatchers'.

@@ +1835,5 @@
>    static const unsigned char OFF_BDADDRESS = 6;
>    static const unsigned char OFF_CHANNEL2 = 12;
>    static const unsigned char OFF_STATUS = 16;
>  
> +  SocketMessageWatcher(int aFd, BluetoothSocketResultHandler* aRes)

I suggest to use the same order for file descriptor and resource handler as in the class's fields.

@@ +2144,5 @@
>  
> +/* |AcceptWatcher| specializes SocketMessageWatcher for Accept
> + * operations by reading the socket messages from Bluedroid and
> + * forwarding the received client socket to the resource handler.
> + * The first message is received immediately. When there's a new

What happened with this comment?

::: dom/bluetooth/bluedroid/BluetoothSocket.cpp
@@ +394,5 @@
>  
> +    BluetoothSocketResultHandler* res = new AcceptResultHandler(GetIO());
> +    sBluetoothSocketInterface->Accept(mFd, res);
> +
> +    GetIO()->mConsumer->SetLastResultHandler(res);

I suggest to put this line between the other two lines. It doesn't matter for correctness, but it seems to make the dependecies in the code more obvious.

@@ +475,5 @@
>                                   BluetoothSocketType aType,
>                                   bool aAuth,
>                                   bool aEncrypt)
>    : mObserver(aObserver)
> +  , mLastRes(nullptr)

You should also clear the field after the connection has been established or failed. The correct methods should be |OnConnectSuccess| and |OnConnectError|.

@@ +535,5 @@
>      BluetoothSocketType::RFCOMM,
>      UUID_OBEX_OBJECT_PUSH,
> +    aChannel, mEncrypt, mAuth, res);
> +
> +  SetLastResultHandler(res);

Again, I feel like this line should be between the other two lines above; just for making the code a bit more obvious.

@@ +585,5 @@
>      NS_LITERAL_STRING("OBEX Object Push"),
>      UUID_OBEX_OBJECT_PUSH,
> +    aChannel, mEncrypt, mAuth, res);
> +
> +  SetLastResultHandler(res);

Ok, that call's not strictly necessary, but I think it makes sense in the context.

@@ +599,5 @@
>      return;
>    }
>  
> +  // Stop watching |SocketMessageWatcher| if any
> +  if (sBluetoothSocketInterface && mLastRes) {

Please use MOZ_ASSERT on |sBluetoothSocketInterface|. I'd guess that something is wrong if that variable is not set here.

::: dom/bluetooth/bluedroid/BluetoothSocket.h
@@ +47,5 @@
>    {
>      mDeviceAddress = aDeviceAddress;
>    }
>  
> +  inline void SetLastResultHandler(BluetoothSocketResultHandler* aRes)

That should maybe called 'current' result handler, instead of 'last'.
Attachment #8494383 - Flags: review?(tzimmermann) → review+
QA Whiteboard: [QAnalyst-Triage+][lead-review+] → [QAnalyst-Triage+][lead-review+][COM=Bluetooth]
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #32)
> Hi Ben,
> 
> Thanks for the patch. The comments are mostly about potential cleanups. I'm
> still confused by your revision. I don't think this is the latest master
> branch. :/

Sorry I'm checking this bug on 2.1 branch since the beginning and will also upload a m-c patch for review.

Two more minor revisions as following. Let me know for any concern.
- add MOZ_ASSERT(mRes) in |SocketMessageWatcher| ctor to force non-null result handler as hash key.
- remove result handler nullity check in |Proceed| since |mRes| is non-null.

> @@ +1835,5 @@
> >    static const unsigned char OFF_BDADDRESS = 6;
> >    static const unsigned char OFF_CHANNEL2 = 12;
> >    static const unsigned char OFF_STATUS = 16;
> >  
> > +  SocketMessageWatcher(int aFd, BluetoothSocketResultHandler* aRes)
> 
> I suggest to use the same order for file descriptor and resource handler as
> in the class's fields.

I also re-order variable declaration order otherwise compiler warns for inconsistency.

> @@ +2144,5 @@
> >  
> > +/* |AcceptWatcher| specializes SocketMessageWatcher for Accept
> > + * operations by reading the socket messages from Bluedroid and
> > + * forwarding the received client socket to the resource handler.
> > + * The first message is received immediately. When there's a new
> 
> What happened with this comment?

Just added |AcceptWatcher| at the beginning to conform with |ConnectWatcher|'s comment.
 
> ::: dom/bluetooth/bluedroid/BluetoothSocket.h
> @@ +47,5 @@
> >    {
> >      mDeviceAddress = aDeviceAddress;
> >    }
> >  
> > +  inline void SetLastResultHandler(BluetoothSocketResultHandler* aRes)
> 
> That should maybe called 'current' result handler, instead of 'last'.

I also rename |mLastRes| to |mCurrentRes| for consistency.
Attachment #8494383 - Attachment is obsolete: true
Target Milestone: --- → 1.3 Sprint 1 - 9/27
Target Milestone: 1.3 Sprint 1 - 9/27 → 2.1 S5 (26sep)
Comment on attachment 8494938 [details] [diff] [review]
[2.1] Patch 1 (v3): Stop watchers when socket is closed, r=tzimmermann, a=2.1+

Approval Request Comment
[Feature/regressing bug #]: smoketest blocker
[User impact if declined]: see bug title
[Describe test coverage new/current, TBPL]:
[Risks and why]: No idea! Ben?
[String/UUID change made/needed]: none that I can see
Attachment #8494938 - Flags: review+
Attachment #8494938 - Flags: approval-mozilla-aurora?
m-c patch based on 2.1 patch.

Also fix build break of BluetootRilListener.cpp caused by bug 1063304 patch 3.c.
Attachment #8494996 - Flags: review?(tzimmermann)
Comment on attachment 8494996 [details] [diff] [review]
[m-c] Patch 1 (v1): Stop watchers when socket is closed, a=2.1+

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

::: dom/bluetooth/bluedroid/BluetoothSocket.cpp
@@ +597,5 @@
>  void
>  BluetoothSocket::CloseSocket()
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT();

|sBluetoothSocketInterface| is missing here.

@@ +647,5 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>    MOZ_ASSERT(mObserver);
> +
> +  mCurrentRes = nullptr;

I suggest to use |SetCurrentResultHandler| here and below.
Attachment #8494996 - Flags: review?(tzimmermann) → review+
Revise based on Thomas' suggestion.
Attachment #8494996 - Attachment is obsolete: true
Fix build break on emulator builds.
Attachment #8495083 - Attachment is obsolete: true
Changes:
- revise v2.1 patch based on comment 36
- fix build break on emulator builds
Attachment #8494938 - Attachment is obsolete: true
Attachment #8494938 - Flags: approval-mozilla-aurora?
Comment on attachment 8495184 [details] [diff] [review]
[final] Patch 1 (v2.1): Stop watchers when socket is closed, r=tzimmermann, r=fabrice, a=2.1+

Set flag per comment 34.

Approval Request Comment
[Feature/regressing bug #]:
  smoketest blocker
[User impact if declined]:
  see bug title
[Describe test coverage new/current, TBPL]:
  https://tbpl.mozilla.org/?tree=Try&rev=0497c6434e3a
[Risks and why]:
  None
[String/UUID change made/needed]:
  None
Attachment #8495184 - Flags: approval-mozilla-aurora?
Attachment #8495184 - Attachment description: [final] Patch 1 (v2.1): Stop watchers when socket is closed, r=tzimmermann, a=2.1+ → [final] Patch 1 (v2.1): Stop watchers when socket is closed, r=tzimmermann, r=fabrice, a=2.1+
Comment on attachment 8495184 [details] [diff] [review]
[final] Patch 1 (v2.1): Stop watchers when socket is closed, r=tzimmermann, r=fabrice, a=2.1+

># HG changeset patch
># User Ben Tian <btian@mozilla.com>
># Date 1411641428 -28800
># Node ID 173c1c45e11782b7957af955f99bb1a38afa21b7
># Parent  db975bd7dda63ee631f367dc4d6374344e51ab4b
>Bug 1063066 - [Flame][KK] Bluetooth transfer does not work in Gallery App (v2.1), r=tzimmermann, r=fabrice, a=2.1+
>
>diff --git a/dom/bluetooth/bluedroid/BluetoothInterface.cpp b/dom/bluetooth/bluedroid/BluetoothInterface.cpp
>--- a/dom/bluetooth/bluedroid/BluetoothInterface.cpp
>+++ b/dom/bluetooth/bluedroid/BluetoothInterface.cpp
>@@ -4,17 +4,17 @@
>  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>  * You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> #include <errno.h>
> #include <unistd.h>
> #include <sys/socket.h>
> #include "base/message_loop.h"
> #include "BluetoothInterface.h"
>-#include "nsAutoPtr.h"
>+#include "nsClassHashtable.h"
> #include "nsThreadUtils.h"
> #include "nsXULAppAPI.h"
> 
> #if MOZ_IS_GCC && MOZ_GCC_VERSION_AT_LEAST(4, 7, 0)
> /* use designated array initializers if supported */
> #define CONVERT(in_, out_) \
>   [in_] = out_
> #else
>@@ -1777,16 +1777,44 @@ BluetoothSocketInterface::Listen(Bluetoo
>                                   fd, ConvertDefault(status, STATUS_FAIL));
>   }
> }
> 
> #define CMSGHDR_CONTAINS_FD(_cmsghdr) \
>     ( ((_cmsghdr)->cmsg_level == SOL_SOCKET) && \
>       ((_cmsghdr)->cmsg_type == SCM_RIGHTS) )
> 
>+class SocketMessageWatcher;
>+
>+/* |SocketMessageWatcherWrapper| wraps SocketMessageWatcher to keep it from
>+ * being released by hash table's Remove() method.
>+ */
>+class SocketMessageWatcherWrapper
>+{
>+public:
>+  SocketMessageWatcherWrapper(SocketMessageWatcher* aSocketMessageWatcher)
>+  : mSocketMessageWatcher(aSocketMessageWatcher)
>+  {
>+    MOZ_ASSERT(mSocketMessageWatcher);
>+  }
>+
>+  SocketMessageWatcher* GetSocketMessageWatcher()
>+  {
>+    return mSocketMessageWatcher;
>+  }
>+
>+private:
>+  SocketMessageWatcher* mSocketMessageWatcher;
>+};
>+
>+/* |sActiveWatchers| maps result handlers to corresponding watchers */
>+static nsClassHashtable<nsRefPtrHashKey<BluetoothSocketResultHandler>,
>+                        SocketMessageWatcherWrapper>
>+  sActiveWatchers;
>+
> /* |SocketMessageWatcher| receives Bluedroid's socket setup
>  * messages on the I/O thread. You need to inherit from this
>  * class to make use of it.
>  *
>  * Bluedroid sends two socket info messages (20 bytes) at
>  * the beginning of a connection to both peers.
>  *
>  *   - 1st message: [channel:4]
>@@ -1803,21 +1831,24 @@ public:
>   static const unsigned char MSG2_SIZE = 16;
> 
>   static const unsigned char OFF_CHANNEL1 = 0;
>   static const unsigned char OFF_SIZE = 4;
>   static const unsigned char OFF_BDADDRESS = 6;
>   static const unsigned char OFF_CHANNEL2 = 12;
>   static const unsigned char OFF_STATUS = 16;
> 
>-  SocketMessageWatcher(int aFd)
>+  SocketMessageWatcher(int aFd, BluetoothSocketResultHandler* aRes)
>   : mFd(aFd)
>   , mClientFd(-1)
>   , mLen(0)
>-  { }
>+  , mRes(aRes)
>+  {
>+    MOZ_ASSERT(mRes);
>+  }
> 
>   virtual ~SocketMessageWatcher()
>   { }
> 
>   virtual void Proceed(BluetoothStatus aStatus) = 0;
> 
>   void OnFileCanReadWithoutBlocking(int aFd) MOZ_OVERRIDE
>   {
>@@ -1832,34 +1863,45 @@ public:
>         break;
>       default:
>         /* message-size error */
>         status = STATUS_FAIL;
>         break;
>     }
> 
>     if (IsComplete() || status != STATUS_SUCCESS) {
>-      mWatcher.StopWatchingFileDescriptor();
>+      StopWatching();
>       Proceed(status);
>     }
>   }
> 
>   void OnFileCanWriteWithoutBlocking(int aFd) MOZ_OVERRIDE
>   { }
> 
>   void Watch()
>   {
>+    // add this watcher and its result handler to hash table
>+    sActiveWatchers.Put(mRes, new SocketMessageWatcherWrapper(this));
>+
>     MessageLoopForIO::current()->WatchFileDescriptor(
>       mFd,
>       true,
>       MessageLoopForIO::WATCH_READ,
>       &mWatcher,
>       this);
>   }
> 
>+  void StopWatching()
>+  {
>+    mWatcher.StopWatchingFileDescriptor();
>+
>+    // remove this watcher and its result handler from hash table
>+    sActiveWatchers.Remove(mRes);
>+  }
>+
>   bool IsComplete() const
>   {
>     return mLen == (MSG1_SIZE + MSG2_SIZE);
>   }
> 
>   int GetFd() const
>   {
>     return mFd;
>@@ -1892,16 +1934,21 @@ public:
>     return ReadInt32(OFF_STATUS);
>   }
> 
>   int GetClientFd() const
>   {
>     return mClientFd;
>   }
> 
>+  BluetoothSocketResultHandler* GetResultHandler() const
>+  {
>+    return mRes;
>+  }
>+
> private:
>   BluetoothStatus RecvMsg1()
>   {
>     struct iovec iv;
>     memset(&iv, 0, sizeof(iv));
>     iv.iov_base = mBuf;
>     iv.iov_len = MSG1_SIZE;
> 
>@@ -1990,16 +2037,17 @@ private:
>     }
>   }
> 
>   MessageLoopForIO::FileDescriptorWatcher mWatcher;
>   int mFd;
>   int mClientFd;
>   unsigned char mLen;
>   uint8_t mBuf[MSG1_SIZE + MSG2_SIZE];
>+  nsRefPtr<BluetoothSocketResultHandler> mRes;
> };
> 
> /* |SocketMessageWatcherTask| starts a SocketMessageWatcher
>  * on the I/O task
>  */
> class SocketMessageWatcherTask MOZ_FINAL : public Task
> {
> public:
>@@ -2041,34 +2089,29 @@ private:
>  * connect operations by reading the socket messages from
>  * Bluedroid and forwarding the connected socket to the
>  * resource handler.
>  */
> class ConnectWatcher MOZ_FINAL : public SocketMessageWatcher
> {
> public:
>   ConnectWatcher(int aFd, BluetoothSocketResultHandler* aRes)
>-  : SocketMessageWatcher(aFd)
>-  , mRes(aRes)
>+  : SocketMessageWatcher(aFd, aRes)
>   { }
> 
>   void Proceed(BluetoothStatus aStatus) MOZ_OVERRIDE
>   {
>-    if (mRes) {
>-      DispatchBluetoothSocketResult(mRes,
>-                                   &BluetoothSocketResultHandler::Connect,
>-                                    GetFd(), GetBdAddress(),
>-                                    GetConnectionStatus(), aStatus);
>-    }
>+    DispatchBluetoothSocketResult(GetResultHandler(),
>+                                 &BluetoothSocketResultHandler::Connect,
>+                                  GetFd(), GetBdAddress(),
>+                                  GetConnectionStatus(), aStatus);
>+
>     MessageLoopForIO::current()->PostTask(
>       FROM_HERE, new DeleteTask<ConnectWatcher>(this));
>   }
>-
>-private:
>-  nsRefPtr<BluetoothSocketResultHandler> mRes;
> };
> 
> void
> BluetoothSocketInterface::Connect(const nsAString& aBdAddr,
>                                   BluetoothSocketType aType,
>                                   const uint8_t aUuid[16],
>                                   int aChannel, bool aEncrypt, bool aAuth,
>                                   BluetoothSocketResultHandler* aRes)
>@@ -2088,65 +2131,97 @@ BluetoothSocketInterface::Connect(const 
>   }
> 
>   if (status == BT_STATUS_SUCCESS) {
>     /* receive Bluedroid's socket-setup messages */
>     Task* t = new SocketMessageWatcherTask(new ConnectWatcher(fd, aRes));
>     XRE_GetIOMessageLoop()->PostTask(FROM_HERE, t);
>   } else if (aRes) {
>     DispatchBluetoothSocketResult(aRes,
>-                                  &BluetoothSocketResultHandler::Connect,
>+                                 &BluetoothSocketResultHandler::Connect,
>                                   -1, EmptyString(), 0,
>                                   ConvertDefault(status, STATUS_FAIL));
>   }
> }
> 
>-/* Specializes SocketMessageWatcher for Accept operations by
>- * reading the socket messages from Bluedroid and forwarding
>- * the received client socket to the resource handler. The
>- * first message is received immediately. When there's a new
>+/* |AcceptWatcher| specializes SocketMessageWatcher for Accept
>+ * operations by reading the socket messages from Bluedroid and
>+ * forwarding the received client socket to the resource handler.
>+ * The first message is received immediately. When there's a new
>  * connection, Bluedroid sends the 2nd message with the socket
>  * info and socket file descriptor.
>  */
> class AcceptWatcher MOZ_FINAL : public SocketMessageWatcher
> {
> public:
>   AcceptWatcher(int aFd, BluetoothSocketResultHandler* aRes)
>-  : SocketMessageWatcher(aFd)
>-  , mRes(aRes)
>-  {
>-    /* not supplying a result handler leaks received file descriptor */
>-    MOZ_ASSERT(mRes);
>-  }
>+  : SocketMessageWatcher(aFd, aRes)
>+  { }
> 
>   void Proceed(BluetoothStatus aStatus) MOZ_OVERRIDE
>   {
>-    if (mRes) {
>-      DispatchBluetoothSocketResult(mRes,
>-                                    &BluetoothSocketResultHandler::Accept,
>-                                    GetClientFd(), GetBdAddress(),
>-                                    GetConnectionStatus(),
>-                                    aStatus);
>-    }
>+    DispatchBluetoothSocketResult(GetResultHandler(),
>+                                 &BluetoothSocketResultHandler::Accept,
>+                                  GetClientFd(), GetBdAddress(),
>+                                  GetConnectionStatus(),
>+                                  aStatus);
>+
>     MessageLoopForIO::current()->PostTask(
>       FROM_HERE, new DeleteTask<AcceptWatcher>(this));
>   }
>-
>-private:
>-  nsRefPtr<BluetoothSocketResultHandler> mRes;
> };
> 
> void
> BluetoothSocketInterface::Accept(int aFd, BluetoothSocketResultHandler* aRes)
> {
>   /* receive Bluedroid's socket-setup messages and client fd */
>   Task* t = new SocketMessageWatcherTask(new AcceptWatcher(aFd, aRes));
>   XRE_GetIOMessageLoop()->PostTask(FROM_HERE, t);
> }
> 
>+/* |DeleteSocketMessageWatcherTask| deletes a watching SocketMessageWatcher
>+ * on the I/O task
>+ */
>+class DeleteSocketMessageWatcherTask MOZ_FINAL : public Task
>+{
>+public:
>+  DeleteSocketMessageWatcherTask(BluetoothSocketResultHandler* aRes)
>+  : mRes(aRes)
>+  {
>+    MOZ_ASSERT(mRes);
>+  }
>+
>+  void Run() MOZ_OVERRIDE
>+  {
>+    // look up hash table for the watcher corresponding to |mRes|
>+    SocketMessageWatcherWrapper* wrapper = sActiveWatchers.Get(mRes);
>+    if (!wrapper) {
>+      return;
>+    }
>+
>+    // stop the watcher if it exists
>+    SocketMessageWatcher* watcher = wrapper->GetSocketMessageWatcher();
>+    watcher->StopWatching();
>+    watcher->Proceed(STATUS_DONE);
>+  }
>+
>+private:
>+  BluetoothSocketResultHandler* mRes;
>+};
>+
>+void
>+BluetoothSocketInterface::Close(BluetoothSocketResultHandler* aRes)
>+{
>+  MOZ_ASSERT(aRes);
>+
>+  /* stop the watcher corresponding to |aRes| */
>+  Task* t = new DeleteSocketMessageWatcherTask(aRes);
>+  XRE_GetIOMessageLoop()->PostTask(FROM_HERE, t);
>+}
>+
> BluetoothSocketInterface::BluetoothSocketInterface(
>   const btsock_interface_t* aInterface)
> : mInterface(aInterface)
> {
>   MOZ_ASSERT(mInterface);
> }
> 
> BluetoothSocketInterface::~BluetoothSocketInterface()
>diff --git a/dom/bluetooth/bluedroid/BluetoothInterface.h b/dom/bluetooth/bluedroid/BluetoothInterface.h
>--- a/dom/bluetooth/bluedroid/BluetoothInterface.h
>+++ b/dom/bluetooth/bluedroid/BluetoothInterface.h
>@@ -60,16 +60,18 @@ public:
>   void Connect(const nsAString& aBdAddr,
>                BluetoothSocketType aType,
>                const uint8_t aUuid[16],
>                int aChannel, bool aEncrypt, bool aAuth,
>                BluetoothSocketResultHandler* aRes);
> 
>   void Accept(int aFd, BluetoothSocketResultHandler* aRes);
> 
>+  void Close(BluetoothSocketResultHandler* aRes);
>+
> protected:
>   BluetoothSocketInterface(const btsock_interface_t* aInterface);
>   ~BluetoothSocketInterface();
> 
> private:
>   const btsock_interface_t* mInterface;
> };
> 
>diff --git a/dom/bluetooth/bluedroid/BluetoothSocket.cpp b/dom/bluetooth/bluedroid/BluetoothSocket.cpp
>--- a/dom/bluetooth/bluedroid/BluetoothSocket.cpp
>+++ b/dom/bluetooth/bluedroid/BluetoothSocket.cpp
>@@ -387,17 +387,20 @@ public:
>   , mFd(aFd)
>   { }
> 
>   NS_IMETHOD Run() MOZ_OVERRIDE
>   {
>     MOZ_ASSERT(NS_IsMainThread());
>     MOZ_ASSERT(sBluetoothSocketInterface);
> 
>-    sBluetoothSocketInterface->Accept(mFd, new AcceptResultHandler(GetIO()));
>+    BluetoothSocketResultHandler* res = new AcceptResultHandler(GetIO());
>+    GetIO()->mConsumer->SetCurrentResultHandler(res);
>+
>+    sBluetoothSocketInterface->Accept(mFd, res);
> 
>     return NS_OK;
>   }
> 
> private:
>   int mFd;
> };
> 
>@@ -468,16 +471,17 @@ DroidSocketImpl::OnSocketCanConnectWitho
>   }
> }
> 
> BluetoothSocket::BluetoothSocket(BluetoothSocketObserver* aObserver,
>                                  BluetoothSocketType aType,
>                                  bool aAuth,
>                                  bool aEncrypt)
>   : mObserver(aObserver)
>+  , mCurrentRes(nullptr)
>   , mImpl(nullptr)
>   , mAuth(aAuth)
>   , mEncrypt(aEncrypt)
> {
>   MOZ_ASSERT(aObserver);
> 
>   EnsureBluetoothSocketHalLoad();
>   mDeviceAddress.AssignLiteral(BLUETOOTH_ADDRESS_NONE);
>@@ -519,23 +523,25 @@ BluetoothSocket::ConnectSocket(const nsA
> {
>   MOZ_ASSERT(NS_IsMainThread());
>   NS_ENSURE_FALSE(mImpl, false);
> 
>   SetConnectionStatus(SOCKET_CONNECTING);
> 
>   mImpl = new DroidSocketImpl(XRE_GetIOMessageLoop(), this);
> 
>+  BluetoothSocketResultHandler* res = new ConnectSocketResultHandler(mImpl);
>+  SetCurrentResultHandler(res);
>+
>   // TODO: uuid as argument
>   sBluetoothSocketInterface->Connect(
>     aDeviceAddress,
>     BluetoothSocketType::RFCOMM,
>     UUID_OBEX_OBJECT_PUSH,
>-    aChannel, mEncrypt, mAuth,
>-    new ConnectSocketResultHandler(mImpl));
>+    aChannel, mEncrypt, mAuth, res);
> 
>   return true;
> }
> 
> class ListenResultHandler MOZ_FINAL : public BluetoothSocketResultHandler
> {
> public:
>   ListenResultHandler(DroidSocketImpl* aImpl)
>@@ -568,34 +574,43 @@ BluetoothSocket::ListenSocket(int aChann
> {
>   MOZ_ASSERT(NS_IsMainThread());
>   NS_ENSURE_FALSE(mImpl, false);
> 
>   SetConnectionStatus(SOCKET_LISTENING);
> 
>   mImpl = new DroidSocketImpl(XRE_GetIOMessageLoop(), this);
> 
>+  BluetoothSocketResultHandler* res = new ListenResultHandler(mImpl);
>+  SetCurrentResultHandler(res);
>+
>   sBluetoothSocketInterface->Listen(
>     BluetoothSocketType::RFCOMM,
>     NS_LITERAL_STRING("OBEX Object Push"),
>     UUID_OBEX_OBJECT_PUSH,
>-    aChannel, mEncrypt, mAuth,
>-    new ListenResultHandler(mImpl));
>+    aChannel, mEncrypt, mAuth, res);
> 
>   return true;
> }
> 
> void
> BluetoothSocket::CloseSocket()
> {
>   MOZ_ASSERT(NS_IsMainThread());
>+  MOZ_ASSERT(sBluetoothSocketInterface);
>+
>   if (!mImpl) {
>     return;
>   }
> 
>+  // Stop any watching |SocketMessageWatcher|
>+  if (mCurrentRes) {
>+    sBluetoothSocketInterface->Close(mCurrentRes);
>+  }
>+
>   // From this point on, we consider mImpl as being deleted.
>   // We sever the relationship here so any future calls to listen or connect
>   // will create a new implementation.
>   mImpl->ShutdownOnMainThread();
>   XRE_GetIOMessageLoop()->PostTask(
>     FROM_HERE, new SocketIOShutdownTask<DroidSocketImpl>(mImpl));
> 
>   mImpl = nullptr;
>@@ -625,24 +640,28 @@ BluetoothSocket::ReceiveSocketData(nsAut
>   mObserver->ReceiveSocketData(this, aMessage);
> }
> 
> void
> BluetoothSocket::OnConnectSuccess()
> {
>   MOZ_ASSERT(NS_IsMainThread());
>   MOZ_ASSERT(mObserver);
>+
>+  SetCurrentResultHandler(nullptr);
>   mObserver->OnSocketConnectSuccess(this);
> }
> 
> void
> BluetoothSocket::OnConnectError()
> {
>   MOZ_ASSERT(NS_IsMainThread());
>   MOZ_ASSERT(mObserver);
>+
>+  SetCurrentResultHandler(nullptr);
>   mObserver->OnSocketConnectError(this);
> }
> 
> void
> BluetoothSocket::OnDisconnect()
> {
>   MOZ_ASSERT(NS_IsMainThread());
>   MOZ_ASSERT(mObserver);
>diff --git a/dom/bluetooth/bluedroid/BluetoothSocket.h b/dom/bluetooth/bluedroid/BluetoothSocket.h
>--- a/dom/bluetooth/bluedroid/BluetoothSocket.h
>+++ b/dom/bluetooth/bluedroid/BluetoothSocket.h
>@@ -8,16 +8,17 @@
> #define mozilla_dom_bluetooth_BluetoothSocket_h
> 
> #include "BluetoothCommon.h"
> #include "mozilla/ipc/SocketBase.h"
> 
> BEGIN_BLUETOOTH_NAMESPACE
> 
> class BluetoothSocketObserver;
>+class BluetoothSocketResultHandler;
> class DroidSocketImpl;
> 
> class BluetoothSocket : public mozilla::ipc::SocketConsumerBase
> {
> public:
>   BluetoothSocket(BluetoothSocketObserver* aObserver,
>                   BluetoothSocketType aType,
>                   bool aAuth,
>@@ -42,18 +43,24 @@ public:
>     aDeviceAddress = mDeviceAddress;
>   }
> 
>   inline void SetAddress(const nsAString& aDeviceAddress)
>   {
>     mDeviceAddress = aDeviceAddress;
>   }
> 
>+  inline void SetCurrentResultHandler(BluetoothSocketResultHandler* aRes)
>+  {
>+    mCurrentRes = aRes;
>+  }
>+
> private:
>   BluetoothSocketObserver* mObserver;
>+  BluetoothSocketResultHandler* mCurrentRes;
>   DroidSocketImpl* mImpl;
>   nsString mDeviceAddress;
>   bool mAuth;
>   bool mEncrypt;
> };
> 
> END_BLUETOOTH_NAMESPACE
>
Comment 42 only adds "r=fabrice" to patch description.
Set "checkin-needed" for m-c patch in comment 38. Try server is in comment 40.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/15aae506ef73

Friendly reminder that your commit message should be summarizing what the patch is doing, not restating the problem it's fixing :)
https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/15aae506ef73
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #45)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/15aae506ef73
> 
> Friendly reminder that your commit message should be summarizing what the
> patch is doing, not restating the problem it's fixing :)
> https://developer.mozilla.org/en-US/docs/Developer_Guide/
> Committing_Rules_and_Responsibilities#Checkin_comment

Got it. Thanks for reminding:)
Attachment #8495184 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Set "checkin-needed" for v2.1 patch in comment 41.
Keywords: checkin-needed
We have bug queries for uplifts. No need to set checkin-needed :)
Flame v2.1 build's Gonk hash: 5883a99b6528ced9dafaed8d3ca2405fb285537e.
Disregard previous comment (Comment 51); Gonk hash not relevant to this bug.
Several testers have not been able to verify this fix. The bug showed up again in today's ST, and this evening I haven't been able to successfully pair my Mac laptop with a Flame to even test this due to:

(1) Either I am hitting Bug 1074152 when I try to send a file from the Flame gallery to the Laptop
(2) I cannot pair the laptop with the Flame - the pairing request seems to go through (get the PIN dialog), but then the devices never pair successfully.

The team will spend some more time on this tomorrow.
Depends on: 1074152
We are blocked from verifying this by bug 1074152.  Will re-check when that is fixed.
(In reply to Marcia Knous [:marcia - use needinfo] from comment #53)
> Several testers have not been able to verify this fix. The bug showed up
> again in today's ST, and this evening I haven't been able to successfully
> pair my Mac laptop with a Flame to even test this due to:
> 
> (1) Either I am hitting Bug 1074152 when I try to send a file from the Flame
> gallery to the Laptop
Users can't hit search again button if they didn't discovery the remote device at the first try.
Now it's already landed on m-c and waiting for uplifting to aurora.
> (2) I cannot pair the laptop with the Flame - the pairing request seems to
> go through (get the PIN dialog), but then the devices never pair
> successfully.
> 
To be clear, I think this bug is also blocked by Bug 1065999 based on this symptom you described.
> The team will spend some more time on this tomorrow.

I have flash today's aurora code base and manually apply the patch provided in Bug 1065999.
And it works fine on my end.
Depends on: 1065999
This issue is verified fixed on todays Flame 2.1

User is able to pair with both a Mac, or Linux based laptop and receive or send files through blue tooth without issue.


Flame 2.1

Device: Flame 2.1 KK (319mb) (Full Flash)
BuildID: 20141012001201
Gaia: d18e130216cd3960cd327179364d9f71e42debda
Gecko: 610ee0e6a776
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
================================================================================================

However I am unable to resolve this bug as fixed because I am blocked from checking on Flame 2.2 because of bug 1080090
QA Whiteboard: [QAnalyst-Triage+][lead-review+][COM=Bluetooth] → [QAnalyst-Triage?][lead-review+][COM=Bluetooth]
Depends on: 1080090
Flags: needinfo?(ktucker)
This issue is now verified fixed for Flame 2.2

User is able to transfer and receive files without issue from a bluetooth connection with another phone, or a laptop.

Flame 2.2

Device: Flame 2.2 Master KK (319mb) (Full Flash)
BuildID: 20141013040202
Gaia: 3b81896f04a02697e615fa5390086bd5ecfed84f
Gecko: f547cf19d104
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Removing bug 1080090, as a work around was found so this bug could be verified
Status: RESOLVED → VERIFIED
No longer depends on: 1080090
QA Whiteboard: [QAnalyst-Triage?][lead-review+][COM=Bluetooth] → [QAnalyst-Triage+][lead-review+][COM=Bluetooth]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.