1109592 - Only run nfcd while NFC is switched on

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

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

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.

Comment 1

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

Attachment: Github pull request for platform_system_nfcd

Comment 2

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

Attachment: [01] Bug 1109592: Only open connection to nfcd while NFC is switched on

Comment 3

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

Attachment: [02] Bug 1109592: Forward socket state from |NfcSocketListener|

Comment 4

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

Attachment: [03] Bug 1109592: Add |NfcListenSocket|

Comment 5

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

Attachment: [04] Bug 1109592: Move |NfcConnector| to a more public place

Comment 6

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

Attachment: [05] Bug 1109592: Listen for connections from NFC daemon

Comment 7

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

Attachment: [06] Bug 1109592: Append random postfix to nfcd socket name

Comment 8

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

Attachment: [07] Bug 1109592: Cleanup unused variables and fields from NFC's IPC code

Comment 9

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

Attachment: Github pull request for gonk-misc

Comment 10

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

Attachment: Github pull request for device-flame

Comment 11

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

Hello

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

Comment 12

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

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

Comment 13

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

(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 14

[Dimi Lee [ooo 16.04 - 21.04］]

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

Comment 15

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

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

Comment 16

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

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 17

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

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?

Comment 18

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

(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 19

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

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 20

[Dimi Lee [ooo 16.04 - 21.04］]

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 21

[Dimi Lee [ooo 16.04 - 21.04］]

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 22

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

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.

Comment 23

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

> ::: 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.

Comment 24

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

> ::: 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)?

Comment 25

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

> 
> > @@ +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.

Comment 26

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

(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.

Comment 27

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

Attachment: [06] Bug 1109592: Cleanup unused variables and fields from NFC's IPC code (v2)

Changes since v1:

  - was patch [07]

Comment 28

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

> 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.

Comment 29

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

(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 30

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

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 31

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

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 32

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

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 33

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

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?

Comment 34

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

> > > 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

Comment 35

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

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.

Comment 36

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

Attachment: [01] Bug 1109592: Only open connection to nfcd while NFC is switched on (v2)

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

Comment 37

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

Attachment: [01] Bug 1109592: Only open connection to nfcd while NFC is switched on (v3)

Changes since v2:

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

Comment 38

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

Attachment: [02] Bug 1109592: Forward socket state from |NfcSocketListener| (v2)

Changes since v1:

  - moved logging to beginning of NfcConnector methods
  - removed unnecessary return statements

Comment 39

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

Attachment: [05] Bug 1109592: Listen for connections from NFC daemon (v2)

Changes since v1:

  - rebased onto [02](v2)

Only rebasing for now. I'd like more discussion about the socket handling before doing other changes.

Comment 40

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

(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?

Comment 41

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

> 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.

Comment 42

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

(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 43

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

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 44

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

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 45

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

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

Comment 46

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

Attachment: [01] Bug 1109592: Only open connection to nfcd while NFC is switched on (v4)

Changes since v3:

  - replaced state-change test for 'discovery' with test for 'not idle'

Comment 47

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

Attachment: [03] Bug 1109592: Move |NfcConnector| to a more public place (v2)

Changes since v1:

  - changed position with [04]

Comment 48

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

Attachment: [04] Bug 1109592: Add |NfcListenSocket| (v2)

Changes since v1:

  - changed positions with [03]

Comment 49

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

Attachment: [05] Bug 1109592: Listen for connections from NFC daemon (v3)

Changes since v2:

  - open listen socket before creating creating NFC thread
  - removed conditional branch for listen socket
  - coding style fixes

Comment 50

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

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 51

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

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

Comment 52

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

(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.

Comment 53

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

Attachment: [01] Bug 1109592: Only open connection to nfcd while NFC is switched on (v5)

Changes since v4:

  - changed |sendNfcErrorResponse| to take a string as second argument
  - replaced |sendPendingMessageError| by new |sendNfcErrorResponse|

Comment 54

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

Hi Sheriffs,

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

Comment 55

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

Trying...

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=aff886671aaa

There are no automated tests for NFC AFAIK, so I only enabled builds.

Comment 56

[Ryan VanderMeulen [:RyanVM]]

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

Comment 57

[Ryan VanderMeulen [:RyanVM]]

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