1107539 - Move Geolocation HAL into system daemon on FxOS

Status: RESOLVED WONTFIX

(Core :: DOM: Geolocation, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

QC provides their own implementation for the Geolocation service on FxOS. Constant changes within libxul.so require porting efforts and prevent existing devices from being updated.

A possible solution for these problems is moving the Geolocation code on FxOS to an external daemon that speaks a semi-stable* IPC protocol. Gecko will only implement some IPC stubs.

As a result, QC devs will be able to replace the daemon with their own implementation; Mozilla devs will be able to update devices' Gecko binary without breaking Geolocation support.

This bug is for investigating and discussing the approach.

* Geolocation's API is quite small and stable. We don't expect major changes.

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

I volunteered to provide a proof-of-concept implementation within the next months.

Comment 2

[Dave Huseby [:huseby]]

have you made any headway on this proof-of-concept?

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Not yet, sorry. I've been busy with Bluetooth recently. I'll try to make progress during Q2.

I recently implemented a daemon for Android's Bluetooth driver. My plan is to take its IPC code and replace the Bluetooth operations with Geolocation. Should be straight-forward.

Comment 4

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi,

I just read through bug 1097229. I'm wondering if we can join forces somehow?

For the Bluetooth daemon, I first wrote a set of interface classes [1] that can be implemented by any driver. In case you're familiar with Bluedroid, you'll recognize the individual methods. Then I added an implementation that calls the Android driver directly. [2] And finally I added support for the daemon. [3] The IPC protocol is defined by BlueZ. [4]

We run the code for the daemon's IPC on the I/O thread. There are several classes for communicating between main thread and I/O thread in [5]. The packing, unpacking and type conversion for the IPC messages is generic [6] and generated by the C++ compiler at build time.

For each version of Android, we pick a fix revision of the IPC protocol. Differences between Android versions are currently handled by ifdef in Gecko's backend code. Gecko's core Bluetooth code is version-agnostic. Some Bluedroid drivers contain vendor extensions. We can work around them in the daemon. In the case of an update of Gecko, the daemon would not be affected. And Bluetooth continues to work because the IPC protocol didn't change.

A similar strategy could be taken for Geolocation. The IPC protocol can be defined by replacing Bluetooth primitives with Geolocation primitives. Most of Bluetooth's IPC code is generic. It can be used for Geolocation as well, as long it's not specific to Bluetooth types.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/bluetooth/BluetoothInterface.h#542
[2] https://mxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothHALInterface.h#15
[3] https://mxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothDaemonInterface.h#22
[4] https://git.kernel.org/cgit/bluetooth/bluez.git/tree/android/hal-ipc-api.txt
[5] https://mxr.mozilla.org/mozilla-central/source/dom/bluetooth/BluetoothInterfaceHelpers.h
[6] https://mxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothDaemonHelpers.h

Comment 5

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Dave

(In reply to Dave Huseby [:huseby] from comment #2)
> have you made any headway on this proof-of-concept?

I pushed some (untested) code to [1]. It contains support for basic GPS functionality. I'll more features over the next weeks.

[1] https://github.com/tdz/platform_system_geolocationd

Comment 6

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: https://github.com/tdz/platform_system_geolocationd

Comment 7

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8585534 [details]
https://github.com/tdz/platform_system_geolocationd

Updated Github repository:

  - added AGPS module
  - some refactoring

Comment 8

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8585534 [details]
https://github.com/tdz/platform_system_geolocationd

Updated Github repository

  - added AGPS-RIL module
  - minor fixes

Comment 9

[:garvan]

Will the location providers be affected? Concretely, will GonkGPSGeolocationProvider or NetworkGeolocationProvider be affected by this change? If so, this may affect future work that MLS has planned (not in a bad way, just that we should know to follow this work closely).

Comment 10

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Garvan,

Yes, I'm currently rewriting |GonkGPSGeolocationProvider| to work with the daemon. I hope to have the patches ready soon. The change is that |GonkGPSGeolocationProvider| will call the daemon instead of the HAL API. But the overall logic in that class will remain the same.

Comment 11

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Adding :scravag because he was interested in the topic.

Comment 12

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] WIP: Added helpers

Comment 13

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] WIP: Added Registry module

Comment 14

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] WIP: Added GPS module

Comment 15

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] WIP: Added AGPS module

Comment 16

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [05] WIP: Added AGPS-RIL module

Comment 17

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [06] WIP: Added Geolocation protocol class

Comment 18

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [07] WIP: Added GPS interface

Comment 19

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [08] WIP: Added AGPS interface

Comment 20

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [09] WIP: Added AGPS-RIL interface

Comment 21

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [10] WIP: Use Geolocation daemon in |GonkGPSGeolocationProvider|

Comment 22

[Thomas Zimmermann [:tzimmermann] [:tdz]]

The WIP patch set is mostly complete, but still needs a lot of testing and debugging.

Comment 23

[Bhavna Sharma]

For which release is this change coming up ?

Comment 24

[Thomas Zimmermann [:tzimmermann] [:tdz]]

None yet, it's just a prototype.

Comment 25

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] WIP: Added helpers

Comment 26

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] WIP: Added Registry module

Comment 27

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] WIP: Added GPS module

Comment 28

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] WIP: Added AGPS module

Comment 29

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [05] WIP: Added AGPS-RIL module

Comment 30

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [06] WIP: Added Geolocation protocol class

Comment 31

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [07] WIP: Added GPS interface

Comment 32

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [08] WIP: Added AGPS interface

Comment 33

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [09] WIP: Added AGPS-RIL interface

Comment 34

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [10] WIP: Use Geolocation daemon in |GonkGPSGeolocationProvider|

Comment 35

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8585534 [details]
https://github.com/tdz/platform_system_geolocationd

Updated Github tree:

  - documented IPC protocol
  - many code fixes

Comment 36

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8585534 [details]
https://github.com/tdz/platform_system_geolocationd

Updated Github repository

  - fixed nmea notification

Comment 37

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] WIP: Added Registry module

Comment 38

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] WIP: Added GPS module

Comment 39

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] WIP: Added AGPS module

Comment 40

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [05] WIP: Added AGPS-RIL module

Comment 41

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [10] WIP: Use Geolocation daemon in |GonkGPSGeolocationProvider|

Comment 42

[Doug Turner (:dougt)]

Overall, I am happy to see someone think about this problem.  Not being able to update FirefoxOS is upsetting.  As for the set of changes, these patches do a lot more than switch over the geolocation provider to use something else!  I will not be able to review in the near term, however I think Kanru would be the best person to review the FFOS stuff.

Have you spoken to mvines (or anyone at QC) about this and will they move this this new framework?  Without commitment, I am not sure that this work is that important.

Another, simpler, approach would be to build a API/ABI that can be frozen between our location provider and whatever they need.  I suspect they would have to drive this effort as only they know what their provider does.

Comment 43

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi,

Thanks for taking a look at the current patch set.

This bug report actually resulted from a discussion with mvines in Portland. I didn't change any of the internal interfaces within Gecko. So if QC is not interested, nothing would change for them.

For our implementation, the patch set improves the code's security and reliability, and reduces Gecko's hardware dependencies. These seem worthwhile goals for B2G's overall architecture.

Comment 44

[Dave Huseby [:huseby]]

Comment on attachment 8592218 [details] [diff] [review]
[01] WIP: Added helpers

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

The Convert functions in the GeolocationHelpers.cpp file should probably be moved to a header file so you can rewrite it as an inline function template and then specialize where needed.  There's lots of implementations where the only difference is the type of the first parameter.  That's a sign that a template can reduce the LOC.

Comment 45

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Github pull request for platform_system_geolocationd

Comment 46

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1107539: Remove |GonkGPSGeolocationProvider|

Comment 47

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1107539: Add registry module for Gonk Geolocation daemon

Comment 48

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 1107539: Add GPS module for Gonk Geolocation daemon

Comment 49

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] Bug 1107539: Add AGPS module for Gonk Geolocation daemon

Comment 50

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [05] Bug 1107539: Add AGPS-RIL module for Gonk Geolocation daemon

Comment 51

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [06] Bug 1107539: Add Gonk Geolocation daemon protocol

Comment 52

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [07] Bug 1107539: Use Gonk Geolocation daemon

Comment 53

[Kan-Ru Chen [:kanru] (UTC+9)]

I think if we could remove some hardware dependency then it's great! However,

I skimmed through the patches and had some questions noted:

* Why is this more secure? Could you elaborate?
* What's the benefit if no more chipset vendor needs it?
* Could the protocol handling code be shared with other part of HAL?
  Why are we writing these new protocol handling code instead of reusing other serializer we already have in code base like IPDL or protobuf? If it's using json then the provider could potentially be implemented in js.
* GonkGeolocationProvider.cpp looks very similar to GonkGPSGeolocationProvider.cpp, it will be easier to review it as a patch instead of a complete rewrite.
* Looks like most of the controlling logic is still in gecko and it highly depends on the behavior of the hardware library. I don't think this improves the porting or update issue very much.

Some nits:

* nsAutoPtr should be UniquePtr
* use LazyLogModule instead of __android_log_print in gecko

Comment 54

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi

Thanks for looking at the patches.

(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #53)
> I think if we could remove some hardware dependency then it's great! However,
> 
> I skimmed through the patches and had some questions noted:
> 
> * Why is this more secure? Could you elaborate?

Besides the address-space separation, the daemon can run without root privileges and with a custom SELinux context. We already do this for Bluetooth and Sensors.


> * What's the benefit if no more chipset vendor needs it?

Security, as mentioned above. The system also becomes more reliable. The driver can crash without taking down the entire system. Gecko would see a disconnect of the IPC socket, and can clean up it's internal resources. In Bluetooth, we even restart the driver after a crash and continue operation. Implementing this in Geolocation should be considerably easier, as it has almost no state (compared to BT).


> * Could the protocol handling code be shared with other part of HAL?
>   Why are we writing these new protocol handling code instead of reusing
> other serializer we already have in code base like IPDL or protobuf? If it's
> using json then the provider could potentially be implemented in js.

It's already shared with other HAL components. Most of the IPC and protocol code is located in ipc/hal. It was originally merged for Bluetooth, which has to be compatible with BlueZ's IPC. (So the code is there anyway.) It was then generalized and re-used for Sensors, and now for Geolocation. What is added here are only a few additional helper functions for this IPC framework.


> * GonkGeolocationProvider.cpp looks very similar to
> GonkGPSGeolocationProvider.cpp, it will be easier to review it as a patch
> instead of a complete rewrite.

That's how where the code started. Trust me, it was not more readable. ;)

If you insist, I'll merge it into GonkGpsGeolocationProvider.cpp, but it's still a complete rewrite.


> * Looks like most of the controlling logic is still in gecko and it highly
> depends on the behavior of the hardware library. I don't think this improves
> the porting or update issue very much.

It's not intended to ease porting to something else than HAL drivers.

For different version of the HAL interface, the protocol can be adapted. The daemon implements a specific version and Gecko can handle different versions of the protocol. This is intentional! We use this design for Bluetooth, where we support all versions of the HAL driver up to L with a single implementation of Gecko.

The daemon is supposed to be dumb. It's highly system dependent and hard to update (FOTA). Gecko fixes are a lot easier to distribute. Again, we made a good experience with this design in Bluetooth.


> Some nits:
> 
> * nsAutoPtr should be UniquePtr
> * use LazyLogModule instead of __android_log_print in gecko

Sure.

Comment 55

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1107539: Add registry module for Gonk Geolocation daemon (v2)

Changes since v1:

  - use |UniquePtr<>|
  - changed logging to use |LogModule|

Comment 56

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 1107539: Add GPS module for Gonk Geolocation daemon (v2)

Changes since v1:

  - use |UniquePtr|
  - update logging

Comment 57

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] Bug 1107539: Add AGPS module for Gonk Geolocation daemon (v2)

Changes since v1:

  - use |UniquePtr<>|
  - update logging

Comment 58

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [05] Bug 1107539: Add AGPS-RIL module for Gonk Geolocation daemon (v2)

Changes since v1:

  - use |UniquePtr<>|
  - update logging

Comment 59

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [06] Bug 1107539: Add Gonk Geolocation daemon protocol (v2)

Changes since v1:

  - update logging

Comment 60

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [07] Bug 1107539: Use Gonk Geolocation daemon (v2)

Changes since v1:

  - update logging

Comment 61

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1107539: Remove |GonkGPSGeolocationProvider| (v2)

Changes since v1:

  - rebased onto bug 1253159

Comment 62

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [07] Bug 1107539: Use Gonk Geolocation daemon (v3)

Changes since v2:

  - rebased onto bug 1253159

Comment 63

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Github pull request for device-flame (kitkat)

Comment 64

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Github pull request for gonk-misc

Comment 65

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Github pull request for b2g-manifest

Comment 66

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Github pull request for device-flame

Comment 67

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [07b] Bug 1107539: Use Gonk Geolocation daemon

Kan-Ru, Patch [07b] is an alternative to [07]. [07b] implements the new daemon provider on top of the old one; as you asked for in the feedback.

Comment 68

[Kan-Ru Chen [:kanru] (UTC+9)]

I'll try to review this by Friday.

Comment 69

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8726653 [details] [diff] [review]
[02] Bug 1107539: Add registry module for Gonk Geolocation daemon (v2)

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

::: dom/system/gonk/GonkGeolocationInterface.cpp
@@ +37,5 @@
> +    OPCODE_UNREGISTER_MODULE = 0x02
> +  };
> +
> +  virtual nsresult Send(DaemonSocketPDU* aPDU,
> +                        DaemonSocketResultHandler* aUserData) = 0;

I guess this function will be defined in latter patch? And note the return type could be a bool.

@@ +48,5 @@
> +                    GeolocationRegistryResultHandler* aRes)
> +  {
> +    UniquePtr<DaemonSocketPDU> pdu =
> +      MakeUnique<DaemonSocketPDU>(SERVICE_ID, OPCODE_REGISTER_MODULE,
> +                                  1); // Service id

Maybe use a self-explanatory name like pduServiceId instead of a comment like this.

nit: weird line breaking.

@@ +50,5 @@
> +    UniquePtr<DaemonSocketPDU> pdu =
> +      MakeUnique<DaemonSocketPDU>(SERVICE_ID, OPCODE_REGISTER_MODULE,
> +                                  1); // Service id
> +
> +    nsresult rv = PackPDU(aServiceId, *pdu);

I guess the interface of PackPDU was introduced by other patches but note that out parameters should be pointers instead of references.

@@ +58,5 @@
> +    rv = Send(pdu.get(), aRes);
> +    if (NS_FAILED(rv)) {
> +      return rv;
> +    }
> +    Unused << pdu.release();

Do you really mean to intentionally leak the pointer?

@@ +59,5 @@
> +    if (NS_FAILED(rv)) {
> +      return rv;
> +    }
> +    Unused << pdu.release();
> +    return rv;

Instead of return rv here, explicitly return NS_OK;

@@ +68,5 @@
> +                      GeolocationRegistryResultHandler* aRes)
> +  {
> +    UniquePtr<DaemonSocketPDU> pdu =
> +      MakeUnique<DaemonSocketPDU>(SERVICE_ID, OPCODE_UNREGISTER_MODULE,
> +                                  1); // Service id

ditto

@@ +70,5 @@
> +    UniquePtr<DaemonSocketPDU> pdu =
> +      MakeUnique<DaemonSocketPDU>(SERVICE_ID, OPCODE_UNREGISTER_MODULE,
> +                                  1); // Service id
> +
> +    nsresult rv = PackPDU(aServiceId, *pdu);

ditto

@@ +78,5 @@
> +    rv = Send(pdu.get(), aRes);
> +    if (NS_FAILED(rv)) {
> +      return rv;
> +    }
> +    Unused << pdu.release();

ditto

@@ +79,5 @@
> +    if (NS_FAILED(rv)) {
> +      return rv;
> +    }
> +    Unused << pdu.release();
> +    return rv;

ditto

::: dom/system/gonk/GonkGeolocationInterface.h
@@ +66,5 @@
> +                     GeolocationError aError);
> +  void DispatchError(GeolocationRegistryResultHandler* aRes,
> +                     nsresult aRv);
> +
> +  GeolocationRegistryModule* mModule;

UniquePtr<GeolocationRegistryModule>

Comment 70

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi!

(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #69)
> Comment on attachment 8726653 [details] [diff] [review]
> [02] Bug 1107539: Add registry module for Gonk Geolocation daemon (v2)
> 
> Review of attachment 8726653 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/GonkGeolocationInterface.cpp
> @@ +37,5 @@
> > +    OPCODE_UNREGISTER_MODULE = 0x02
> > +  };
> > +
> > +  virtual nsresult Send(DaemonSocketPDU* aPDU,
> > +                        DaemonSocketResultHandler* aUserData) = 0;
> 
> I guess this function will be defined in latter patch? And note the return
> type could be a bool.

Yep, that's patch [06]. Each module class uses |Send| for sending PDUs to the daemon. Since all modules share the same connection, |Send| is implemented in a class that inherits form all module classes. That class also maintains the connection state.

> @@ +48,5 @@
> > +                    GeolocationRegistryResultHandler* aRes)
> > +  {
> > +    UniquePtr<DaemonSocketPDU> pdu =
> > +      MakeUnique<DaemonSocketPDU>(SERVICE_ID, OPCODE_REGISTER_MODULE,
> > +                                  1); // Service id
> 
> Maybe use a self-explanatory name like pduServiceId instead of a comment
> like this.

Not sure if that's worth it. These values are the size of the PDU's arguments. But it's just a hint to reduct the number of calls to the allocator. If the overall sum is incorrect, the PDU class will allocate more memory as required.

> nit: weird line breaking.

What do you suggest instead?

> @@ +50,5 @@
> > +    UniquePtr<DaemonSocketPDU> pdu =
> > +      MakeUnique<DaemonSocketPDU>(SERVICE_ID, OPCODE_REGISTER_MODULE,
> > +                                  1); // Service id
> > +
> > +    nsresult rv = PackPDU(aServiceId, *pdu);
> 
> I guess the interface of PackPDU was introduced by other patches but note
> that out parameters should be pointers instead of references.

True. I would not want to change this interface, because it is also used a lot by HAL and Bluetooth.

> @@ +58,5 @@
> > +    rv = Send(pdu.get(), aRes);
> > +    if (NS_FAILED(rv)) {
> > +      return rv;
> > +    }
> > +    Unused << pdu.release();
> 
> Do you really mean to intentionally leak the pointer?

Oh, that surely deserves a comment in the code. The PDU (here and in similar places) is handed over to the I/O thread, which then maintains the reference.

> @@ +59,5 @@
> > +    if (NS_FAILED(rv)) {
> > +      return rv;
> > +    }
> > +    Unused << pdu.release();
> > +    return rv;
> 
> Instead of return rv here, explicitly return NS_OK;

Ok.

Comment 71

[Doug Turner (:dougt)]

I'd like to review a patch if this work is picked up again.  For now, clearing review flag.