939056 - B2G NFC: enable/disable MOZ_NFC at runtime

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

I found this from Bug 939052,
now MOZ_NFC is enabled by default when building B2G, even on the platform NFC isn't supported, i.e. emulator, Unagi, ....

Should we only enable MOZ_NFC only for some specific device, like Nexus-4 ?
Or at least we should enable MOZ_NFC for Nexus-4 *for now*, so other devs won't suffer from a feature which is not supported on his platform.

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

ni? for gps,

Hi gps, can you kindly provide some suggestion here?

Thank you :)

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Well what are we doing with RIL?

Comment 3

[Garner Lee]

How far does .tmp-config --> .config go at the top B2G.git/config.sh file?

One such flag is "MAKE_FLAGS=j10" as in 10 jobs.

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #2)
> Well what are we doing with RIL?

In gonk-misc/Android.mk [1], we have:

  export CONFIGURE_ARGS="$(GECKO_CONFIGURE_ARGS)" && \

So basically set GECKO_CONFIGURE_ARGS in any Android.mk or, more preferable, per device BoardConfig.mk will do the job.

Besides, considering that NFC is not supported on most of the Firefox OS devices by now, maybe that should be disabled by default in gecko/configure.in.

However, there is also opinions about elimination of these configure arguments, and a unified Gecko binary for all Firefox OS devices.

[1]: https://github.com/mozilla-b2g/gonk-misc/blob/master/Android.mk#L281

Comment 5

[Gregory Szorc [:gps]]

MOZ_NFC requires --enable-nfc. You'll need to set up your mozconfigs (or whatever your build system uses) to only pass --enable-nfc if the build you are producing wants NFC support.

Comment 6

[Garner Lee]

The flag --enable-nfc used to be in gonk-misc/default-gecko-config. We'd have to fork it though.

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

request for 1.3+ for now all B2G devices (emulator, unagi, ...) will enable NFC by default. We need to turn it on only when NFC is supported by platform.

Comment 9

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Garner Lee from comment #6)
> The flag --enable-nfc used to be in gonk-misc/default-gecko-config. We'd
> have to fork it though.

You don't.  See comment 4.

Comment 10

[Garner Lee]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #9)

> You don't.  See comment 4.

Ah. Something like this?

BoardConfig.mk:
TARGET_GLOBAL_CFLAGS += -mfpu=neon -mfloat-abi=softfp -DMOZ_NFC=1

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Garner Lee from comment #10)
> Ah. Something like this?
> BoardConfig.mk:
> TARGET_GLOBAL_CFLAGS += -mfpu=neon -mfloat-abi=softfp -DMOZ_NFC=1

No.  Add following line into BoardConfig.mk:

  GECKO_CONFIGURE_ARGS := --enable-nfc

Comment 12

[Garner Lee]

Attachment: gecko/configure.in: Remove MOZ_NFC=1 from android 15, 17, 18

Comment 13

[Garner Lee]

Attachment: Mako specific --enable-nfc flag setting.

device/lge/mako repo, add GECKO_CONFIGURE_ARGS to BoardConfig.mk.

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8334981 [details] [diff] [review]
gecko/configure.in: Remove MOZ_NFC=1 from android 15, 17, 18

Although it looks reasonable, but I'm not one of the Build peers.  This patch doesn't contain "--enable-nfc in device specific BoardConfig.mk file" part, so I would suggest you to remove it.

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8334985 [details] [diff] [review]
Mako specific --enable-nfc flag setting.

I don't know where to apply this patch, and mwu owns Gonk bits.

Comment 16

[Michael Wu [:mwu]]

Comment on attachment 8334981 [details] [diff] [review]
gecko/configure.in: Remove MOZ_NFC=1 from android 15, 17, 18

We do not support per device geckos.

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Michael Wu [:mwu] from comment #16)
> We do not support per device geckos.

Then we are not supposed to have GECKO_CONFIGURE_ARGS in gonk-misc/Android.mk, isn't it?  Please give alternatives to distinguish "platform has no NFC" & "platform has NFC but dead now".

Comment 18

[Garner Lee]

I'll throw some ideas out for discussion, if it hasn't already been had many times over:

On IRC, one suggestion was:

Exponential backoff, was a max retry or time limit, so we don't end up with a bunch of events in the log trying to connect with a non-existent NFC daemon. The check isn't CPU intensive at all, but the timer does wake the device CPU(s) up. Or as Kyle asked, what does RIL do? For example, a future FirefoxOS tablet won't necessarily have have a cell radio.

Some kind of configurable system variable that isn't in gecko indicating presence of NFC (one gecko build config to rule them all, except for really platform specific stuff like cameras maybe).

Comment 19

[Michael Wu [:mwu]]

Yeah this is exactly the same problem as RIL on tablets without 3G/data. We should find solutions for both.

I think we should actually do both backoff with max retry plus some sort of system property. Trying forever to reconnect is only going to burn power unnecessarily, so it seems like a good idea to limit reconnection attempts regardless. However, it's also good to be explicit about whether one should expect RIL or NFC.

For reference, it looks like android maintains a list of features in /system/etc/permissions . We don't use that, so the next closest thing is probably system properties.

Comment 21

[Marco Chen [:mchen]]

(In reply to Michael Wu [:mwu] from comment #16)
> Comment on attachment 8334981 [details] [diff] [review]
> gecko/configure.in: Remove MOZ_NFC=1 from android 15, 17, 18
> 
> We do not support per device geckos.

1. As I know that some works in Gecko will refer to preference for doing different jobs.
ex: low memory killer attributes, cell broadcast, mute during voice call from Audio HAL or RIL (planning)
2. Different Gonk-Versions also made gecko can't be compatible to different devices.

(So basically the current Gecko already be a device based object.)

3. Also consider to more low memory and storage device, we need to configure each system attribute and remove unused modules for reducing spaces. 
4. Expect more device types will join FxOS (ex: TV, setup box, POS), I think Gecko should be customized for each type by different configuration and modules combination. 

I suggest Gecko can be customized then it can't be compatible to each devices.

Comment 22

[Vicamo Yang [:vicamo][:vyang]]

The thing I don't really understand is, why Mozilla disallows per device Geckos?  Neither does Mozilla manufacture any device, nor deliver updates to any one of them.  Why different Gecko binaries is ever a concern to Mozilla?  As a device maker, can't I re-compile Gecko with RealView if I want to?  Debian has IceWeasel.  Does it somehow breaks per-distribution-Geckos' rule?

Comment 23

[Garner Lee]

Any decisions on what to do yet given 1.3 cutoff is/was approaching?

Comment 24

[Ken Chang[:kenkai]]

Dimi, please keep handling this. Thanks.

Comment 25

[Dimi Lee [:dimi]]

Attachment: [WIP]Gecko: Propose solution for runtime enable/disable NFC

Comment 26

[Dimi Lee [:dimi]]

Attachment: [WIP]device: Propose solution for runtime enable/disable NFC

Comment 27

[Dimi Lee [:dimi]]

Comment on attachment 8340985 [details] [diff] [review]
[WIP]Gecko: Propose solution for runtime enable/disable NFC

Attachment is proposed solution if we are going to runtime enable/disable NFC
1. Use system property "ro.nfc.enabled" to check if nfc should be enabled or not. Configured in device/lge/mako/init.mako.rc
2. Check system property for mozNfc webidl
3. Check system property for Nfc.js & NfcContentHelper.js (not include in the patch)

Comment 28

[Garner Lee]

Some comments on WIP:
SettingsUI currently checks for the existance of navigator.mozNfc to decide to show NFC or not.

Based on the "Per device Gecko configure options" discussion posted on mozilla-dev-b2g, should the mozNfc object get created at all (gecko/dom/nfc/nsNfc.js)? If not, we need a chrome side check (somehow) for "ro.nfc.enabled" there as well to not return an object. If it still needs to be there, the DOM functions need to be no-ops.

Comment 29

[Michael Wu [:mwu]]

Comment on attachment 8340988 [details] [diff] [review]
[WIP]device: Propose solution for runtime enable/disable NFC

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

The recommended way of adding properties is to add them to the PRODUCT_PROPERTY_OVERRIDES in device files like https://github.com/mozilla-b2g/device-mako/blob/b2g-4.3_r2.1/device.mk . Some devices also have a system.prop file you can edit, which is even better.

Comment 30

[Michael Wu [:mwu]]

Comment on attachment 8340985 [details] [diff] [review]
[WIP]Gecko: Propose solution for runtime enable/disable NFC

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

Seems reasonable though I don't know this code very well. However, you might have to pay attention to making sure that the check for nfc is done on the parent process side - I don't think we're allowed to rely on property_get in the child process.

::: dom/base/Navigator.cpp
@@ +1807,5 @@
>  {
>    nsCOMPtr<nsPIDOMWindow> win = GetWindowFromGlobal(aGlobal);
> +
> +  char value[PROPERTY_VALUE_MAX];
> +  property_get("ro.nfc.enabled", value, NULL);

"0" might be a better default value than NULL.

Comment 31

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8340985 [details] [diff] [review]
[WIP]Gecko: Propose solution for runtime enable/disable NFC

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

::: dom/base/Navigator.cpp
@@ +1807,5 @@
>  {
>    nsCOMPtr<nsPIDOMWindow> win = GetWindowFromGlobal(aGlobal);
> +
> +  char value[PROPERTY_VALUE_MAX];
> +  property_get("ro.nfc.enabled", value, NULL);

If this is a new Mozilla-only property, it should be "ro.moz.nfc.enabled" just like all other "ro.moz.ril.*".

Comment 32

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8340985 [details] [diff] [review]
[WIP]Gecko: Propose solution for runtime enable/disable NFC

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

Should we also need to check is Nfc enable in ipc/nfc/Nfc.cpp?
So the socket won't keep reconnecting nfcd all the time.

Comment 33

[[:fabrice] Fabrice Desré]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #32)
 
> Should we also need to check is Nfc enable in ipc/nfc/Nfc.cpp?
> So the socket won't keep reconnecting nfcd all the time.

Absolutely.

Comment 34

[Garner Lee]

^-- Maybe add something like "isEnabledOnPlatform()" interface as a pure virtual to UnixSocketConsumer so RIL is forced to get this check too?

Comment 35

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Fabrice Desré [:fabrice] from comment #33)
> (In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #32)
>  
> > Should we also need to check is Nfc enable in ipc/nfc/Nfc.cpp?
> > So the socket won't keep reconnecting nfcd all the time.
> 
> Absolutely.

Just discussed with Dimi, 
if Nfc.js won't call SystemWorkerManager.registerNfcWorker, then the socket won't be established.

Comment 36

[Dimi Lee [:dimi]]

There are two part to fix this issue:
1. Show/Hide mozNfc webidl
2. Enable/Disable NFC worker

Create bug 947100 for Enable/Disable NFC worker

Comment 37

[Dimi Lee [:dimi]]

Attachment: [WIP]Gecko: Propose solution for runtime enable/disable NFC v2

Comment 38

[Dimi Lee [:dimi]]

Attachment: [WIP]Gecko: Propose solution for runtime enable/disable NFC v2

Comment 39

[Dimi Lee [:dimi]]

Comment on attachment 8344547 [details] [diff] [review]
[WIP]Gecko: Propose solution for runtime enable/disable NFC v2

This patch is used to show/hide mozNfc at runtime.
The idea is send a dummy message to NFC module. If NFC module exist then the return value of SendSyncMessage will be a non-zero array. NFC module enable/disable is implement in bug 947100
Could you help provide some suggestions ? Thanks !

Comment 40

[Dimi Lee [:dimi]]

Attachment: Enable/Disable mozNfc at runtime

Comment 41

[Dimi Lee [:dimi]]

Attachment: Enable/Disable mozNfc at runtime

Comment 42

[Dimi Lee [:dimi]]

Comment on attachment 8345215 [details] [diff] [review]
Enable/Disable mozNfc at runtime

This patch is used to enable/disable mozNfc at runtime.

Previous proposed solution is check system property in HasNfcSupport function and Michael Wu comment about we should not check system property in child process.

So this patch uses another way to know NFC is enable or disable in HasNfcSupport.
The idea is send a dummy message to NFC module, if NFC module is enabled then return result will be a non-zero array.

The patch depends on fixed in bug 947100.

Comment 43

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8345215 [details] [diff] [review]
Enable/Disable mozNfc at runtime

>+  // If NFC module exist then the return value will be a non-zero array.

s/exist/exists/.

I assume you mean "nonempty array", not "non-zero array", right?

>+  nsCOMPtr<nsISyncMessageSender> cpmm = do_GetService("@mozilla.org/childprocessmessagemanager;1");

Will this return null in the chrome process?  Do we want to claim that nfc is unsupported there?

>+  if (!cpmm)
>+    return false;

Braces around if bodies, please.  This comes up several times in this patch.

>+  JS::Value aRetval;

It's not an argument, so no "a" prefix, please.  Also, this needs to be a JS::Rooted<JS::Value>.

>+  nsIPrincipal* principal = sop ? principal = sop->GetPrincipal() : NULL;

sop can never be null here, so no need to null-check.

>+  if (NS_OK != cpmm->SendSyncMessage(NS_LITERAL_STRING("NFC:Dummy"), JSVAL_NULL,
>+                                         JSVAL_NULL, principal, aCx, 0, &aRetval)) {

This is the part that definitely gets the r-.

If you look at SendSyncMessage, it will throw exceptions on aCx in many cases in which it returns non-ok values.  But your code here will just leave the exception on aCx and return to the caller without telling it an exception was thrown, which will put the JSContext in a bad state.  The _next_ code that tries to run on it will get a mysterious exception randomly.

You need to either report or clear the exception if there is one.

As far as nits, JS::NullValue(), and fix the indent please.  And you probably want NS_FAILED instead of direct compares to NS_OK.

>+  JS::Rooted<JSObject*> obj(aCx, JSVAL_TO_OBJECT(aRetval));

What if the value is not an object?  I think you probably want something more like:

  if (!retval.isObject()) {
    return false;
  }
  JS::Rooted<JSObject*>(aCx, &retval.toObject());

>+  if (!JS_IsArrayObject(aCx, obj) || !JS_GetArrayLength(aCx, obj, &length))

JS_GetArrayLength can throw; that's what a false return from it means.  See comments about unhandled exceptions on aCx above.

I can't speak to the Nfc.js changes, and in particular whether they would in fact return a nonempty array.

Should I be scared that we're not doing a permission check for this property right now, even though we have a function to do so?  :(

Comment 44

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

This is defect for NFC feature. Should be fixed in 1.3.

Comment 45

[Dimi Lee [:dimi]]

Attachment: Enable/Disable mozNfc at runtime v2

Comment 46

[Dimi Lee [:dimi]]

(In reply to Boris Zbarsky [:bz] from comment #43)
> Comment on attachment 8345215 [details] [diff] [review]
> Enable/Disable mozNfc at runtime
> 
> >+  // If NFC module exist then the return value will be a non-zero array.
> 
> s/exist/exists/.
> 
> I assume you mean "nonempty array", not "non-zero array", right?
> 
> >+  nsCOMPtr<nsISyncMessageSender> cpmm = do_GetService("@mozilla.org/childprocessmessagemanager;1");
> 
> Will this return null in the chrome process?  Do we want to claim that nfc
> is unsupported there?
> 
> >+  if (!cpmm)
> >+    return false;
> 
> Braces around if bodies, please.  This comes up several times in this patch.
> 
> >+  JS::Value aRetval;
> 
> It's not an argument, so no "a" prefix, please.  Also, this needs to be a
> JS::Rooted<JS::Value>.
> 
> >+  nsIPrincipal* principal = sop ? principal = sop->GetPrincipal() : NULL;
> 
> sop can never be null here, so no need to null-check.
> 
> >+  if (NS_OK != cpmm->SendSyncMessage(NS_LITERAL_STRING("NFC:Dummy"), JSVAL_NULL,
> >+                                         JSVAL_NULL, principal, aCx, 0, &aRetval)) {
> 
> This is the part that definitely gets the r-.
> 
> If you look at SendSyncMessage, it will throw exceptions on aCx in many
> cases in which it returns non-ok values.  But your code here will just leave
> the exception on aCx and return to the caller without telling it an
> exception was thrown, which will put the JSContext in a bad state.  The
> _next_ code that tries to run on it will get a mysterious exception randomly.
> 
> You need to either report or clear the exception if there is one.
> 
> As far as nits, JS::NullValue(), and fix the indent please.  And you
> probably want NS_FAILED instead of direct compares to NS_OK.
> 
> >+  JS::Rooted<JSObject*> obj(aCx, JSVAL_TO_OBJECT(aRetval));
> 
> What if the value is not an object?  I think you probably want something
> more like:
> 
>   if (!retval.isObject()) {
>     return false;
>   }
>   JS::Rooted<JSObject*>(aCx, &retval.toObject());
> 
> >+  if (!JS_IsArrayObject(aCx, obj) || !JS_GetArrayLength(aCx, obj, &length))
> 
> JS_GetArrayLength can throw; that's what a false return from it means.  See
> comments about unhandled exceptions on aCx above.
> 
> I can't speak to the Nfc.js changes, and in particular whether they would in
> fact return a nonempty array.
> 
> Should I be scared that we're not doing a permission check for this property
> right now, even though we have a function to do so?  :(

About the permission check there is another bug working on it - bug 946047. 

Thanks for your comment. Vicamo suggested another approach which is much more straightforward so i will update another patch to review.

Comment 47

[Dimi Lee [:dimi]]

Comment on attachment 8345721 [details] [diff] [review]
Enable/Disable mozNfc at runtime v2

Based on bug 947100, if platform does not support NFC, NFC content helper will not be created.

So we can use do_GetService to know if NFC is supported.

Comment 48

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8345721 [details] [diff] [review]
Enable/Disable mozNfc at runtime v2

r=me

Comment 49

[Jose Manuel Cantera]

if the device does not include NFC hardware, how in Gaia we will detect it? the mozNfc object will not present in navigator? 

or should we try to get an error some place on the API calls?

Comment 50

[Garner Lee]

(In reply to Jose M. Cantera from comment #49)
> if the device does not include NFC hardware, how in Gaia we will detect it?
> the mozNfc object will not present in navigator? 

mozNfc being missing simply means there's no permission declared for "nfc", checked by the MozNfc.webidl HasnfcSupport (implemented in Navigator.cpp).

I believe Bug 947100 is intended to directly answer the remaining question:

> 
> or should we try to get an error some place on the API calls?

Comment 51

[[:fabrice] Fabrice Desré]

Is there something preventing this patch from landing? If not and you have no commit rights, please use checkin-neeeded.

Comment 52

[Dimi Lee [:dimi]]

(In reply to Fabrice Desré [:fabrice] from comment #51)
> Is there something preventing this patch from landing? If not and you have
> no commit rights, please use checkin-neeeded.

I was waiting for bug 949370 before and it is clear now.

Comment 53

[Dimi Lee [:dimi]]

Attachment: Rebased fix for bug 939056

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/0b5646f568bb

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0b5646f568bb

Comment 56

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/63d2929a55ed

Comment 57

[Kai-Zhen Li [:seinlin][:kli]]

It also depends on bug 946047.

Comment 59

[Joe Cheng [:jcheng] (please needinfo)]

status-b2g-v1.3t fixed. remove [tarako]