1196125 - Launch Bluetooth PBAP/MAP app by sending system message if the apps are not ready for receiving related events.

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Jamin Liu [:jaliu][:Jamin]]

To support Bluetooth PBAP and MAP as a server, FxOS should listen to the related events for providing service. Since FxOS haven't supported something like Android background service, it's necessary to provide a mechanism to launch the PBAP/MAP service app when receiving PBAP/MAP requests.

Comment 1

[Will Wang [:WillWang]]

Take this great bug ;-)

Comment 2

[Will Wang [:WillWang]]

Nominate as 2.2r blocking since PBAP/MAP requests will be missed.

Comment 3

[Ben Tian (inactive)]

Set 2.2r blocker for it's required to wake app up to handle incoming requests.

Comment 4

[Will Wang [:WillWang]]

Currently I can trigger a fixed gaia app by sending system message,
and I'm writing a function which can trigger the corresponding app for each PBAP/MAP event.

Comment 5

[Will Wang [:WillWang]]

Now, I am able to:

1. Make an MAP event by jHMI tool
2. Filter out the event when |DistributeSignal| is called
3. Queue the event within BluetoothService
4. Send a system message to open the SMS gaia app, which can be identified in b2g-ps.

Comment 6

[Will Wang [:WillWang]]

I am adding the gaia patches of PBAP/MAP to test the whole process,
reference patches are as following:
- PBAP: landed on 2.2r, refer to Bug 1200091
- MAP: not landed on any branch, has a WIP patch on Bug 1218685

Comment 7

[Will Wang [:WillWang]]

Now,
- Bluetooth adapter becomes aware of the object creation by specified gaia app(say, sms app)
- Then, once the gaia app calls |addEventListener|, |EventListenerAdded| in gecko will be consequently called to perform checking for a set of events, using |HasListenersFor|. [1]

====
[1] piece of log:

 46 11-18 19:25:30.710 I/Messages(25223): Content JS LOG: ==== after BA
 47 11-18 19:25:30.710 I/Messages(25223):     at init/< (app://sms.gaiamobile.org/services/js/map/map_manager.js:97:15)
 48 11-18 19:25:30.713 I/Messages(25223): Content JS LOG: defaultAdapter changed. address: 22:22:16:5c:bb:9c
 49 11-18 19:25:30.713 I/Messages(25223):     at init/< (app://sms.gaiamobile.org/services/js/map/map_manager.js:98:1)
 50 11-18 19:25:30.713 I/GeckoBluetooth(25223): EventListenerAdded: ==== HasListenersFor(nsGkAtoms::onmapfolderlistingreq)  = [0]
 51 11-18 19:25:30.713 I/GeckoBluetooth(25223): EventListenerAdded: ==== HasListenersFor(nsGkAtoms::onmapmessageslistingreq)  = [1]
 52 11-18 19:25:30.713 I/GeckoBluetooth(25223): EventListenerAdded: ==== HasListenersFor(nsGkAtoms::onmapgetmessagereq)  = [0]
 53 11-18 19:25:30.713 I/GeckoBluetooth(25223): EventListenerAdded: ==== HasListenersFor(nsGkAtoms::onmapsetmessagestatusreq)  = [0]
 54 11-18 19:25:30.713 I/GeckoBluetooth(25223): EventListenerAdded: ==== HasListenersFor(nsGkAtoms::onmapsendmessagereq)  = [0]
 55 11-18 19:25:30.714 I/Messages(25223): Content JS LOG: ==== adapter.addEventListener = mapmessageslistingreq
 56 11-18 19:25:30.714 I/Messages(25223):     at init/bind_event (app://sms.gaiamobile.org/services/js/map/map_manager.js:73:7)
 57 11-18 19:25:30.714 I/GeckoBluetooth(25223): EventListenerAdded: ==== HasListenersFor(nsGkAtoms::onmapfolderlistingreq)  = [0]
 58 11-18 19:25:30.714 I/GeckoBluetooth(25223): EventListenerAdded: ==== HasListenersFor(nsGkAtoms::onmapmessageslistingreq)  = [1]
 59 11-18 19:25:30.714 I/GeckoBluetooth(25223): EventListenerAdded: ==== HasListenersFor(nsGkAtoms::onmapgetmessagereq)  = [1]
 60 11-18 19:25:30.714 I/GeckoBluetooth(25223): EventListenerAdded: ==== HasListenersFor(nsGkAtoms::onmapsetmessagestatusreq)  = [0]
 61 11-18 19:25:30.714 I/GeckoBluetooth(25223): EventListenerAdded: ==== HasListenersFor(nsGkAtoms::onmapsendmessagereq)  = [0]
 62 11-18 19:25:30.714 I/Messages(25223): Content JS LOG: ==== adapter.addEventListener = mapgetmessagereq

Comment 8

[Will Wang [:WillWang]]

Attachment: [WIP] event-missing.patch

Just for reference, it's a "WIP" patch for my currently ongoing testing.

Comment 9

[Ben Tian (inactive)]

Remove 2.2r+ blocking flag for internal resource reallocation.

Comment 10

[Will Wang [:WillWang]]

Attachment: [WIP] event-missing-v2.patch

After consideration, today I refactor the behavior and control structure for several code sections.
Especially the way we determine whether the expected bluetooth signal observer exist or not in |DistributeSignal|.

I will state the reasons in detail later.

Comment 11

[Will Wang [:WillWang]]

Attachment: event-missing-v3.patch

Hi Ben, Shawn,

Apologize for my late upload.

Here is my revised patch, which can fix this event missing issue,
that is, PBAP/MAP events will not be missed anymore when the corresponding gaia app is not launched in advance.

The brief handling process:
1. Queue the incoming event when no observer was registered
2. Launch corresponding gaia app via system message (|broadcastMessage|)
3. Gaia app will |addEventListener| for every event
4. Register the observer once all event listener are set
5. De-queue every pending event to gaia via |DistributeSignal|
6. Corresponding gaia listener is invoked for the event
 
Within the above process, I think some points are worth discussing,
for example:
A. System message can be more precisely utilized by calling |sendMessage| instead of broadcasting it.
B. I changed the event signal name distributed by PBAP/MAP from KEY_ADAPTER to KEY_PBAP/KEY_MAP to achieve better code quality, however there might have better way on this, please correct me if any.
C. I choose not to limit the launched app by app origin for better flexibility
D. I limit the observer will not be registered unless all event listener are set

Please ignore logs added in this patch, which will be removed once r+.
Besides, some modifications are introduced by bug 1186840(which was just landed in b2g-inbound) and I will rebase it once needed.

Please kindly review my patch and I will fix it asap,
thanks!

Comment 12

[Will Wang [:WillWang]]

Typo:

(In reply to Will Wang [:WillWang] from comment #11)
> B. I changed the event signal name distributed by PBAP/MAP ....

signal name (X)
signal path (O)

Comment 13

[Ben Tian (inactive)]

Comment on attachment 8694131 [details] [diff] [review]
event-missing-v3.patch

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

Please split your change and bug 1186840 change, and remove redundant logs. I'll check the patch flow first.

Comment 14

[Will Wang [:WillWang]]

Attachment: 0001-Bug-1196125-Launch-corresponding-gaia-app-to-receive.patch

Hi Ben,

Thanks for your suggestion!

Here is the revised patch which remove debugging logs and split the MAP bug part,
thanks!

Comment 15

[Ben Tian (inactive)]

Comment on attachment 8694153 [details] [diff] [review]
0001-Bug-1196125-Launch-corresponding-gaia-app-to-receive.patch

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

::: dom/bluetooth/common/BluetoothCommon.h
@@ +227,5 @@
>  
>  /**
> + * System message to launch sms app
> + */
> +#define SYS_MSG_BT_MAP_REQ                    "sms-received"

This will trigger original gaia handler for this system message and results in undesired side effect.

@@ +232,5 @@
> +
> +/**
> + * System message to launch dialer app
> + */
> +#define SYS_MSG_BT_PBAP_REQ                   "bluetooth-dialer-command"

Ditto.

Comment 16

[Will Wang [:WillWang]]

Attachment: 0001-Bug-1196125-Launch-corresponding-gaia-app-to-receive.patch

Per comment 15, here is the revised patch which replaces the used system message with a new one, and it was tested for both MAP[1] and PBAP[2].

[1]

12-02 17:28:18.236 I/GeckoBluetooth( 1138): DistributeSignal: No observer registered for path /B2G/bluetooth/map
...
12-02 17:28:18.337 I/GeckoBluetooth( 1138): DistributeSignal: Queue the request and sent system message to launch sms app
12-02 17:28:18.684 I/Messages( 1761): Content JS LOG: ==== after BA
...
12-02 17:28:19.184 I/Messages( 1761): Content JS LOG: ==== 1 [map] mapmessageslisting

APPLICATION    SEC USER     PID   PPID  VSIZE  RSS     WCHAN    PC        NAME
b2g              0 root      1138  1     359564 113932 ffffffff b6d8a8dc S /system/b2g/b2g
(Nuwa)           0 root      1144  1138  109548 21104 ffffffff b6d8a8dc S /system/b2g/b2g
Default Home Sc  2 u0_a1319  1319  1144  187692 52188 ffffffff b6d8a8dc S /system/b2g/b2g
Built-in Keyboa  2 u0_a1536  1536  1144  126076 31148 ffffffff b6d8a8dc S /system/b2g/b2g
Find My Device   2 u0_a1711  1711  1144  128668 30504 ffffffff b6d8a8dc S /system/b2g/b2g
Messages         2 u0_a1761  1761  1144  132804 39412 ffffffff b6d8a8dc S /system/b2g/b2g
(Preallocated a  2 u0_a1791  1791  1144  122940 26148 ffffffff b6d8a8dc S /system/b2g/b2g

[2]

12-02 17:28:37.414 I/GeckoBluetooth( 1138): OnSocketConnectSuccess: PBAP socket is connected
...
12-02 17:28:43.522 I/GeckoBluetooth( 1138): DistributeSignal: No observer registered for path /B2G/bluetooth/pbap
...
12-02 17:28:43.615 I/GeckoBluetooth( 1138): DistributeSignal: Queue the request and sent system message to launch dialer app
...
12-02 17:28:43.964 I/Communications( 1791): Content JS LOG: got pullphonebook event
12-02 17:28:43.964 I/Communications( 1791):     at pullphonebook (app://communications.gaiamobile.org/pbap/js/pbap.js:74:5)
12-02 17:28:43.964 I/Communications( 1791): Content JS LOG: [object BluetoothPhonebookPullingEvent]
12-02 17:28:43.964 I/Communications( 1791):     at pullphonebook (app://communications.gaiamobile.org/pbap/js/pbap.js:75:5)


APPLICATION    SEC USER     PID   PPID  VSIZE  RSS     WCHAN    PC        NAME
b2g              0 root      1138  1     356272 114644 ffffffff b6d8a8dc S /system/b2g/b2g
(Nuwa)           0 root      1144  1138  109548 21104 ffffffff b6d8a8dc S /system/b2g/b2g
Default Home Sc  2 u0_a1319  1319  1144  186356 51928 ffffffff b6d8a8dc S /system/b2g/b2g
Built-in Keyboa  2 u0_a1536  1536  1144  125052 31160 ffffffff b6d8a8dc S /system/b2g/b2g
Find My Device   2 u0_a1711  1711  1144  124476 30816 ffffffff b6d8a8dc S /system/b2g/b2g
Messages         2 u0_a1761  1761  1144  131780 39000 ffffffff b6d8a8dc S /system/b2g/b2g
Communications   2 u0_a1791  1791  1144  128892 33776 ffffffff b6d8a8dc S /system/b2g/b2g
(Preallocated a  2 u0_a1829  1829  1144  122940 25920 ffffffff b6d8a8dc S /system/b2g/b2g

Comment 17

[Ben Tian (inactive)]

(In reply to Will Wang [:WillWang] from comment #16)
> Per comment 15, here is the revised patch which replaces the used system
> message with a new one, and it was tested for both MAP[1] and PBAP[2].

Please open another bug for corresponding gaia change in dialer and message apps, and prepare pull request for review.

Comment 18

[Will Wang [:WillWang]]

(In reply to Ben Tian [:btian] from comment #17)
> (In reply to Will Wang [:WillWang] from comment #16)
> > Per comment 15, here is the revised patch which replaces the used system
> > message with a new one, and it was tested for both MAP[1] and PBAP[2].
> 
> Please open another bug for corresponding gaia change in dialer and message
> apps, and prepare pull request for review.

OK, I am discussing the Gaia change with @Sean this afternoon, and will do it as you suggested.

Comment 19

[Ben Tian (inactive)]

Comment on attachment 8694652 [details] [diff] [review]
0001-Bug-1196125-Launch-corresponding-gaia-app-to-receive.patch

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

::: dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp
@@ +1346,4 @@
>                              Map::AppParametersTagId::StatusValue);
>  
>    bs->DistributeSignal(NS_LITERAL_STRING(MAP_SET_MESSAGE_STATUS_REQ_ID),
> +                       NS_LITERAL_STRING(KEY_MAP), data);

Also revise [1]

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp#1422

::: dom/bluetooth/common/BluetoothService.cpp
@@ +263,5 @@
> +  // observer table.
> +  if (XRE_IsParentProcess()){
> +
> +    // For pairing requests
> +    if(!mPendingPairReqSignals.IsEmpty() &&

nit: space before (

@@ +264,5 @@
> +  if (XRE_IsParentProcess()){
> +
> +    // For pairing requests
> +    if(!mPendingPairReqSignals.IsEmpty() &&
> +        aNodeName.EqualsLiteral(KEY_PAIRING_LISTENER)) {

nit: align with line above.

@@ +271,5 @@
> +      }
> +      mPendingPairReqSignals.Clear();
> +    }
> +
> +    // For Map requests

nit: MAP requests

@@ +279,5 @@
> +      }
> +      mPendingMapSignals.Clear();
> +    }
> +
> +    // For Pbap request

nit: PBAP request's'

@@ +290,3 @@
>    }
> +
> +

Remove extra newlines.

@@ +364,5 @@
>    MOZ_ASSERT(NS_IsMainThread());
>  
>    BluetoothSignalObserverList* ol;
> +
> +  // If there is a corresponing signal path (key of hash table) in Observer

nit: correspon'd'ing

Also the comment is incorrect, it should be

  // Broadcast signal if corresponding observer list exists for the signal path

@@ +374,5 @@
>      return;
>    }
>  
> +  BT_LOGR("No observer registered for path %s",
> +      NS_ConvertUTF16toUTF8(aSignal.path()).get());

nit: 2-space indention.

@@ +382,5 @@
> +  // launch bluetooth certified app.
> +  if (aSignal.path().EqualsLiteral(KEY_PAIRING_LISTENER)) {
> +    mPendingPairReqSignals.AppendElement(aSignal);
> +    BT_ENSURE_TRUE_VOID_BROADCAST_SYSMSG(
> +        NS_LITERAL_STRING(SYS_MSG_BT_PAIRING_REQ),

nit: 2-space indention.

@@ +384,5 @@
> +    mPendingPairReqSignals.AppendElement(aSignal);
> +    BT_ENSURE_TRUE_VOID_BROADCAST_SYSMSG(
> +        NS_LITERAL_STRING(SYS_MSG_BT_PAIRING_REQ),
> +        BluetoothValue(EmptyString()));
> +  // If there is no signal path for KEY_MAP in observer table

Move the comment inside if (... KEY_MAP) condition. Also the comment seems unfinished: what to do if no KEY_MAP signal path?

@@ +389,5 @@
> +  } else if (aSignal.path().EqualsLiteral(KEY_MAP)) {
> +    mPendingMapSignals.AppendElement(aSignal);
> +    BT_ENSURE_TRUE_VOID_BROADCAST_SYSMSG(
> +        NS_LITERAL_STRING(SYS_MSG_BT_MAP_REQ),
> +        BluetoothValue(EmptyString()));

nit: 2-space indention.

@@ +390,5 @@
> +    mPendingMapSignals.AppendElement(aSignal);
> +    BT_ENSURE_TRUE_VOID_BROADCAST_SYSMSG(
> +        NS_LITERAL_STRING(SYS_MSG_BT_MAP_REQ),
> +        BluetoothValue(EmptyString()));
> +    BT_LOGR("Queue the request and sent system message to launch sms app");

Remove the log.

@@ +391,5 @@
> +    BT_ENSURE_TRUE_VOID_BROADCAST_SYSMSG(
> +        NS_LITERAL_STRING(SYS_MSG_BT_MAP_REQ),
> +        BluetoothValue(EmptyString()));
> +    BT_LOGR("Queue the request and sent system message to launch sms app");
> +  // If there is no signal path for KEY_PBAP in observer table

Move the comment inside if (... KEY_PBAP) condition. Also the comment seems unfinished: what to do if no KEY_PBAP signal path?

@@ +396,5 @@
> +  } else if (aSignal.path().EqualsLiteral(KEY_PBAP)) {
> +    mPendingPbapSignals.AppendElement(aSignal);
> +    BT_ENSURE_TRUE_VOID_BROADCAST_SYSMSG(
> +        NS_LITERAL_STRING(SYS_MSG_BT_PBAP_REQ),
> +        BluetoothValue(EmptyString()));

nit: 2-space indention.

@@ +397,5 @@
> +    mPendingPbapSignals.AppendElement(aSignal);
> +    BT_ENSURE_TRUE_VOID_BROADCAST_SYSMSG(
> +        NS_LITERAL_STRING(SYS_MSG_BT_PBAP_REQ),
> +        BluetoothValue(EmptyString()));
> +    BT_LOGR("Queue the request and sent system message to launch dialer app");

Remove the log.

::: dom/bluetooth/common/BluetoothService.h
@@ +727,5 @@
>    BluetoothSignalObserverTable mBluetoothSignalObserverTable;
>  
>    nsTArray<BluetoothSignal> mPendingPairReqSignals;
> +  nsTArray<BluetoothSignal> mPendingMapSignals;
> +  nsTArray<BluetoothSignal> mPendingPbapSignals;

Rename to |mPending*ReqSignals|.

::: dom/bluetooth/common/webapi/BluetoothAdapter.cpp
@@ +2023,5 @@
> +BluetoothAdapter::EventListenerAdded(nsIAtom* aType)
> +{
> +  DOMEventTargetHelper::EventListenerAdded(aType);
> +
> +  // Currently, we will not register signal handlre until all listener are set

Revise comment as following:

  // Do not register signal handler until all listeners are set

Comment 20

[Ben Tian (inactive)]

(In reply to Will Wang [:WillWang] from comment #18)
> OK, I am discussing the Gaia change with @Sean this afternoon, and will do
> it as you suggested.

Shawn, do you think we should land this mechanism right now? The reason I ask is, to land corresponding gaia change, we must convince gaia app owner/peer to r+ the change, which is useless right now as PBAP and MAP are incomplete. The corresponding gaia apps/system message flow may be subject to change before we complete PBAP and MAP gaia implementation.

IMO we have at least two options:
1) land neither gecko nor gaia change until PBAP/MAP gaia implementation is done
2) land general mechanism (bug 1229431) in gecko now, and land corresponding gaia change after PBAP/MAP gaia implementation is done

Let me know your thoughts.

Comment 21

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

(In reply to Ben Tian [:btian] from comment #20)
> (In reply to Will Wang [:WillWang] from comment #18)
> > OK, I am discussing the Gaia change with @Sean this afternoon, and will do
> > it as you suggested.
> 
> Shawn, do you think we should land this mechanism right now? The reason I
> ask is, to land corresponding gaia change, we must convince gaia app
> owner/peer to r+ the change, which is useless right now as PBAP and MAP are
> incomplete. The corresponding gaia apps/system message flow may be subject
> to change before we complete PBAP and MAP gaia implementation.
> 
> IMO we have at least two options:
> 1) land neither gecko nor gaia change until PBAP/MAP gaia implementation is
> done
> 2) land general mechanism (bug 1229431) in gecko now, and land corresponding
> gaia change after PBAP/MAP gaia implementation is done
> 
> Let me know your thoughts.

I would consider how we are going to implement bug1229431, and do we just enhance (or generalize) this solution based on this patch? Do we completely drop everything? If we're going to drop every piece of this patch, I believe landing this patch is not helpful.

If we're not going to land this patch based on the reason that it's not a general solution. I prefer option (2) since we call it "general" solution, that really doesn't matter to how gaia implementation is done or not. But somehow we shall ask gaia people if they are OK with this system-message invoking.

Comment 22

[Ben Tian (inactive)]

Resolve as WONTFIX since B2G is discontinued.