This should be somewhat similar to WebBluetooth (bug 674737). The goal is to allow a web app on one NFC-enabled phone to be able to discover a web app on another NFC-enabled phone. Many details TBD.

Hi Chris, Arno and I would like to implement the NFC API as contribution of Deutsche Telekom. Attached you can find a first draft of the API in xpidl format. The API proposal mainly based on phonegap-nfc (https://github.com/chariotsolutions/phonegap-nfc) and Android API (http://developer.android.com/reference/android/nfc/NfcAdapter.html). We also took some other NFC API's (e.g. Qt Mobility 1.2 Connectivity API) into consideration. The tag/tech nomenclature is from Android. The idl is mainly based on existing ones (netwerk/wifi, dom/telephony, etc.). We decided to use a syntax similar to netwerk/wifi instead of nsIDOMEventListener because you want to be able to attach listener with a filter (e.g. mimetype, etc.) This is just a first draft and any feedback is welcome. It also leaves out all constants we plan more or less taking one to one from Android (e.g. NDEF tag types, etc.). Furthermore the draft only is for NDEF tag reading/writing we will add more functionality like p2p communication and other tech later but we kept them in mind. We will be implementing it on top of libnfc-nxp which is already in the B2G source tree. As we are new to Gecko development we also have several questions: 1. Where should we put that new interface (maybe netwerk/nfc)? 2. Should we moz prefix anything in the idl ? 3. How are we supposed to implement security (e.g. not every app should be able to use this feature. Maybe also only foreground apps should be allowed to be notified). Is there any similar feature we can take a look at. Also is there something like Android's Intent system so apps can be started once a certain tag gets discovered in B2G?

Hey Markus, (In reply to Markus Neubrand from comment #2) > Hi Chris, > > Arno and I would like to implement the NFC API as contribution of Deutsche > Telekom. Attached you can find a first draft of the API in xpidl format. > Great! Your approach looks good, but I'm not the best reviewer of DOM APIs. > As we are new to Gecko development we also have several questions: I'm also not the best person to answer these questions, but folks on the CC list can help. > 1. Where should we put that new interface (maybe netwerk/nfc)? My guess would be dom/nfc/interfaces. Others can correct me. > 2. Should we moz prefix anything in the idl ? I believe our style is that DOM interfaces are names nsIDOM*, but again I'm not an expert here. > 3. How are we supposed to implement security (e.g. not every app should be > able to use this feature. Maybe also only foreground apps should be allowed > to be notified). Is there any similar feature we can take a look at. Also is > there something like Android's Intent system so apps can be started once a > certain tag gets discovered in B2G? Obviously, I wouldn't let these issues slow down your prototyping. Currently, we have a very simple but rather hard-to-use permissions band-aid built on Gecko "preferences". We can point you to an example when it becomes relevant. We're currently working on a cleaner permissions manager (https://bugzilla.mozilla.org/show_bug.cgi?id=707625), which hopefully will be ready by the time you're done here. Similarly, we're in the middle of prototyping an intents mechanism, but it's not ready for use within gecko yet. Let (us) know if you have more questions about gecko hacking.

A few random comments: - Don't use hasSupport() methods. I think it would be better to have navigator.nfc returning null if there is no support or use another API (like Device Capabilities API) to know if the device has NFC support; - Your listeners objects are not really Weby. you should use addEventListener() and DOM events instead; - In nsINdef, I would change isWriteable() function to a readonly attribute and I don't really like the readonly handling but I've no better solution off-hand; - In nsINdefRecord, can the 'create' methods be sync safely? Should we use an async API here? Generally, I think it would be better to discuss that API in dev-webapi mailing list [1] instead of here. [1] https://www.mozilla.org/about/forums/#dev-webapi

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #4) > - Your listeners objects are not really Weby. you should use > addEventListener() and DOM events instead; I agree. I realize you mention filtering as a reason for not going with EventTarget/EventListener in comment 2. Event listeners could easily filter themselves, though, right? It's not like NFC events would occur at a high frequency, or am I wrong? > - In nsINdefRecord, can the 'create' methods be sync safely? Should we use > an async API here? In particular, why do those methods exist on nsINdefRecord? Actually, why do they exist at all? :) I'm not 100% sure I understand all the abstractions here yet. It would be good to talk through some use cases with JS pseudocode on the dev-webapi mailinglist that Mounir mentioned, particularly for those who are not super familiar with NFC yet. :)

@Chris: I will make those changes and DOMify our API. (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #4) > A few random comments: > - Don't use hasSupport() methods. I think it would be better to have > navigator.nfc returning null if there is no support or use another API (like > Device Capabilities API) to know if the device has NFC support; Will do. > - Your listeners objects are not really Weby. you should use > addEventListener() and DOM events instead; I think if we don't build that filter mechanism into the API but instead leave it to the application to filter that there will be huge overhead. Imagine if there are ~10 apps which want to receive NFC tag discoveries. They'll all be notified and have to do the same filtering. I don't think that this is a good solution. Of course that problem won't occur if we only allow e.g. the foreground app to interact with the NFC API. > - In nsINdef, I would change isWriteable() function to a readonly attribute > and I don't really like the readonly handling but I've no better solution > off-hand; Will do > - In nsINdefRecord, can the 'create' methods be sync safely? Should we use > an async API here? Those are meant as helper methods to be able to create a Uri/Mime NdefRecord without having to know how to fill tnf, type and id with the appropriate constants. In Java/C++ land those would be static methods I don't know how to do that stuff in JS. Maybe we should move them into a separate helper interface to make that clear. > > Generally, I think it would be better to discuss that API in dev-webapi > mailing list [1] instead of here. > > [1] https://www.mozilla.org/about/forums/#dev-webapi Once I made the mentioned changes I will post it to the mailing list with some simple use cases. Btw. I am not super familiar with NFC yet by any means either. Thanks for the feedback!

(In reply to Markus Neubrand from comment #6) > Those are meant as helper methods to be able to create a Uri/Mime NdefRecord > without having to know how to fill tnf, type and id with the appropriate > constants. In Java/C++ land those would be static methods I don't know how > to do that stuff in JS. Maybe we should move them into a separate helper > interface to make that clear. There are no static methods like that in DOM APIs. I'm curious, though, why content would want to create those NdefRecords at all. It seems to me content merely consumes these objects.

(In reply to Markus Neubrand from comment #6) > I think if we don't build that filter mechanism into the API but instead > leave it to the application to filter that there will be huge overhead. > Imagine if there are ~10 apps which want to receive NFC tag discoveries. > They'll all be notified and have to do the same filtering. I don't think > that this is a good solution. Of course that problem won't occur if we only > allow e.g. the foreground app to interact with the NFC API. I see only two reasons to keep filtering outside of the listening handler code: 1. performance issues 2. security issues We should be able to solve (2) by simply not firing the event if the listener isn't allowed to know about it. For (1), that would be an issue if there are a lot of events being sent and we really want to minimize the number of applications that are going to be waken to handle them. However, I've the feeling that NFC isn't something used that much and those events will be fired only at some specific moments when the user is actually interacting with something specific. Is that correct? In my opinion, the cost of using a new way to handle events (inconsistency) seems higher than the win it gives (prevents a few events to be fired).

there were some interesting attacks and ideas shown at cansec that we may want to be cognizant of as we move forward with this

needs to be scheduled for a team review

See https://wiki.mozilla.org/WebAPI/WebNFC for API draft

Markus, note that you will need to ask a review to a DOM peer and ask a super-review to a super-reviewer. Those two persons have to be different.

Yeah, I'm doing the first round for cleanup, and part of that is trying to split out the boilerplate and DOM reviewable needs. We'll hopefully have superreviewers in on the second round.

Unless something has changed drastically, this is not a blocker for V1.

Comment on attachment 633560 [details] [diff] [review] Rebased first draft implementation Review of attachment 633560 [details] [diff] [review]: ----------------------------------------------------------------- Not too bad on the first pass. Mostly cosmetic issues, plus some changes because of things shifting between gecko and gaia. A few things to add: - You'll need a DOM Peer to superreview this as it's a change to the DOM. I'll talk to someone about doing that. - I can't actually comment on the file specifically since there's nothing there, but you need to add your component info to Nfc.manifest Also, we need to check for NFC existence, like seeing if the nfc daemon executable is even available. Since this was based off the RIL, it makes assumptions that aren't really allowed for something that's an optional feature. It might be worth adding a static DoesNFCExist() function or something in dom/nfc/Nfc.h that checks whether the NFC executable even exists, otherwise if you try to boot the IO thread for talking to the socket, it'll just sit there and poll for the life of the gecko process, even though it never has a chance of running. ::: b2g/app/b2g.js @@ +16,4 @@ > // XXX TODO : we should read them from a file somewhere > pref("b2g.privileged.domains", "http://browser.gaiamobile.org, > http://calculator.gaiamobile.org, > + http://nfc-demo.gaiamobile.org, Remove, we do this over in gaia now. ::: dom/Makefile.in @@ +82,5 @@ > endif > > +ifdef MOZ_B2G_NFC > +DIRS += \ > + nfc \ Just one DIRS += nfc line here, don't need multiline ::: dom/nfc/Makefile.in @@ +1,1 @@ > +# ***** BEGIN LICENSE BLOCK ***** Nit: New license block on this @@ +64,5 @@ > +#LOCAL_INCLUDES = \ > +# -I$(topsrcdir)/dom/base \ > +# -I$(topsrcdir)/dom/system/b2g \ > +# -I$(topsrcdir)/content/events/src \ > +# $(NULL) Remove ::: dom/nfc/Nfc.cpp @@ +1,3 @@ > +/* -*- Mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 40 -*- */ > +/* vim: set ts=2 et sw=2 tw=40: */ > +/* ***** BEGIN LICENSE BLOCK ***** New license block @@ +59,5 @@ > + > +USING_NFC_NAMESPACE > +using mozilla::Preferences; > + > +#define DOM_TELEPHONY_APP_PHONE_URL_PREF "dom.telephony.app.phone.url" This isn't telephony @@ +90,5 @@ > + nsCOMPtr<nsIScriptGlobalObject> sgo = do_QueryInterface(aOwner); > + NS_ENSURE_TRUE(sgo, nsnull); > + > + nsCOMPtr<nsIScriptContext> scriptContext = sgo->GetContext(); > + NS_ENSURE_TRUE(scriptContext, nsnull); Why are you doing the above work? You don't need a script context at this point. @@ +139,5 @@ > + > +NS_IMETHODIMP > +Nfc::NdefDiscovered(const nsAString &aNdefRecords) > +{ > + //NS_ASSERTION(aNdefRecords, "Null records!"); Nit: remove @@ +159,5 @@ > + // Dispatch incoming event. > + nsRefPtr<NfcNdefEvent> event = NfcNdefEvent::Create(result); > + NS_ASSERTION(event, "This should never fail!"); > + > + LOG("DOM: Created event => dispatching"); Nit: Can you wrap these log calls in a DEBUG define or something? otherwise this is going to get really spammy. @@ +161,5 @@ > + NS_ASSERTION(event, "This should never fail!"); > + > + LOG("DOM: Created event => dispatching"); > + rv = event->Dispatch(ToIDOMEventTarget(), NS_LITERAL_STRING("ndefdiscovered")); > + LOG("DOM: Event dispatched (%d)", NS_ERROR_GET_CODE(rv)); NS_ENSURE_SUCCESS will NS_WARNING for you, so you don't need to double log. @@ +246,5 @@ > + if (!allowed) { > + *aTelephony = nsnull; > + return NS_OK; > + } > + }*/ I realize this is commented out at the moment, but you're gonna need app permissions (We've got this in current bluetooth if you want to check out how I do it there). Replace this block with URIIsChromeOrInPref from nsContentUtils. We'll have to add whitelisting to gaia too, but that's pretty easy. ::: dom/nfc/Nfc.h @@ +44,5 @@ > + > +#include "nsIDOMNfc.h" > +#include "nsINfc.h" > + > +class nsIScriptContext; Nit: I don't see where this is needed to be predeclared in the header? @@ +49,5 @@ > +class nsPIDOMWindow; > + > +BEGIN_NFC_NAMESPACE > + > +class Nfc : public nsDOMEventTargetHelper, Nit: To avoid conflict with the Nfc stuff over in ipc, we might want to call this nsNfc, since it's the implementation of the nsINfc interface. @@ +50,5 @@ > + > +BEGIN_NFC_NAMESPACE > + > +class Nfc : public nsDOMEventTargetHelper, > + public nsIDOMNfc Nit: Alignment @@ +55,5 @@ > +{ > + nsCOMPtr<nsINfc> mNfc; > + nsCOMPtr<nsINFCCallback> mNFCCallback; > + > + NS_DECL_EVENT_HANDLER(ndefdiscovered) I don't see in the IDL where you're set up to fire an onndefdiscovered event? @@ +92,5 @@ > +private: > + Nfc(); > + ~Nfc(); > + > + class NFCCallback : public nsINFCCallback Not sure why this needs to be a member class? ::: dom/nfc/NfcCommon.h @@ +45,5 @@ > +#include "nsCycleCollectionParticipant.h" > +#include "nsDebug.h" > +#include "nsDOMEventTargetHelper.h" > +#include "nsStringGlue.h" > +#include "nsTArray.h" This is a ton of stuff to include everywhere if you aren't actually declaring any types here. Move these to the compilation units that need them. @@ +55,5 @@ > +#define USING_NFC_NAMESPACE \ > + using namespace mozilla::dom::nfc; > + > +class nsIDOMNfc; > +class nsIDOMNfcNdef; What do these need to be predeclared up here for? ::: dom/nfc/NfcFactory.h @@ +1,3 @@ > +/* -*- Mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 40 -*- */ > +/* vim: set ts=2 et sw=2 tw=40: */ > +/* ***** BEGIN LICENSE BLOCK ***** Nit: New license header if we keep this. @@ +45,5 @@ > + > +// Implemented in Nfc.cpp. > + > +nsresult > +NS_NewNfc(nsPIDOMWindow* aWindow, nsIDOMNfc** aNfc); This could probably be moved to Nfc.h (or nsNfc.h if it's fixed like I recommended :) ) ::: dom/nfc/NfcNdefEvent.cpp @@ +1,3 @@ > +/* -*- Mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 40 -*- */ > +/* vim: set ts=2 et sw=2 tw=40: */ > +/* ***** BEGIN LICENSE BLOCK ***** Nit: new license header @@ +46,5 @@ > + > +USING_NFC_NAMESPACE > + > +// static > +already_AddRefed<NfcNdefEvent> Returning a nsRefPtr should return as TemporaryPtr. However, since this is a DOM event, you probably want to be returning nsIDOMNfcNdefEvent? ::: dom/nfc/NfcNdefEvent.h @@ +1,3 @@ > +/* -*- Mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 40 -*- */ > +/* vim: set ts=2 et sw=2 tw=40: */ > +/* ***** BEGIN LICENSE BLOCK ***** Nit: new license header ::: dom/nfc/nsIDOMNavigatorNfc.idl @@ +1,1 @@ > +#include "nsISupports.idl" Nit: Needs license header. ::: dom/nfc/nsIDOMNfc.idl @@ +1,1 @@ > +#include "nsIDOMEventTarget.idl" Nit: Needs license header @@ +1,3 @@ > +#include "nsIDOMEventTarget.idl" > + > +interface nsIDOMNfcNdefRecord; Nit: Prototype not used, remove. ::: dom/nfc/nsIDOMNfcNdefEvent.idl @@ +1,1 @@ > +#include "nsIDOMEvent.idl" Nit: Needs license header @@ +1,3 @@ > +#include "nsIDOMEvent.idl" > + > +interface nsIDOMNfcNdefRecord; Nit: prototype not used, remove. ::: dom/system/gonk/Nfc.js @@ +1,1 @@ > +/* ***** BEGIN LICENSE BLOCK ***** Nit: Needs new license header. ::: dom/system/gonk/SystemWorkerManager.cpp @@ +536,5 @@ > return NS_OK; > } > > +nsresult > +SystemWorkerManager::InitNFC(JSContext *cx) Nit: This is Nfc everywhere else. Should probably stick with one or the other. @@ +544,5 @@ > + // worker lives in RadioInterfaceLayer.js. All we do here is hold it alive and > + // hook it up to the NFC thread. > + nsresult rv; > + nsCOMPtr<nsIWorkerHolder> worker = do_CreateInstance(kNfcWorkerCID, &rv); > + LOG("Error: %d", NS_ERROR_GET_CODE(rv)); This'll print out on NS_ENSURE_SUCCESS failing, don't need it. @@ +546,5 @@ > + nsresult rv; > + nsCOMPtr<nsIWorkerHolder> worker = do_CreateInstance(kNfcWorkerCID, &rv); > + LOG("Error: %d", NS_ERROR_GET_CODE(rv)); > + NS_ENSURE_SUCCESS(rv, rv); > + NS_ENSURE_TRUE(worker, NS_ERROR_FAILURE); You should fail before you get here, since rv will have failed? ::: dom/system/gonk/nfc_consts.js @@ +1,2 @@ > +/* ***** BEGIN LICENSE BLOCK ***** > + * Version: MPL 1.1/GPL 2.0/LGPL 2.1 Nit: New license header @@ +37,5 @@ > + * > + * ***** END LICENSE BLOCK ***** */ > + > +// Allow this file to be imported via Components.utils.import(). > +const EXPORTED_SYMBOLS = Object.keys(this); Is there gonna be more to this? ::: dom/system/gonk/nfc_worker.js @@ +1,1 @@ > +/* ***** BEGIN LICENSE BLOCK ***** Nit: New license header @@ +43,5 @@ > + > +// We leave this as 'undefined' instead of setting it to 'false'. That > +// way an outer scope can define it to 'true' (e.g. for testing purposes) > +// without us overriding that here. > +let DEBUG = true; Probably want to land this undef'd. ::: dom/system/gonk/nsINfc.idl @@ +1,1 @@ > +/* ***** BEGIN LICENSE BLOCK ***** Nit: New license header @@ +39,5 @@ > + > +#include "nsISupports.idl" > + > +[scriptable, uuid(999fa430-6180-11e1-b86c-0800200c9a66)] > +interface nsINFCCallback : nsISupports nit: One interface per idl file (wasn't even aware you could do this). Also: NFC should be Nfc if you're going to stay consistent with the majority of your code thus far. ::: dom/system/gonk/nsNfc.h @@ +1,1 @@ > +/* ***** BEGIN LICENSE BLOCK ***** Nit: New license header @@ +34,5 @@ > + * the terms of any one of the MPL, the GPL or the LGPL. > + * > + * ***** END LICENSE BLOCK ***** */ > + > +// This must always match the CID given in RadioInterfaceLayer.manifest! Nit: This isn't RIL :) ::: ipc/nfc/Makefile.in @@ +1,1 @@ > +# ***** BEGIN LICENSE BLOCK ***** Nit: new license header ::: ipc/nfc/Nfc.cpp @@ +1,2 @@ > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */ > +/* vim: set sw=4 ts=8 et ft=cpp: */ Nit: new license header @@ +83,5 @@ > +struct NfcClient : public RefCounted<NfcClient>, > + public MessageLoopForIO::Watcher > + > +{ > + typedef queue<NfcData*> NfcDataQueue; Nit: 2 spaces for tabs. Also: can probably use nsTArray instead of queue, but queue is probably fine (since I know this is copied from RIL :) ) @@ +358,5 @@ > + goto clean_and_return; > + } else { > + mCurrentWriteOffset += written; > + } > +// LOG("XXXXXXXXXXXXXXXXXXXXXXX written:%ld offset:%ld\n", written, mCurrentWriteOffset); Nit: remove ::: ipc/nfc/Nfc.h @@ +1,3 @@ > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > +/* vim: set sw=2 ts=8 et ft=cpp: */ > +/* ***** BEGIN LICENSE BLOCK ***** Nit: New license header @@ +38,5 @@ > + * > + * ***** END LICENSE BLOCK ***** */ > + > +#ifndef mozilla_ipc_Nfc_h > +#define mozilla_ipc_Nfc_h 1 Nit: Don't need a 1, just a define is fine. Also keep all lowercase just for consistency sake. @@ +46,5 @@ > +namespace base { > +class MessageLoop; > +} > + > +class nsIThread; nit: Unused prototype @@ +54,5 @@ > + > + > +struct NfcData > +{ > + char* json; Nit: 2 spaces for tabs. Also: If this is json, it's probably coming in as UTF8, right? Might we want to do the conversation to nsCString at the time it's received? @@ +61,5 @@ > +class NfcConsumer : public RefCounted<NfcConsumer> > +{ > +public: > + virtual ~NfcConsumer() { } > + virtual void MessageReceived(NfcData* aMessage) { } Should be pure virtual @@ +71,5 @@ > + > +void StopNfc(); > + > +} // namespace ipc > +} // namepsace mozilla Nit: namespace

Talked to a few people and we're running low on device reviewing bandwidth right now, so we'll get the DOM Peer review and Module owner/SR Superreviewer after some of our cleanup is done.

Guys we should have a security review of the proposed architecture of what we are doing here before we get too far down the road. There have been several talks at prominent security conferences this year on NFC and we want to ensure we have everything thought out to avoid problems. Please work with me to find a lead from your end so we can find a date to have a security "chat" about this.

- SystemWorker IPC updated to reflect RIL - Updated all headers to include appropriate license - Implemented feedback from previous review - Not implemented feedback: o Left NfcCallback as member class as it doesn't really make sense to separate it and RIL does this as well ;) o Left NfcData as char* cause we treat it like that on the daemon side and base64 encode content anyways

Request for top level review for Andreas Gal.

Re-upload tarball (the daemon code nfcd's tar file didn't unpack correctly). Each patch goes into the respective directories, and nfcd goes into the <FirefoxOS>/ build root directory.

Comment on attachment 678619 [details] Patch #3 tarball for NFC feature for review (gecko, gaia, nfcd, gonk-misc, b2g) ... A tarball? Seriously? Attaching gal to review this since he apparently asked for it.

Comment on attachment 678633 [details] [diff] [review] NFC daemon feature patch (without external project source directories) 463kb. Yeah. You guys have fun with this.

Ok. I reviewed the patches pretty thoroughly. This is not going to be a popular comment, but please bear with me. The current architecture uses the JNI library code of the android NFC implementation, which itself is based on libnxp and libhardware. The JNI code is wrapped with a JNI emulation layer, that itself uses the boehm gc and a synchronization library. On top of that code sits a socket daemon that Gecko talks to. Concerns: - The nfc daemon runs with device access (root). Adding a ton of new C/C++ code running as root is a serious security risk. - The quality of the android JNI code is abysmal. We would have to very carefully vet this code. - Both the boehm gc and the synchronization library are large code bases. Running them as root is questionable. I am also concerned about memory leaks since boehm is a conservative gc. - The boilerplate to active code ratio of the current architecture is really bad. Disregarding boehm gc and the synchronization library the actual relevant code is less than 10%. The rest is JNI and JNI emulation boilerplate. With the external libraries this drops to below 5%. Here is what I propose instead: I would like to replicate the architecture of the wifi driver in Gecko. Instead of an external demo, we have a nfc worker which uses ctypes to call directly into libhardware and libnxp. I am attaching a JS file with the ctypes definition for libhardware as an example how simple this is. We will rewrite the small fraction of actual NFC calls we use in JS in the worker. The work communicates with the existing DOM code using JSON/postMessage. We won't need any changes to gonk any more.

I estimate that it will take about 2 weeks to write the JS code for this. The code should be around 1500 lines of code for the worker. Since we already have a working solution, it will be very easy to debug this. We can basically run the two code bases side by side to debug the new production quality worker code. The proposed architecture should be pretty straight forward to implement, and will add JS code, which is inherently memory safe. Only the actual ctype calls will add significant risk of buffer overflows as root etc. We won't have to touch gonk, and the JS code will be much less code added than the current solution, and use much less memory, so we should be able to ship it with all hardware we ship and make it part of the standard Gecko distro.

Markus, Arno, what do you guys think?

Thanks for your feedback. We were a little surprised by some of your comments since we have discussed the basic architecture with Philip and Kyle several times before and they have not voiced any objections. Here some specific responses to your comments: * Running nfcd as non-root is easy to change (and definitely needs to happen) * You mentioned “synchronization library” in your review. We are not sure what you are referring to. Note that libnxp relies on pthread synchronization so this is not something we can change. * libnxp is not a widely used library and in terms of API design and documentation no where near the wifi driver that you refer to. To give you an idea of the libnxp API, here just one little example: typedef NFCSTATUS ( *pphNfcIF_Register_t) ( phNfcIF_sReference_t *psReference, phNfcIF_sCallBack_t if_callback, void *psIFConfig ); We don’t want to appear whiny, but without the slightest documentation API like this is incomprehensible. Note the use of void* that is quite common in libnxp. What it is supposed to point to you can only derive by reverse engineering the JNI code. The Android JNI code was written by NXP people so they know their own library. To reverse engineer this would be a huge effort. The JNI code is used by millions of Android devices which in our opinion vets this code. * Boehm is a conservative collector, but chances of leaks are statistically negligible. Note that Boehm is also used in dozens of other projects and can be regarded as a mature and reliable library. We might be able to replace this with some reference counting mechanism to remove Boehm. * Regardless of the worker architecture under the hood we think we should at least land the DOM code to reduce the maintenance burden. We should have a face-to-face discussion to discuss the details and how to proceed from here.

I definitely agree on the DOM code part. We should get that factored out and landed. It should talk to a service that will implement the NFC backend (which is the piece we still have to figure out how to do exactly). I agree on the poor quality of the API of libnxp. Generating those calls with ctypes could be painful. I did some experiments around that myself. I am attaching a code snippet I wrote to wrap the **** library into a cleaner API. We could probably lift large parts of the JNI implementation for this wrapper code (basically copying the code, removing the JNI-isms along the way). Sorry about the lack of architecture attention for this earlier. We are pretty focused on v1 features, so none of the core architects got around to taking a look here. Let me know what you think about the copy & paste, remove jni-isms idea.

The other two libraries are jansson-2.3.1/ and libatomic_ops/. My key concern here is that we are pulling in 160k lines of libraries for 13k lines of actual code, of which more than 10k lines are JNI wrapper code. This doesn't look like a good deal as is.

Thanks for agreeing to land the DOM-specific portions of the NFC patch. We will update the API proposal as a first step to make this happen. WRT to the nfcd specific comments, it seems that it boils down to a matter of style. You wrote that the JNI-isms should be removed and this could be accomplished by lifting large portions of the JNI code. You also made a comment that it is not good to have 10k lines of original contributions vs 160k lines of other code such as the JNI code or Boehm GC. Well, you could also look at this from a different perspective: there is nothing wrong to make use of other Open Source software. In fact, perhaps that is what one should do. We’d rather write only 10k lines than ‘reinventing’ the other 160k. The huge benefit of using the JNI code is maintainability: when you guys upgraded to ICS, all we had to do copy over the new JNI code of the NFC implementation in ICS. If we had followed your approach, all the work we would have done for pre-ICS would have to be redone. Likewise the same work would need to be redone for post-ICS updates and bug fixes to the JNI code. Again, it looks like the discussion boils down to a matter of style. How do you suggest we proceed from here?

I spoke with Kyle, the gecko side of landing the DOM mostly already exists (gonk-misc must enable it via the --enable-b2g-nfc flag), and is okay to land that way. Over the last gecko patch, I've also added a small change in ipc to temporarily disable the remaining part of the NFC gecko implementation. I'll apply for "level 1" access.

This nearly identical code to gecko_05.patch appears to cause constructor count issues now in nsDOMClassInfo.cpp at runtime with the matching 12/6/2012 gecko code. Adding code patch here for feedback/comment: In debug only code block: for (i = 0; i < eDOMClassInfoIDCount; i++) { if (!sClassInfoData[i].u.mConstructorFptr || sClassInfoData[i].mDebugID != i) { MOZ_NOT_REACHED("Class info data out of sync, you forgot to update " "nsDOMClassInfo.h and nsDOMClassInfo.cpp! Fix this, " "mozilla will not work without this fixed!"); return NS_ERROR_NOT_INITIALIZED; } }

Fixed gecko over the last gecko_06.patch. Can pass DEBUG check on new xpcom object. (not fully working yet in gaia).

The gecko DOM changes and matching gaia changes are uploaded (latest available code as of 12/21), and work well with the system daemon nfcd. That daemon (nfcd) is not yet updated with the ref counting code.

Try run for 760a45ecdcb7 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=760a45ecdcb7 Results (out of 1 total builds): success: 1 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dgarnerlee@gmail.com-760a45ecdcb7

Comment on attachment 695046 [details] [diff] [review] NFC Gecko patch to git.mozilla.org 5bad6f1d595945a4b4acc1125447ec0669bf6474 (release) Review of attachment 695046 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/nsIRadioInterfaceLayer.idl @@ +224,5 @@ > readonly attribute DOMString voicemailDisplayName; > + > + nsIDOMDOMRequest iccOpenChannel(in nsIDOMWindow window, in DOMString aid); > + nsIDOMDOMRequest iccExchangeAPDU(in nsIDOMWindow window, in long channel, in jsval apdu); > + nsIDOMDOMRequest iccCloseChannel(in nsIDOMWindow window, in long channel); Are these changes accidentally committed? You don't need them in this patch.

Try run for 67e60cd31899 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=67e60cd31899 Results (out of 1 total builds): success: 1 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dgarnerlee@gmail.com-67e60cd31899

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #48) > Comment on attachment 695046 [details] [diff] [review] > NFC Gecko patch to git.mozilla.org 5bad6f1d595945a4b4acc1125447ec0669bf6474 > (release) > > Review of attachment 695046 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/system/gonk/nsIRadioInterfaceLayer.idl > @@ +224,5 @@ > > readonly attribute DOMString voicemailDisplayName; > > + > > + nsIDOMDOMRequest iccOpenChannel(in nsIDOMWindow window, in DOMString aid); > > + nsIDOMDOMRequest iccExchangeAPDU(in nsIDOMWindow window, in long channel, in jsval apdu); > > + nsIDOMDOMRequest iccCloseChannel(in nsIDOMWindow window, in long channel); > > Are these changes accidentally committed? You don't need them in this patch. Yes, new try submit below has it removed.

Try run for ad675f3268e1 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=ad675f3268e1 Results (out of 1 total builds): success: 1 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dgarnerlee@gmail.com-ad675f3268e1

Try run for 47cd45afb5a1 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=47cd45afb5a1 Results (out of 1 total builds): success: 1 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dgarnerlee@gmail.com-47cd45afb5a1

Try server comment submission: https://tbpl.mozilla.org/?tree=Try&rev=37556585cc69 The (linux) builds appear to be here: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dgarnerlee@gmail.com-37556585cc69/ The gecko side patch includes the secure element feature needed for mobile wallet implementation.

Comment on attachment 709320 [details] [diff] [review] Latest gecko code, Mercurial modified version of the same patch. Review of attachment 709320 [details] [diff] [review]: ----------------------------------------------------------------- Hi Garner, I'm glad to review RIL parts, but for others like DOM, IPC, you should go for better reviewers like :mounir & :qDot. Please separate the files into several patches. Mine would likely be: ril_consts.js, ril_worker.js, RadioInterfaceLayer.js, RILContentHelper.js. Besides, I want to know why does NFC need new APIs in RIL. Even it does, it won't be in WebTelephony. MobileConnection::icc is a better home for it.

"nfcd" is the system daemon side of the Nfc DOM patch. I sits between the NXP libraries and hardware, and the rest of the Gecko DOM. The new version implements refcounting for memory management, instead of using the garbage collector. The last specific reference concerning the switch was in Comment 38. Note: the size of the patch includes supporting the jannson JSON library (and test cases...)

Comment on attachment 591592 [details] API proposal Marking old *.idl API proposal as obsolete, as they are being updated with new patches.

This Gecko DOM patch contains the interface changes needed to enable NFC.

This Gecko DOM implementation code for NFC IDL files.

Attachment 713215 [details] [diff] also contains the System/gonk and IPC interface to a nfcd daemon that talks to the nfc hardware.

Makefiles for nfc. There is additionally a system property that enables the compiled nfc feature at runtime (setprop nfc.enabled) in the previous attachment 683781 [details] [diff] [review].

Needed for competitive parity.

There seems to be some movement on this bug - any idea when a security review might be needed for this feature (or any idea when this is expected to land) ?

I don't have a concrete idea of exactly when, but at last check, it'll be in "v.next". I'm preparing another set of patches to catch up to very recent changes to the Makefiles, as well as more platform integration, using MozActivity for nfc event dispatch.

NFC system daemon patch file (nfcd). This patch removes Boem GC (obsolete), libatomic (obsolete), and jansson. Jansson, a JSON library, currently lives in a forked repository (github.com/svic/jansson) currently, with extra Android.mk files for compilation.

Comment on attachment 730470 [details] [diff] [review] NFC DOM Interface files Review of attachment 730470 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/telephony/nsITelephonyProvider.idl @@ +113,5 @@ > + > + // UICC SE API > + nsIDOMDOMRequest iccOpenChannel(in DOMString aid); > + nsIDOMDOMRequest iccExchangeAPDU(in long channel, in jsval apdu); > + nsIDOMDOMRequest iccCloseChannel(in long channel); Aren't these APIs already landed?

Hi Vicamo, Good point. I'll check the code origin, and reupload the patch as needed.

Removed obsolete telephony Icc{Open,Close,ExchangeAPDU} in favor of IccManager.

Removed obsolete Icc{Open,Close,ExchangeAPDU} in telephony from last patch.

Comment on attachment 730471 [details] [diff] [review] NFC DOM JS, and permissions. Review of attachment 730471 [details] [diff] [review]: ----------------------------------------------------------------- Hi, Curtis, since you expressed interest last July, I put in some changes relevant to NFC security, and it's likely a good time to get some dialog started on the security design. If you're not the rignt person, (or I flagged it incorrectly) please let me know. I note that there has been earlier non-security focused reviews of the code (Thanks to Andreas, Kyle, and Vicamo), although an official decision of sorts of how to move forward for reviewing and landing code, and where security review should be present in that process, will be fantastic.

(In reply to Garner Lee from comment #78) > Comment on attachment 730471 [details] [diff] [review] > NFC DOM JS, and permissions. > > Review of attachment 730471 [details] [diff] [review]: > ----------------------------------------------------------------- > > Hi, Curtis, since you expressed interest last July, I put in some changes > relevant to NFC security, and it's likely a good time to get some dialog > started on the security design. If you're not the rignt person, (or I > flagged it incorrectly) please let me know. I am the right person to get things rolling, however, the person that will actually review the code is Paul, as he owns the sec-review bug that is blocking this. > > I note that there has been earlier non-security focused reviews of the code > (Thanks to Andreas, Kyle, and Vicamo), although an official decision of > sorts of how to move forward for reviewing and landing code, and where > security review should be present in that process, will be fantastic.

Comment on attachment 730465 [details] [diff] [review] NFC system daemon patch file (nfcd). Obsoleting, we'll take care of this in bug 860907 since it's b2g specific.

Comment on attachment 730474 [details] [diff] [review] NFC Demo. Also accepts ndef-discovered activities. Obsoleting and moving to bug 860910

Comment on attachment 730473 [details] [diff] [review] System app update. Platform integration with NFC MozActivities. Obsoleting and moving to bug 860910

Comment on attachment 683781 [details] [diff] [review] Proposed changes to have B2G system property to enable/disable nfc. Moving to bug 860907

Comment on attachment 730474 [details] [diff] [review] NFC Demo. Also accepts ndef-discovered activities. Move to bug 860910

There's a good chance this is getting rewritten as a UnixSocket implementation, but putting it up for completeness sake for the moment.

Ok. Resplit and reordered the patches into something a bit easier to divide up and review. A couple of small changes I made on the way through: - For some reason the nsIDOMDOMRequest was added as an interface prototype in Telephony IDLs? Seemed unneeded since they weren't touched otherwise? - layout/build makefile conflicted. Just shuffled position.

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #92) > - For some reason the nsIDOMDOMRequest was added as an interface prototype > in Telephony IDLs? Seemed unneeded since they weren't touched otherwise? No reason anymore, nsIDOMDOMRequest is unneeded. The WebICC feature landed in mozilla-central, which meant there was a duplicate implementation in FFOS telephony that apparently is incompletely removed.

Ok, well, I went ahead and removed those in the above patch set. BTW, assuming you don't want to rebuild the set by hand, I'm maintaining this git branch while we go through reviews: https://github.com/qdot/mozilla-central/tree/webnfc-674741

Ok I should be able to start the security review this week. I am a little unsure of where to start though. Is there any useful documentation or can someone give me a brief overview (either in the bug, or ping me for a walk-through)?

Comment on attachment 736605 [details] [diff] [review] Patch 5 (v1) - NFC Permissions Changes Review of attachment 736605 [details] [diff] [review]: ----------------------------------------------------------------- ::: extensions/cookie/Permission.txt @@ +110,5 @@ > > +nfc > +{} > +WebNFC > +Near Field Communications You update is fine, but this file is out of date and needs to just go away. (bug 818767)

Comment on attachment 736604 [details] [diff] [review] Patch 4 (v1) - NFC Chrome Worker Implementation Review of attachment 736604 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/Nfc.js @@ +75,5 @@ > + let message = event.data; > + debug("Received message: " + JSON.stringify(message)); > + switch (message.type) { > + case "ndefDiscovered": > + ppmm.broadcastAsyncMessage("NFC:NdefDiscovered", message); Is this OK to be broadcastAsyncMessage? The APIs that I have reviewed in the past mainly used sendAsyncMessage(aMsgName, aContent) to send the message to specific listener (at least I think that is what it does). Is a broadcast message sent to all apps, or only those apps which have registered to receive said message type. If its the latter, then I think its no problem, since you added the permissions check to dom/messages/SystemMessagePermissionsChecker.jsm, and on install only those apps with the 'nfc' permission can register to receive these messages. Mainly just asking this question to confirm my own understanding of the system message manager code.

(In reply to Paul Theriault [:pauljt] from comment #97) > Comment on attachment 736604 [details] [diff] [review] > Patch 4 (v1) - NFC Chrome Worker Implementation > > Review of attachment 736604 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/system/gonk/Nfc.js > @@ +75,5 @@ > > + let message = event.data; > > + debug("Received message: " + JSON.stringify(message)); > > + switch (message.type) { > > + case "ndefDiscovered": > > + ppmm.broadcastAsyncMessage("NFC:NdefDiscovered", message); > > Is this OK to be broadcastAsyncMessage? The APIs that I have reviewed in the > past mainly used sendAsyncMessage(aMsgName, aContent) to send the message to > specific listener (at least I think that is what it does). Is a broadcast > message sent to all apps, or only those apps which have registered to > receive said message type. If its the latter, then I think its no problem, > since you added the permissions check to > dom/messages/SystemMessagePermissionsChecker.jsm, and on install only those > apps with the 'nfc' permission can register to receive these messages. > Mainly just asking this question to confirm my own understanding of the > system message manager code. Hi Paul, The DOM callback/listener code is restricted by the certified "nfc" permission as you understand. The object will be null otherwise if the app does not have it declared in the manfest. There's possibly more than one "certified" app with callbacks (and all of them will receive the callback), but that is also up to whoever is designing the device OS release image. If you know the common usecase is to allow us to restrict it to one callback only, I can look into that. I'm not as familiar with sendAsyncMessage, but the broadcast what RIL used in the past. The NFC code is based on that code originally.

Remove obsolete Permissions.txt changes.

Update jsval field to nsString

Update jsval field to nsString.

Update jsval field to nsString.

Comment on attachment 736607 [details] [diff] [review] Patch 7 (v1) - NFC B2G IPC Implementation Review of attachment 736607 [details] [diff] [review]: ----------------------------------------------------------------- So this is mostly an r- because of design changes since this was first implemented. We've moved most of the functions in here to UnixSocket.h/.cpp, since multiple components (ril, bluetooth, etc...) were copy functions from each other. NFC should also move to UnixSocket. That said, this move should /drastically/ reduce the amount of code in these patches. You should be able to use the current RIL implementation as a guide to do this, as you did the first time around. Basically, you're going to need a "Connector" class that sets up the socket address struct, then possibly a couple of small utility functions to ferry packets to your worker. Let me know if you have any questions.

Comment on attachment 740998 [details] [diff] [review] Patch 1 (v2) - NFC DOM IDL Review of attachment 740998 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/nfc/nsIDOMMozNdefRecord.idl @@ +8,5 @@ > +interface nsIDOMMozNdefRecord : nsISupports { > + readonly attribute octet tnf; > + readonly attribute DOMString type; > + readonly attribute DOMString id; > + readonly attribute jsval payload; Comments in general are good when it's not obvious what the fields are, doubly so when it's a jsval. ::: dom/nfc/nsIDOMNavigatorNfc.idl @@ +7,5 @@ > +interface nsIDOMNfc; > + > +[scriptable, uuid(c5814d20-4dcf-11e1-b86c-0800200c9a66)] > +interface nsIDOMNavigatorNfc : nsISupports { > + nit: Kill blank lines ::: dom/nfc/nsIDOMNfcNdefEvent.idl @@ +9,5 @@ > +[scriptable, builtinclass, uuid(41fcd640-4dd4-11e1-b86c-0800200c9a66)] > +interface nsIDOMNfcNdefEvent : nsIDOMEvent > +{ > + [implicit_jscontext] > + readonly attribute jsval ndefMessages; Comment on what type ndefMessages are would be helpful. Guessing it's just records?

Comment on attachment 740998 [details] [diff] [review] Patch 1 (v2) - NFC DOM IDL Review of attachment 740998 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/nfc/nsIDOMMozNdefRecord.idl @@ +4,5 @@ > + > +#include "nsISupports.idl" > + > +[scriptable, builtinclass, uuid(42a70160-c4a3-11e1-9b21-0800200c9a66)] > +interface nsIDOMMozNdefRecord : nsISupports { Any reason why this is MozNdefRecord but everything else isn't prefixed?

Comment on attachment 736602 [details] [diff] [review] Patch 2 (v1) - NFC DOM Boilerplate Review of attachment 736602 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsGkAtomList.h @@ +722,4 @@ > GK_ATOM(onlevelchange, "onlevelchange") > GK_ATOM(onLoad, "onLoad") > GK_ATOM(onload, "onload") > +GK_ATOM(onndefdiscovered, "onndefdiscovered") Don't these also need #ifdef MOZ_B2G_NFC? ::: dom/base/Navigator.cpp @@ +1465,5 @@ > +#define LOG(args...) __android_log_print(ANDROID_LOG_INFO, "Gonk NFC", args) > +#else > +#include <android/log.h> > +#define LOG(args...) __android_log_print(ANDROID_LOG_INFO, "Gonk NFC", args) > +#endif I don't think this is gonna be a welcomed addition to navigator. Remove the debug messages and use an ifdef'd DEBUG NS_WARNING() if you need to track messages in debug.

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #105) > Comment on attachment 740998 [details] [diff] [review] > Patch 1 (v2) - NFC DOM IDL > > Review of attachment 740998 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/nfc/nsIDOMMozNdefRecord.idl > @@ +4,5 @@ > > + > > +#include "nsISupports.idl" > > + > > +[scriptable, builtinclass, uuid(42a70160-c4a3-11e1-9b21-0800200c9a66)] > > +interface nsIDOMMozNdefRecord : nsISupports { > > Any reason why this is MozNdefRecord but everything else isn't prefixed? No strong reason, (Moz)NdefRecord stands on it's own, but it takes after MozActivity.Any preference either way?