Closed Bug 1237493 Opened 6 years ago Closed 6 years ago

[NFC] Separate Gecko and Gonk layers for accessing Androids properties

Categories

(Firefox OS Graveyard :: NFC, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX
2.6 S9 - 3/11

People

(Reporter: tnguyen, Unassigned)

References

Details

User Story

As a developer, I want to avoid using |libcutils.property_get()| in the NFC module, so that Gecko can work more independently. (Hence, it provides the flexibility to deploy Gecko to work for various android-based Gonk versions, and easier to be ported to non-android platform, regarding NFC functionality).

Attachments

(1 file, 3 obsolete files)

NFC uses property_get to access Android properties:
- Start and stop nfcd daemon from gecko
- Access ro.moz.nfc.enabled property to enable/disable in runtime
Create this bug to track that
Blocks: 1230032
property_get has no relation with OTA.
No longer blocks: 1230032
Summary: [OTA][NFC] Separate Gecko and Gonk layers for accessing Androids properties → [NFC] Separate Gecko and Gonk layers for accessing Androids properties
Attached patch WIP start nfcd from HAL (obsolete) — Splinter Review
WIP : start nfcd from HAL
> - Access ro.moz.nfc.enabled property to enable/disable in runtime

There're 2 approaches for that:
1 - Move property_get("ro.moz.nfc.enabled", "false") to hal/gonk which contains such calls to Android APIs.
We will need a nice JS interface to access this hal/gonk function in order to detect if NFC is supported
2 - Using device-dependent preference. 
Have to add (for example, to shinano device) 
EXPORT_DEVICE_PREFS := $(LOCAL_PATH)/prefs
to device/sony/shinano-common/device.mk 
I would prefer to use #2 because we don't have to use low-level Android calls. 
If the shared preference is used, Foxfooders will require a FOTA to get NFC working again. I think we may have a transition phase (support both shared preference and Android property, then usage of Android property will be removed later) such as :
const NFC_ENABLED = Services.prefs.getBoolPref("nfc.enabled") || (libcutils.property_get("ro.moz.nfc.enabled", "false") === "true")
Hi

IIRC 'ro,moz.nfc.enabled' means that NFC is supported by the device. We could detect this by the presence of nfcd in the file system. The code to do this could live in the HAL module and provide an abstract interface, something like

  HAL::SystemServiceIsSupported("NFC") == true/false

HAL already has code for starting and stopping system services. The check would be a natural extension and other modules could adopt it easily. This would also not require a FOTA.

The problem with either option #1 or #2 is that it checks a setting without looking at the device's content. So the setting might say that NFC is available, but it actually isn't.
Thanks for your comment
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #4)
> Hi

> The problem with either option #1 or #2 is that it checks a setting without
> looking at the device's content. So the setting might say that NFC is
> available, but it actually isn't.

AFAIK, we expected have the same gecko (and also nfcd, I think) in different devices, it means NFC code still will be built even in un-supported NFC device. Then ro,moz.nfc.enabled will decide NFC feature could  be run. That's why we have both MOZ_NFC flag and ro,moz.nfc.enabled property to handle that (MOZ_NFC obviously would filter and not add NFC related code to image)
ro,moz.nfc.enabled property could enable/disable NFC at runtime in supported NFC device.
Hi

(In reply to Thomas Nguyen[:tnguyen][:thomas][:nguyen] from comment #6)
> ro,moz.nfc.enabled property could enable/disable NFC at runtime in supported
> NFC device.

'ro.' == readonly

I have two situations in mind.

 (1) We build an OTA for Gecko with NFC support and flash it onto a device without NFC. Gecko should now detect that it runs on a non-NFC device. The presence/absence of nfcd will tell this. Always having NFC support in Gecko is not a problem, and nfcd can be removed from devices that don't support NFC.

 (2) A user enables 'ro.moz.nfc.enabled' on a device without NFC. Gecko should probably log a warning that NFC is enabled but not supported by the base system.

In both cases the presence of nfcd signals whether the base system supports NFC. If nfcd is present, a user setting (such as ro.nfc.moz.enabled) could still override the auto-detection. And nfcd is easy to test for: !access("/system/bin/nfcd", F_OK)
Thank you

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #7)
> Hi
> 
> (In reply to Thomas Nguyen[:tnguyen][:thomas][:nguyen] from comment #6)
> > ro,moz.nfc.enabled property could enable/disable NFC at runtime in supported
> > NFC device.
> 
> 'ro.' == readonly

I think there's naming issue here, because I saw ro.moz.nfc.enabled is a solution of Bug 939056 

> 
> I have two situations in mind.
> 
>  (1) We build an OTA for Gecko with NFC support and flash it onto a device
> without NFC. Gecko should now detect that it runs on a non-NFC device. The
> presence/absence of nfcd will tell this. Always having NFC support in Gecko
> is not a problem, and nfcd can be removed from devices that don't support
> NFC.
> 
>  (2) A user enables 'ro.moz.nfc.enabled' on a device without NFC. Gecko
> should probably log a warning that NFC is enabled but not supported by the
> base system.


That's a good idea, but I am not sure if we have to do that. From basic user's perspective, NFC enable setting will not be showed in un-supported device. ro.nfc.moz.enabled setting is used for only advanced user or developer who could use command line to access
> In both cases the presence of nfcd signals whether the base system supports
> NFC. If nfcd is present, a user setting (such as ro.nfc.moz.enabled) could
> still override the auto-detection. And nfcd is easy to test for:
> !access("/system/bin/nfcd", F_OK)
Thank you, I think we could add it to Navigator.cpp this condition check
> > 'ro.' == readonly
> 
> I think there's naming issue here, because I saw ro.moz.nfc.enabled is a
> solution of Bug 939056 

I see.

> That's a good idea, but I am not sure if we have to do that. From basic
> user's perspective, NFC enable setting will not be showed in un-supported
> device. ro.nfc.moz.enabled setting is used for only advanced user or
> developer who could use command line to access

Right, that's the kind of user I have in mind. Printing a warning doesn't hurt.

From what I have in mind, the algorithm for detecting the presence of NFC (or any other driver) would look like this:

  bool test_nfc()
  {
    bool has_nfcd = !access(...)

    bool nfc_supported = get_property("nfc.enable", has_nfcd);

    if (!has_nfcd && nfc_supported) {
      log("NFC is not supported by this device")
    }

    return has_nfcd && nfc_supported;
  }
Attached patch Access Android properties patch (obsolete) — Splinter Review
Attachment #8704959 - Attachment is obsolete: true
Attachment #8708897 - Attachment description: 0001-Bug-1237493-NFC-Separate-Gecko-and-Gonk-layers-for-a.patch → Access Android properties patch
Attached file Device shinano pr (obsolete) —
Attachment #8708897 - Flags: feedback?(allstars.chh)
Attachment #8708900 - Flags: feedback?(allstars.chh)
Hi Yoshi,
Could you please take a look?
Thanks
Comment on attachment 8708897 [details] [diff] [review]
Access Android properties patch

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

1. Using pref to enable/disable NFC should be another bug, since it requires WebIDL change and is trying to solve different problem than this bug.
2. Pref is used to get/set properties inside Gecko, however the bug here is to get properties *outside* Gecko.
3. Compiler flag serves different purpose than Prefs, however you're mixing it, I don't know how build peers think of it, but this makes things worser to me.

::: dom/base/Navigator.cpp
@@ +2491,5 @@
>  bool
>  Navigator::HasNFCSupport(JSContext* /* unused */, JSObject* aGlobal)
>  {
> +  bool hasDomNfcEnabled = Preferences::GetBool("dom.nfc.enabled", false);
> +  bool hasNfcd = !access("/system/bin/nfcd", F_OK);

This is absolutely wrong,
the purose of removing property_get is to remove dependencies on Android, however the path you hardcode here, is still for Android only.

And from security perspective, I could easily cheat Gecko by doing
$echo > /system/bin/nfcd
on Systems that doesn't have NFC.
Attachment #8708897 - Flags: feedback?(allstars.chh) → feedback-
Hi Yoshi

> And from security perspective, I could easily cheat Gecko by doing
> $echo > /system/bin/nfcd
> on Systems that doesn't have NFC.

It was not meant for security. It's for Gecko to auto-detect if the service is available on the base system. Testing for nfcd is a good start. If nfcd isn't there, there's no point in exporting the DOM API. Using only preferences/properties/etc doesn't provide auto-detection.

I agree that it could be cheated easily, but I would not connect it to security. If an attacker can write to /system/bin, the security has already been compromised.

OTOH, I could imagine that an attacker tricks the system into starting NFC where nfcd isn't available; to exploit a bug in the start-up code. Testing for the presence of nfcd early would protect against this.
Thomas,

> ::: dom/base/Navigator.cpp
> @@ +2491,5 @@
> >  bool
> >  Navigator::HasNFCSupport(JSContext* /* unused */, JSObject* aGlobal)
> >  {
> > +  bool hasDomNfcEnabled = Preferences::GetBool("dom.nfc.enabled", false);
> > +  bool hasNfcd = !access("/system/bin/nfcd", F_OK);
> 
> This is absolutely wrong,
> the purose of removing property_get is to remove dependencies on Android,
> however the path you hardcode here, is still for Android only.

A possible location for this test would be hal/gonk/SystemService.cpp.
(In reply to Yoshi Huang[:allstars.chh] from comment #14)
> Comment on attachment 8708897 [details] [diff] [review]
> Access Android properties patch
> 
> Review of attachment 8708897 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 2. Pref is used to get/set properties inside Gecko, however the bug here is
> to get properties *outside* Gecko.


Looking at Bug 945630, I see there's a way that we load a device-dependent preference settings. The device-dependent preference should be outside of gecko.
I just keep the default value (false) in all.js and override with a device specific file - true(in device/sony/shinano-common/prefs/nfc.js).
We have to add the file path to device.mk 
EXPORT_DEVICE_PREFS := $(LOCAL_PATH)/prefs
 

> 3. Compiler flag serves different purpose than Prefs, however you're mixing
> it, I don't know how build peers think of it, but this makes things worser
> to me.
Are you talking to change in all.js?
+#ifdef MOZ_NFC
+pref("dom.nfc.enabled", false);
+#endif

It's only default value of the preference. The value will be overrided in device/sony/shinano-common/prefs/nfc.js. We only need to care about this preference if MOZ_NFC exist.
(In reply to Thomas Nguyen[:tnguyen][:thomas][:nguyen] from comment #17)
> 
> Looking at Bug 945630, I see there's a way that we load a device-dependent
> preference settings. The device-dependent preference should be outside of
> gecko.
>
Don't you still see the problem?
How come a pref called dom.nfc.* could be a gonk-specific thing?

If the naming starts with *dom*, it should be configured inside Gecko and Gecko only.
  
> 
> > 3. Compiler flag serves different purpose than Prefs, however you're mixing
> > it, I don't know how build peers think of it, but this makes things worser
> > to me.
> Are you talking to change in all.js?
> +#ifdef MOZ_NFC
> +pref("dom.nfc.enabled", false);
> +#endif
> 
NO

> It's only default value of the preference. The value will be overrided in
> device/sony/shinano-common/prefs/nfc.js. We only need to care about this
> preference if MOZ_NFC exist.
Thanks, but I think I know this.
And in fact I did reviewed your patch, so next time tell me something I don't know.

PLEASE THINK THE DIFFERENCES between
1. #ifdef MOZ_NFC
2. Prefs
3. property_get

If you have some other idea like using HalService, is fine, too.

They all can be used to do what you want here, but they serve with different purposes.
PLEASE LOOK BACK TO YOUR ORIGINAL PROBLEM AND CHOOSE WHAT IS THE BEST AND WHAT MAKES SENSE MOST!

From my understanding, the statement you made from Comment 0 is to remove using libcutils.property_get, which is an Android interface,but you did *NOT* mention that we should stop getting properties from Gonk.
But your patch are using pref directly, which is not doing the same thing as you described.

So please think your problem and solution, and make sure you describe them clearly.
I have been reviewed your patches for a while but the sitations are mostly the same.
Your code could work, but it's not the best answer.

Image that I ask you to fix my laptop, but in the end you re-decorate my whole house!
Your approach is over-kill, and most of them have introduced different problems than we try to solve in the first place.

Thanks
Yoshi, take it easy! ;)
Yoshi, thanks you
I apologize for causing you frustration by continuing to ask what you've already mentioned.
Let me look back and see what would be the best solution in this case.
I appreciate your feedback, sharing and thanks again.
Update the user story.

Hi Thomas Nguyen,
Please discuss your solution with Yoshi in detail, and update it here once you have a consensus.
Thanks.
Assignee: nobody → tnguyen
Status: NEW → ASSIGNED
User Story: (updated)
Target Milestone: --- → 2.6 S9 - 3/11
Attachment #8708900 - Attachment is obsolete: true
Attachment #8708897 - Attachment is obsolete: true
I had a discussion with Yoshi and decided only to remove property_get using in start and stop nfcd daemon.
The purpose of removing property_get/set in js and having the property interfaces in hal/ is to provide generic interfaces in different platforms, however that is still uncertain.
We are keep using
https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/systemlibs.js?case=true&from=systemlibs.js
Thanks
Attachment #8712020 - Flags: review?(allstars.chh)
Attachment #8712020 - Flags: review?(allstars.chh) → review+
Assignee: tnguyen → nobody
Status: ASSIGNED → NEW
Keywords: checkin-needed
Thomas, I am confused.
If you are going to land this patch, it doesn't make sense to reset the assignee.
Why did you do these two actions at the same time?
Do we really want to land this patch now?
Flags: needinfo?(tnguyen)
This only removes property_get using in start and stop nfcd daemon and I think we should land the patch.
Sorry to make you confused, due to new plan, I just tried to filter my bug list. I reset the assignee, but feel free ni or reassign to me if required.
Thanks
Flags: needinfo?(tnguyen)
(In reply to Thomas Nguyen[:tnguyen][:thomas][:nguyen] from comment #25)
> This only removes property_get using in start and stop nfcd daemon and I
> think we should land the patch.
> Sorry to make you confused, due to new plan, I just tried to filter my bug
> list. I reset the assignee, but feel free ni or reassign to me if required.
> Thanks
Never mind.
Since you are going to land this bug, you shouldn't reset the assignee.

Let's land it.
Assignee: nobody → tnguyen
Status: NEW → ASSIGNED
Assignee: tnguyen → nobody
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.