Closed Bug 854846 Opened 7 years ago Closed 7 years ago

Failed to show pairing confirmation window during sending a file via Bluetooth App

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:leo+, firefox22 wontfix, firefox23 wontfix, firefox24 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix)

RESOLVED FIXED
blocking-b2g leo+
Tracking Status
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix

People

(Reporter: gyeh, Assigned: qdot)

References

()

Details

Attachments

(2 files, 7 obsolete files)

5.31 KB, patch
Details | Diff | Splinter Review
5.08 KB, patch
Details | Diff | Splinter Review
device: unagi
gecko: b2g-18
gaia: v1-train

STR:
1. Launch Settings app and pair with a macbook
2. Unpair with unagi from macbook
3. Launch Gallary app and press share button
   (Somehow, Settings app is killed due to OOP)
4. Send a file from unagi to macbook
5. Pairing process is re-initiated by macbook and unagi receive 

Expected result:
* Settings app should be launched in background
* And then get pending system message of "bluetooth-requestconfirmation"
* And then show pairing confirmation window

Actual result:
* window_manager failed to open Settings app
* No pairing confirmation window is shown






I played with several different phones and tablets. For some devices, they won't re-initiate the pairing process, and unagi can send file successfully. After transferring, we can also find unagi on remote device as a paired device. It should be fine in this case.

However, for other devices like macbook, pairing process is re-initiated and we failed to handle this situation as described above.
By the way, I saw another problem also happened when issueing Bluetooth AVRCP play command via system message, Music application cannot be launched. I am not sure something related to current problem "window_manager failed to open Settings app".
Please ignore what I've said on comment 1, it is a different issue. Music can be launched but in background not foreground.
(In reply to Shawn Huang from comment #1)
> By the way, I saw another problem also happened when issueing Bluetooth
> AVRCP play command via system message, Music application cannot be launched.
> I am not sure something related to current problem "window_manager failed to
> open Settings app".

We always don't expect an app is opened without certain permission because arbitrarily opening a new app would interrupt current app and results in an app transition effect.

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/window_manager.js#L1463-L1470

For your use case, the only way I could figure out now is to add a new attribute in event.detail to tell system app that we want to 'do bring the app to foreground'.

But I wonder this has some permission concern, so please check with backend API folks, e.x. Jonas.
@Alive,
Thanks for comment. I open another bug 855208 for my avrcp and music app case.
Blocks: 849101
Nominate because this blocks 849101 and I guess the root cause are the same.
Assignee: nobody → alive
blocking-b2g: --- → leo?
Today I checked something w/ Gina:

* This only happens on few devices.
* The characteristic of devices is: That device would ask pairing again even when it has paired to fxOS before.
* Settings app is opened by Window Manager via system message's open-app event in the background.
* Settings app doesn't go into setMessageHandler after it's launched until timeout(~30s)
* Only happens when bluetooth transfer app is opened.

I wonder because 2 apps are requesting BT adapter resource so the last one -- settings app -- is  delayed to get callback of getDefaultAdaptor.
(In reply to Alive Kuo [:alive] from comment #6)
> Today I checked something w/ Gina:
> 
> * This only happens on few devices.

are any of those devices our shipping hardware for v1.1?
(In reply to lsblakk@mozilla.com from comment #7)
> (In reply to Alive Kuo [:alive] from comment #6)
> > Today I checked something w/ Gina:
> > 
> > * This only happens on few devices.
> 
> are any of those devices our shipping hardware for v1.1?
I think he refered to "remote" devices which support Bluetooth 2.1 (secure simple pairing),
Yesterday I made some logging in the path to show pairing:

1. Gecko asks WM to open settings app(open-app)
2. WM opens settings app
3. settings app lazily loads connectivity.js
4. connectivity.js registers system event handler
5. connectivity.js lazily loads bluetooth subpage by chaning hash to '#bluetooth'
6. After 1.5 second, connectivity.js directly calls onPairingRequest in bluetooth.js
7. onPairingRequest calls window.open(....,'attention')
8. Gecko passes the mozbrowseropenwindow to gaia attentionscreen
9. Gaia displays the attentionscreen on top of current app

The only thing could be improved in this procedure is the double lazy loading steps, but I had tested with
1). Send file to A device
2). When sending in BTtransfer app, use B device to require pairing to my unagi.
And it works fine.

So now the problem is why only sending/pairing in the same device blocks.
Blocked because blocks a blocker
blocking-b2g: leo? → leo+
steal the bug.
Assignee: alive → arthur.chen
Settings app is supposed to be launched for the pairing request. Not sure about the reason but somehow an iframe does not start to load until any callback of |defaultAdapter.connect| is called. That's why the pairing confirmation window is shown after the pairing timeout.
I found we did a blocking call (connect) on IO thread in unixsocket. If the pairing confirmation window which is a iframe is also loaded in IO thread, then it might be blocked by the blocking call of connect. Make sense?
Kyle, any idea for comment 13?
I'm not really familiar with the "pair then send" functionality, but if we're receiving a file, wouldn't accept() be getting called?

Anyways, yeah, it looks like there's an incorrect assumption that connect() will only be called once we can actually connect. We should probably handle connect the same way that tzimmerman did accept() in bug 836523 (BTW, should we uplift that to v1.1 while we're at this?), this time making the socket non-block and adding a logic branch in OnFileCanWrite that actually finishes out the connect() call once poll kicks back something for us to do. cc'ing tzimmermann on this to get his thoughts.
Just found another failure case too: Notice how there's a whole bunch of places we just { return; } if we have a failure, for issues like not being able to create an fd? We never throw the ERROR signal to the main thread there, meaning a connection failure on those issues would fail silently and the main thread would never progress.
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #15)
> I'm not really familiar with the "pair then send" functionality, but if
> we're receiving a file, wouldn't accept() be getting called?

Ignore this question. Just realized I misread the direction the file was going in the description. :)
Hey there!

Setting the socket fd to non-blocking, calling connect on it, and giving it to the IO thread should work. The IO thread should run OnFileCanWrite when the connection has been established and the socket should be ready. A successive write call should report any errors that occurred during the connect. Reminds me that we need to implement bug 852757. :/
Ok. I've got a WIP version of the connect call done, will post here soon.

Thinking about filing a follow up on our silent erroring issue. Tried fixing it in this one but it gets kinda big and complicated.
Ok. Bug 867400 filed as silent return followup.
I threw this together pretty quick, and it's only gotten the "does it compile?" test. About to try on phone now. Just making sure it's up here because I'm going to be at a conference the next couple of days and don't want things stalled if I can't work efficiently during the boring talks. :)
Attachment #743860 - Flags: feedback?(tzimmermann)
Hunk staging while trying to pull apart my connect and silent error changes seems to have gone awry. Fixed.
Attachment #743860 - Attachment is obsolete: true
Attachment #743860 - Flags: feedback?(tzimmermann)
Attachment #743867 - Flags: feedback?(tzimmermann)
Hmm. I'm getting the following error when I try to send a file to the macbook:

I/Gecko   (  109): [Parent 109] WARNING: BluetoothOppManager has been already connected: file /share/code/mozbuild/B2G/gecko/dom/bluetooth/BluetoothOppManager.cpp, line 263
I/Gecko   (  109): [Parent 109] WARNING: Unknown Profile: file /share/code/mozbuild/B2G/gecko/dom/bluetooth/linux/BluetoothDBusService.cpp, line 2522

Is it trying to send the file through the wrong service or something?
Adding a b2g18/birch version.
I'v tried patch Patch 1 v1 for b2g18, it looks like connection failure always occurs.
Comment on attachment 743867 [details] [diff] [review]
Patch 1 (v2) - WIP: Make connect non-blocking

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

Just two things I noticed.

::: ipc/unixsocket/UnixSocket.cpp
@@ +812,5 @@
> +    }
> +  } else if (status == SOCKET_CONNECTING) {
> +    int error;
> +    socklen_t len = sizeof(error);
> +    getsockopt(mFd.get(), SOL_SOCKET, SO_ERROR, &error, &len);

Error checking; just in case...

@@ +830,1 @@
>      }

I think for correctness you should set the file descriptor back to blocking before calling SetUp. Otherwise the same code paths in UnixSocket might behave differently, depending on the way the connection was established.
Attachment #743867 - Flags: feedback?(tzimmermann) → feedback+
Addressed comments from feedback, and also fixed issue where I meant to check errno instead of the return value, since if errno > 0 then ret is ALWAYS -1, so it'll never equal E_INPROGRESS. This made things work for me.
Attachment #743867 - Attachment is obsolete: true
Attachment #745402 - Flags: review?(tzimmermann)
Attachment #744042 - Attachment is obsolete: true
Attachment #745421 - Flags: review?(tzimmermann)
Comment on attachment 745421 [details] [diff] [review]
B2G18 - Patch 1 (v2) - Make connect non-block

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

There is a v3 of this patch, so this file is outdated, right?
Attachment #745421 - Flags: review?(tzimmermann) → review-
Comment on attachment 745402 [details] [diff] [review]
Patch 1 (v3) - Make UnixSocket connect() function non-blocking

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

Looks good, except for the commit message. ;)

While reviewing this patch I though that there are so many places where we dispatch socket events to the main thread, we should probably have a single method for this.
Attachment #745402 - Flags: review?(tzimmermann) → review+
Since this has been identified as a gecko bug. Feel free to steal it if you want. :)
Comment on attachment 745421 [details] [diff] [review]
B2G18 - Patch 1 (v2) - Make connect non-block

It's v2 of the B2G version of the patch, because I started the B2G versioning 1 behind the m-c versions.
Attachment #745421 - Flags: review- → review?(tzimmermann)
Assignee: arthur.chen → kyle
(In reply to Thomas Zimmermann [:tzimmermann] from comment #30)

> Looks good, except for the commit message. ;)

Agh. I keep forgetting that matters now that I'm not landing my own stuff to all the branches. :|

> While reviewing this patch I though that there are so many places where we
> dispatch socket events to the main thread, we should probably have a single
> method for this.

Yeah, I definitely agree. I'm hoping to do some serious cleanup of all of that in bug 843857.
Comment on attachment 745421 [details] [diff] [review]
B2G18 - Patch 1 (v2) - Make connect non-block

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

Looks good.
Attachment #745421 - Flags: review?(tzimmermann) → review+
hmm... after trying patch (v3), i still cannot connect to HFP headset. i will also help to check later.
Comment on attachment 745402 [details] [diff] [review]
Patch 1 (v3) - Make UnixSocket connect() function non-blocking

Adding Shawn as extra review since he's reporting problems and we want this verified working before landing.
Attachment #745402 - Flags: review?(shawnjohnjr)
Got stuck with a bad commit off m-i during try. Will retry after headset/file transfer verification.

https://tbpl.mozilla.org/?tree=Try&rev=93b25e041abb
Attachment #745402 - Flags: review?(shawnjohnjr) → review?(shuang)
Ok. I just tried pairing a headset on v1-train branch with the b2g18 patch above. I can pair and connect fine, but I'm getting:

I/GonkDBus(  110): UnixSocket Connection delayed!
I/Gecko   (  110): [Parent 110] WARNING: Trying to SendCommand() without a SLC: file /share/code/mozbuild/B2G/gecko/dom/bluetooth/BluetoothHfpManager.cpp, line 1078

If I reboot the phone, the headset reconnects and comes up, and it works fine. So, looks like we're doing something out of order. Will continue diagnosing.
Ended up testing the last patch incorrectly. Made sure RIL (which uses UnixSocket) connected to the outside world (which it did), but wasn't checking the return to make sure the OnFileCanWrite branch and SocketEvents were firing. Turns out they weren't, because I set up a read watch (via SetUpIO()), which should've been a write watch. Changing that to a write watch, and then setting the read watch after connection, fixes the issues Shawn was seeing. I tested this on an unagi, it now seems to work for me.
Attachment #745421 - Attachment is obsolete: true
Attachment #747120 - Flags: review?(tzimmermann)
Attachment #745402 - Attachment is obsolete: true
Attachment #745402 - Flags: review?(shuang)
Attachment #747159 - Flags: review?(tzimmermann)
Attachment #747120 - Flags: review?(tzimmermann) → review+
Comment on attachment 747159 [details] [diff] [review]
Patch 1 (v4) - Make UnixSocket connect() function non-blocking

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

You can r= me with the two comments fixed here and in the patch for b2g18.

::: ipc/unixsocket/UnixSocket.cpp
@@ +549,4 @@
>      NS_WARNING("Cannot create socket address!");
>      return;
>    }
> +  

Is that a white-space error? If so please clean it up before committing the patch.

@@ +563,5 @@
>    if (ret) {
> +    if (errno == EINPROGRESS) {
> +      // Select blocking IO again, since we've now at least queue'd the connect
> +      // as nonblock.
> +      int block_opts = fcntl(mFd.get(), F_GETFL, 0) & ~O_NONBLOCK;

I should have seen this before, but there should be an error check for fcntl here.
Attachment #747159 - Flags: review?(tzimmermann) → review+
r+'d by tzimmermann, plus nits picked.
Attachment #747159 - Attachment is obsolete: true
Attachment #748987 - Attachment description: Patch 1 (v5) - Make UnixSocket connect() function non-blocking → Patch 1 (v5) - Make UnixSocket connect() function non-blocking; r=tzimmermann
https://hg.mozilla.org/mozilla-central/rev/6c52203a47b8
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Flags: in-moztrap?
Flags: in-moztrap? → in-moztrap+
This issue has been Failed verified on Nexus5 2.2, and successfully verifiedon Nexus5 3.0
N5-2.2
20150308002503
Gaia Revision          166491b92278dc9e648f8d49ab02d9ca00d74421
Gaia Date              2015-03-06 18:26:27
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/a48af0b5a6e4
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.0
Firmware(Incremental)  eng.cltbld.20150308.052750
Firmware Date          Sun Mar  8 05:28:06 EDT 2015
Bootloader             HHZ12d

N5-3.0:
Build ID               20150308160204
Gaia Revision          fea83511df9ccba64259346bc02ebf2c417a12c2
Gaia Date              2015-03-08 06:36:28
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/eab4a81e4457
Gecko Version          39.0a1
Device Name            hammerhead
Firmware(Release)      5.0
Firmware(Incremental)  eng.cltbld.20150308.192431
Firmware Date          Sun Mar  8 19:24:47 EDT 2015
Bootloader             HHZ12d
Whiteboard: v2.2-nexus-5-l
I try this issue more times on Nexus5 2.2, it can work agian normally.
Whiteboard: v2.2-nexus-5-l
You need to log in before you can comment on or make changes to this bug.