We currently start nfcd during boot. The problems with this approach are - resource consumption - increased start-up time of the system - cannot restart failed nfcd We should instead only run nfcd while NFC has been switched on by the user. Recent changes in Gecko make this possible.

Hello With this patch set, nfcd will only run while NFC is switched on; for the reasons given in the bug description.

Hi Thomas Thanks for working on this. However we're still working on v2.2+ bugs, also there's a workshop for P2P and Chinese New Year are coming, we will start to review and also test on your patch until March. Is this okay for you? Thanks

(In reply to Yoshi Huang[:allstars.chh] from comment #12) > Hi Thomas > Thanks for working on this. > However we're still working on v2.2+ bugs, also there's a workshop for P2P > and Chinese New Year are coming, we will start to review and also test on > your patch until March. > > Is this okay for you? Sure. The bug is one of my Q1 goals, so March is still OK. :)

Comment on attachment 8556435 [details] [diff] [review] [01] Bug 1109592: Only open connection to nfcd while NFC is switched on Review of attachment 8556435 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/nfc/gonk/Nfc.js @@ +629,5 @@ > receiveMessage: function receiveMessage(message) { > + > + /* Return early if we don't call out to the NFC Service. Otherwise > + * we might start nfcd for no reason. > + */ Sync comment format, here and below @@ +644,5 @@ > + if (!this.nfcService) { > + if (!this.pendingNfcService && !this.startNfcService()) { > + this.sendPendingMessageErrorResponse(message); > + } > + this.pendingMessageQueue.push(message); Only queue messages to pendingMessageQueue when not error ? @@ +732,5 @@ > }, > > shutdown: function shutdown() { > + if (this.nfcService) { > + this.shutdownNfcService(); Should we move following into |shutdownNfcService| ? while (this.pendingMessageQueue.length) { this.sendPendingMessageErrorResponse(this.pendingMessageQueue.shift()); } I am not sure if it is the corner case but if xpcom was shutdown during initialization, messages in pendingMessageQueue will not be cleared

Ping, just a friendly remainder to not forget about reviewing this patch set. :)

Comment on attachment 8556435 [details] [diff] [review] [01] Bug 1109592: Only open connection to nfcd while NFC is switched on Review of attachment 8556435 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/nfc/gonk/Nfc.js @@ +503,5 @@ > + return; > + } > + > + let nfcMsgType = message.name + "Response"; > + message.data.errorMsg = "NotInitialize"; Should it be "NoInitiailized"? Also I noticed MozNFC.webidl has the same typo. @@ +521,5 @@ > > switch (message.type) { > case "InitializedNotification": > + this.nfcService = this.pendingNfcService; > + if (message.status == 0) { Right now nfcd always reports 0, maybe we could file a seperate bug to fix that, also add the error handling below in that bug. @@ +635,5 @@ > + case "NFC:QueryInfo": > + return {rfState: this.rfState}; > + default: > + break; > + } Maybe we could file a seperate bug to land this minor nit first. @@ +641,5 @@ > + /* Start NFC Service if necessary. Messages are held in a > + * queue while initialization is being performed. > + */ > + if (!this.nfcService) { > + if (!this.pendingNfcService && !this.startNfcService()) { The sequence looks odd to me. In this function (Nfc.receiveMessage), it will process "ChangeRFState, Read/WriteNDEF, MakeReadOnly, Format, Transceive, and SendFile", but except 'ChangeRFState', receiveing all other messages means "NFC is already on", so content side could get the NFCTag/NFCPeer object to do these operations. So I think maybe we could just call startNfcService when "ChangeRFState(Discovery)" is called.

Comment on attachment 8556436 [details] [diff] [review] [02] Bug 1109592: Forward socket state from |NfcSocketListener| Review of attachment 8556436 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/nfc/gonk/NfcService.cpp @@ +363,5 @@ > mThread->Dispatch(runnable, nsIEventTarget::DISPATCH_NORMAL); > } > > +void > +NfcService::OnConnectSuccess(enum SocketType aSocketType) enum SocketType /* not used */ ::: ipc/nfc/Nfc.h @@ +18,5 @@ > { > public: > + enum SocketType { > + STREAM_SOCKET > + }; I am not sure what is this for?

(In reply to Yoshi Huang[:allstars.chh] from comment #17) > Comment on attachment 8556436 [details] [diff] [review] > [02] Bug 1109592: Forward socket state from |NfcSocketListener| > > Review of attachment 8556436 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/nfc/gonk/NfcService.cpp > @@ +363,5 @@ > > mThread->Dispatch(runnable, nsIEventTarget::DISPATCH_NORMAL); > > } > > > > +void > > +NfcService::OnConnectSuccess(enum SocketType aSocketType) > > enum SocketType /* not used */ > > ::: ipc/nfc/Nfc.h > @@ +18,5 @@ > > { > > public: > > + enum SocketType { > > + STREAM_SOCKET > > + }; > > I am not sure what is this for? Forget all these I saw you update these in other patch.

Comment on attachment 8556441 [details] [diff] [review] [06] Bug 1109592: Append random postfix to nfcd socket name Review of attachment 8556441 [details] [diff] [review]: ----------------------------------------------------------------- This looks to me is a security feature we should discuss this in a seperate bug.

Comment on attachment 8556441 [details] [diff] [review] [06] Bug 1109592: Append random postfix to nfcd socket name Review of attachment 8556441 [details] [diff] [review]: ----------------------------------------------------------------- Hi Thomas, Do you think it would be better if we move |mListenSocketName| related stuff into NfcConnector ? Then we can get rid of those socket name handling in NfcService

Comment on attachment 8556440 [details] [diff] [review] [05] Bug 1109592: Listen for connections from NFC daemon Review of attachment 8556440 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/nfc/gonk/NfcService.cpp @@ +317,5 @@ > + > + mListenSocketName = BASE_SOCKET_NAME; > + > + bool success = mListenSocket->Listen(new NfcConnector(), mConsumer); > + if (!success) { Add a warning message @@ +393,5 @@ > { > MOZ_ASSERT(NS_IsMainThread()); > > + switch (aSocketType) { > + case LISTEN_SOCKET: { In MDN coding style i saw there is two space Indentation for |case|

Comment on attachment 8556434 [details] Github pull request for platform_system_nfcd Updated Github pull request - cleaned up patches to better fit Mozilla coding style (switch, methods, args) - added branches to if-else after option parsing - cleaned up several static variables (nfcdRw, MsgListener) Dimi, I cleaned up the points you mentioned and also added some other cleanups for static variables. r?-ing you again, so you won't miss these changes.

> ::: dom/nfc/gonk/Nfc.js > @@ +629,5 @@ > > receiveMessage: function receiveMessage(message) { > > + > > + /* Return early if we don't call out to the NFC Service. Otherwise > > + * we might start nfcd for no reason. > > + */ > > Sync comment format, here and below You mean using '//' for comments? > > @@ +644,5 @@ > > + if (!this.nfcService) { > > + if (!this.pendingNfcService && !this.startNfcService()) { > > + this.sendPendingMessageErrorResponse(message); > > + } > > + this.pendingMessageQueue.push(message); > > Only queue messages to pendingMessageQueue when not error ? Oh, right. I think there should be a 'return' after sending the error response. > > @@ +732,5 @@ > > }, > > > > shutdown: function shutdown() { > > + if (this.nfcService) { > > + this.shutdownNfcService(); > > Should we move following into |shutdownNfcService| ? > while (this.pendingMessageQueue.length) { > this.sendPendingMessageErrorResponse(this.pendingMessageQueue.shift()); > } > I am not sure if it is the corner case but if xpcom was shutdown during > initialization, > messages in pendingMessageQueue will not be cleared Good point. I'll add code to do this.

> ::: dom/nfc/gonk/Nfc.js > @@ +503,5 @@ > > + return; > > + } > > + > > + let nfcMsgType = message.name + "Response"; > > + message.data.errorMsg = "NotInitialize"; > > Should it be "NoInitiailized"? > Also I noticed MozNFC.webidl has the same typo. I thing I looked at the strings in MozNFC.webidl to find a suitable error message. Other strings also look a bit strange. So in any case, I'd prefer if this could be fixed in a separate bug. > @@ +521,5 @@ > > > > switch (message.type) { > > case "InitializedNotification": > > + this.nfcService = this.pendingNfcService; > > + if (message.status == 0) { > > Right now nfcd always reports 0, > maybe we could file a seperate bug to fix that, also add the error handling > below in that bug. Shall I remove the error handling from this patch and file a separate bug instead? > @@ +635,5 @@ > > + case "NFC:QueryInfo": > > + return {rfState: this.rfState}; > > + default: > > + break; > > + } > > Maybe we could file a seperate bug to land this minor nit first. OK. I'll open a bug report for that. > @@ +641,5 @@ > > + /* Start NFC Service if necessary. Messages are held in a > > + * queue while initialization is being performed. > > + */ > > + if (!this.nfcService) { > > + if (!this.pendingNfcService && !this.startNfcService()) { > > The sequence looks odd to me. > > In this function (Nfc.receiveMessage), it will process > "ChangeRFState, Read/WriteNDEF, MakeReadOnly, Format, Transceive, and > SendFile", > but except 'ChangeRFState', receiveing all other messages means "NFC is > already on", so content side could get the NFCTag/NFCPeer object to do these > operations. > > So I think maybe we could just call startNfcService when > "ChangeRFState(Discovery)" is called. This would basically mean some more error checking. Is 'ChangeRfState(Discovery)' always the first message (except QueryInfo)?

> > > @@ +635,5 @@ > > > + case "NFC:QueryInfo": > > > + return {rfState: this.rfState}; > > > + default: > > > + break; > > > + } > > > > Maybe we could file a seperate bug to land this minor nit first. > > OK. I'll open a bug report for that. Or maybe we should simply queue messages into |pendingMessageQueue| in |sendToNfcService|. Establishing the connection could then happen in the switch case of NFC:ChangeRFState. BTW, the current code in |receiveMessage| sets message.data.sessionId for QueryInfo and returns it to the content helper. Other code seems to clear the session ID first. It looks like receiveMessage reveals information to an untrusted process.

(In reply to Yoshi Huang[:allstars.chh] from comment #19) > Comment on attachment 8556441 [details] [diff] [review] > [06] Bug 1109592: Append random postfix to nfcd socket name > > Review of attachment 8556441 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks to me is a security feature > we should discuss this in a seperate bug. It's just partly about security. Another point is reliability when stale sockets from old connections are around. Anyway, I created bug 1140393 for this patch.

Changes since v1: - was patch [07]

> Hi Thomas, > Do you think it would be better if we move |mListenSocketName| related stuff > into NfcConnector ? > Then we can get rid of those socket name handling in NfcService We need mListenSocketName when we start nfcd in |NfcService::OnConnectSuccess|. The instance of |NfcConnector| runs on the I/O thread, while NfcService mostly (?) runs on the main thread. If we move the naming functionality to |NfcConnector| I think we might get some ugly multi-threading issues. BTW, during Q2 2015, I intend to go through all the IPC classes in ipc/ and cleanup the interfaces and improve some of the implementation. Maybe this will allow to find a place for the address randomization (now in bug 1140393). Especially since there's similar code in Bluetooth.

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #24) > > ::: dom/nfc/gonk/Nfc.js > > @@ +503,5 @@ > > > + return; > > > + } > > > + > > > + let nfcMsgType = message.name + "Response"; > > > + message.data.errorMsg = "NotInitialize"; > > > > Should it be "NoInitiailized"? > > Also I noticed MozNFC.webidl has the same typo. > > I thing I looked at the strings in MozNFC.webidl to find a suitable error > message. Other strings also look a bit strange. So in any case, I'd prefer > if this could be fixed in a separate bug. > Seperate bug is okay. > > @@ +521,5 @@ > > > > > > switch (message.type) { > > > case "InitializedNotification": > > > + this.nfcService = this.pendingNfcService; > > > + if (message.status == 0) { > > > > Right now nfcd always reports 0, > > maybe we could file a seperate bug to fix that, also add the error handling > > below in that bug. > > Shall I remove the error handling from this patch and file a separate bug > instead? > Seperate bug is okay. > > > @@ +641,5 @@ > > > + /* Start NFC Service if necessary. Messages are held in a > > > + * queue while initialization is being performed. > > > + */ > > > + if (!this.nfcService) { > > > + if (!this.pendingNfcService && !this.startNfcService()) { > > > > The sequence looks odd to me. > > > > In this function (Nfc.receiveMessage), it will process > > "ChangeRFState, Read/WriteNDEF, MakeReadOnly, Format, Transceive, and > > SendFile", > > but except 'ChangeRFState', receiveing all other messages means "NFC is > > already on", so content side could get the NFCTag/NFCPeer object to do these > > operations. > > > > So I think maybe we could just call startNfcService when > > "ChangeRFState(Discovery)" is called. > > This would basically mean some more error checking. Is > 'ChangeRfState(Discovery)' always the first message (except QueryInfo)? ChangeRFState is the first message, however the valud of rfState will depend on Gaia implementation.

Comment on attachment 8556436 [details] [diff] [review] [02] Bug 1109592: Forward socket state from |NfcSocketListener| Review of attachment 8556436 [details] [diff] [review]: ----------------------------------------------------------------- ::: ipc/nfc/Nfc.cpp @@ +216,2 @@ > // Nothing to do here. > CHROMIUM_LOG("NFC: %s\n", __FUNCTION__); nits, the LOG function is moved to before 'if in Part 5 It should be done here.

Comment on attachment 8556436 [details] [diff] [review] [02] Bug 1109592: Forward socket state from |NfcSocketListener| Review of attachment 8556436 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/nfc/gonk/NfcService.cpp @@ +367,5 @@ > +NfcService::OnConnectSuccess(enum SocketType aSocketType) > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + return; return; is not neccesary. same before below.

Comment on attachment 8556438 [details] [diff] [review] [03] Bug 1109592: Add |NfcListenSocket| Review of attachment 8556438 [details] [diff] [review]: ----------------------------------------------------------------- cancelling r+ for this part is related to Part 5

Comment on attachment 8556440 [details] [diff] [review] [05] Bug 1109592: Listen for connections from NFC daemon Review of attachment 8556440 [details] [diff] [review]: ----------------------------------------------------------------- I prefer the leave the low level socket handling to ipc/nfc/ Like the NfcConsumer. and let NfcService to handle more high level stuff. ::: dom/nfc/gonk/NfcService.cpp @@ +310,5 @@ > mHandler = new NfcMessageHandler(); > > mConsumer = new NfcConsumer(this); > > + if (!mListenSocket) { What does this 'if (!mListenSocket)' mean? When will mListenSocket is already running before NfcService start? @@ +320,5 @@ > + bool success = mListenSocket->Listen(new NfcConnector(), mConsumer); > + if (!success) { > + mThread->Shutdown(); > + mThread = nullptr; > + return NS_ERROR_FAILURE; create listenSocket before creating the thread?

> > > Should it be "NoInitiailized"? > > > Also I noticed MozNFC.webidl has the same typo. > > > > I thing I looked at the strings in MozNFC.webidl to find a suitable error > > message. Other strings also look a bit strange. So in any case, I'd prefer > > if this could be fixed in a separate bug. > > > Seperate bug is okay. Bug 1140993

Hi > Review of attachment 8556440 [details] [diff] [review]: > ----------------------------------------------------------------- > > I prefer the leave the low level socket handling to ipc/nfc/ Like the > NfcConsumer. > and let NfcService to handle more high level stuff. I'm not sure if I really understand your proposing here, so let me explain the current design and the rational behind it. Low-level socket handling already is located in ipc/nfc. |NfcConsumer| is a stream socket for transferring data. Adding a listen socket to it wouldn't make sense from a design perspective. Please note that the stream socket can get closed, while the listen socket remains open. See my next paragraph on automatic restarts for that. We'd need |NfcService| to know about the listen socket then. BTW we use also use this design for Bluetooth. There's a class called |BluetoothDaemonInterface| that maintains two stream sockets and one listen socket. Having a similar design in NFC seems reasonable to me. > ::: dom/nfc/gonk/NfcService.cpp > @@ +310,5 @@ > > mHandler = new NfcMessageHandler(); > > > > mConsumer = new NfcConsumer(this); > > > > + if (!mListenSocket) { > > What does this 'if (!mListenSocket)' mean? > When will mListenSocket is already running before NfcService start? ATM probably never. I guess we can remove it here. The overall context is that high-level code can restart NFC in the case of a crash in nfcd. In that case, |NfcService| would already exist, the stream socket would be closed, but the listen socket would still be open and ready to accept a new connection from a re-started nfcd. However the code is not yet ready for this scenario. > @@ +320,5 @@ > > + bool success = mListenSocket->Listen(new NfcConnector(), mConsumer); > > + if (!success) { > > + mThread->Shutdown(); > > + mThread = nullptr; > > + return NS_ERROR_FAILURE; > > create listenSocket before creating the thread? We can do that, but it doesn't make any difference. The connection procedure runs on the main thread. So |Start| will complete in any case before a connection is established.

Changes since v1: - rebased onto bug 1140383 - removed error handling for failed initialization (doesn't happen ATM) - send error message if Gecko shuts down during initialization - only start nfcd on ChangeRFState(Discover), reply error for other messages - coding style cleanups

Changes since v2: - same as (v2), but (v2) contained wrong patch file

Changes since v1: - moved logging to beginning of NfcConnector methods - removed unnecessary return statements

Changes since v1: - rebased onto [02](v2) Only rebasing for now. I'd like more discussion about the socket handling before doing other changes.

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #35) > Hi > > > Review of attachment 8556440 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > I prefer the leave the low level socket handling to ipc/nfc/ Like the > > NfcConsumer. > > and let NfcService to handle more high level stuff. > > I'm not sure if I really understand your proposing here, so let me explain > the current design and the rational behind it. > > Low-level socket handling already is located in ipc/nfc. |NfcConsumer| is a > stream socket for transferring data. Adding a listen socket to it wouldn't > make sense from a design perspective. > > Please note that the stream socket can get closed, while the listen socket > remains open. See my next paragraph on automatic restarts for that. We'd > need |NfcService| to know about the listen socket then. > > BTW we use also use this design for Bluetooth. There's a class called > |BluetoothDaemonInterface| that maintains two stream sockets and one listen > socket. Having a similar design in NFC seems reasonable to me. > NfcService has some problem is that it looks like a service living in parent process to process NFC requests from child processes, but it is more like a 'NFC Daemon Interface'. For example we don't have a clear boundary between 'NFC Service' and 'NFC daemon interface' like bluetooth does. That's why I said to seperate those socket handling. But I am okay to land your changes here because the seperation may take longer and could impact your goal. > > ::: dom/nfc/gonk/NfcService.cpp > > @@ +310,5 @@ > > > mHandler = new NfcMessageHandler(); > > > > > > mConsumer = new NfcConsumer(this); > > > > > > + if (!mListenSocket) { > > > > What does this 'if (!mListenSocket)' mean? > > When will mListenSocket is already running before NfcService start? > > ATM probably never. I guess we can remove it here. > > The overall context is that high-level code can restart NFC in the case of a > crash in nfcd. Sorry I don't get this part, who will restart NFC and which part of NFC stack will be restarted?

> NfcService has some problem is that it looks like a service living in parent > process to process NFC requests from child processes, but it is more like a > 'NFC Daemon Interface'. > For example we don't have a clear boundary between 'NFC Service' and 'NFC > daemon interface' like bluetooth does. > That's why I said to seperate those socket handling. > But I am okay to land your changes here because the seperation may take > longer and could impact your goal. Hey I'm not trying to mess up the NFC code; just trying to find a good solution ;) A question: if NfcService is not about socket handling, what's the purpose of this class? It also contains the NFC thread, but the work of the NFC thread could also be performed on the I/O thread AFAICT. I mentioned in another comment that I intend to cleanup the IPC interfaces in ipc/ during Q2 of this year. Once that happened, it might make sense to re-evaluate the code in NfcService.{cpp,h}. BTW the internal interface in Bluetooth is mostly unrelated to its socket code. The interface is the result of implementing multiple backends; while the socket code results from other requirements. > > > > The overall context is that high-level code can restart NFC in the case of a > > crash in nfcd. > Sorry I don't get this part, > who will restart NFC and which part of NFC stack will be restarted? Let's say, NFC crashes. Another part of the software stack (Gaia, |NfcService|, Nfc.js) can detect this and call the Start method of |NfcService| to restart nfcd. In this case, mListenSocket would exist already. But, as I mentioned, that's not implemented right now, so I'll remove the conditional from the patch.

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #41) > > NfcService has some problem is that it looks like a service living in parent > > process to process NFC requests from child processes, but it is more like a > > 'NFC Daemon Interface'. > > For example we don't have a clear boundary between 'NFC Service' and 'NFC > > daemon interface' like bluetooth does. > > That's why I said to seperate those socket handling. > > But I am okay to land your changes here because the seperation may take > > longer and could impact your goal. > > Hey I'm not trying to mess up the NFC code; just trying to find a good > solution ;) > > A question: if NfcService is not about socket handling, what's the purpose > of this class? It also contains the NFC thread, but the work of the NFC > thread could also be performed on the I/O thread AFAICT. > My original idea is to create some abstraction like BluetoothDaemonInterface you said, but I didn't have time to refactor that. Free feel to refactor it, I'd also love to see NFC/BT could share more common structure.

Comment on attachment 8556438 [details] [diff] [review] [03] Bug 1109592: Add |NfcListenSocket| Review of attachment 8556438 [details] [diff] [review]: ----------------------------------------------------------------- This part should be listed as Part 4, or should be merged with Part 5.

Comment on attachment 8574594 [details] [diff] [review] [01] Bug 1109592: Only open connection to nfcd while NFC is switched on (v3) Review of attachment 8574594 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/nfc/gonk/Nfc.js @@ +596,5 @@ > + * > + * @param message > + * An nsIMessageListener's message parameter. > + */ > + sendPendingMessageErrorResponse: function sendPendingMessageErrorResponse(message) { This function could be replaced by sendNfcErrorResponse with errorCode set to NOT_INITIAILIZED. @@ +724,5 @@ > + // Start NFC Service if necessary. Messages are held in a > + // queue while initialization is being performed. > + if (!this.nfcService) { > + if ((message.name == "NFC:ChangeRFState") && > + (message.data.rfState == "discovery") && rfState != 'idle'

Comment on attachment 8574608 [details] [diff] [review] [05] Bug 1109592: Listen for connections from NFC daemon (v2) Review of attachment 8574608 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/nfc/gonk/NfcService.cpp @@ +401,5 @@ > { > MOZ_ASSERT(NS_IsMainThread()); > + > + switch (aSocketType) { > + case LISTEN_SOCKET: { indent

Changes since v3: - replaced state-change test for 'discovery' with test for 'not idle'

Changes since v1: - changed position with [04]

Changes since v1: - changed positions with [03]

Changes since v2: - open listen socket before creating creating NFC thread - removed conditional branch for listen socket - coding style fixes

Hi > ::: dom/nfc/gonk/Nfc.js > @@ +596,5 @@ > > + * > > + * @param message > > + * An nsIMessageListener's message parameter. > > + */ > > + sendPendingMessageErrorResponse: function sendPendingMessageErrorResponse(message) { > > This function could be replaced by > sendNfcErrorResponse with errorCode set to NOT_INITIAILIZED. Are you sure? I'm asking because NOT_INITIALIZED doesn't exist yet AFAICT, and |sendNfcErrorResponse| seems to support Gecko-internal errors only.

Comment on attachment 8556434 [details] Github pull request for platform_system_nfcd Updated Github pull request - added NBOUNDS constant for socket name's '\0' - added missing 'break' in options parser - coding-style fixes

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #50) > Hi > > > ::: dom/nfc/gonk/Nfc.js > > @@ +596,5 @@ > > > + * > > > + * @param message > > > + * An nsIMessageListener's message parameter. > > > + */ > > > + sendPendingMessageErrorResponse: function sendPendingMessageErrorResponse(message) { > > > > This function could be replaced by > > sendNfcErrorResponse with errorCode set to NOT_INITIAILIZED. > > Are you sure? I'm asking because NOT_INITIALIZED doesn't exist yet AFAICT, > and |sendNfcErrorResponse| seems to support Gecko-internal errors only. You can add a new error code, Also I think it doesn't make sense that nfcd will return a NotInitialize error code. I'll also fix that in Bug 1140993. Also I just found sendNfcErrorResponse is not used at all, you could remove it.

Changes since v4: - changed |sendNfcErrorResponse| to take a string as second argument - replaced |sendPendingMessageError| by new |sendNfcErrorResponse|

Hi Sheriffs, Please check-in the attached patches [01] to [06] and the three pull requests. Thanks!

Trying... https://treeherder.mozilla.org/#/jobs?repo=try&revision=aff886671aaa There are no automated tests for NFC AFAIK, so I only enabled builds.

https://hg.mozilla.org/integration/b2g-inbound/rev/6c311023dcab https://hg.mozilla.org/integration/b2g-inbound/rev/226052175769 https://hg.mozilla.org/integration/b2g-inbound/rev/fb83f94efd3c https://hg.mozilla.org/integration/b2g-inbound/rev/0193b32071a2 https://hg.mozilla.org/integration/b2g-inbound/rev/e0c5c9fea6bb https://hg.mozilla.org/integration/b2g-inbound/rev/b3e6ad2b1d4b Master: https://github.com/mozilla-b2g/platform_system_nfcd/commit/ab0e87a2be365b9c7470a9deca1eeaaa2f962b18 Master: https://github.com/mozilla-b2g/gonk-misc/commit/2e9f0341416e97926d4cfdb1ff961ec4d4069b0a kitkat: https://github.com/mozilla-b2g/device-flame/commit/b83fc73de7b64594cd74b33e498bf08332b5d87b

https://hg.mozilla.org/mozilla-central/rev/6c311023dcab https://hg.mozilla.org/mozilla-central/rev/226052175769 https://hg.mozilla.org/mozilla-central/rev/fb83f94efd3c https://hg.mozilla.org/mozilla-central/rev/0193b32071a2 https://hg.mozilla.org/mozilla-central/rev/e0c5c9fea6bb https://hg.mozilla.org/mozilla-central/rev/b3e6ad2b1d4b