993891 - B2G NFC: NFC_IPC_MSG_NAMES in Nfc.js is handled twice.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

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

NFC_IPC_MSG_NAME in Nfc.js [1] is being handled twice in Nfc.js.

One is in gMessageManager.receiveMessage [2],
and the other is in Nfc.receiveMessage[3]

[1]: http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/Nfc.js?from=Nfc.js#47
[2]: http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/Nfc.js?from=Nfc.js#289
[3]: http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/Nfc.js?from=Nfc.js#530

Also I am not sure why two receiveMessage here.

Sidd, can you comment ?

Comment 1

[psiddh]

Here was the rationale to use separate receiveMessage().
 - gMessageManager.receiveMessage() is used for all the events that come from content process but do not talk to Gonk (nfcd). For example, this 'receiveMessage' will not handle  'NFC:WriteNDEF' as this event should talk to 'nfcd'.

 - Nfc.prototype's 'receiveMessage' handles messages from content process that need to talk to gonk(nfcd).

 - Moreover, generally messages 'NFC_IPC_PEER_MSG_NAMES' are meant for gMessageManager.receiveMessage and these messages need not be checked for 'session token sanity checks'.

 - Nfc.prototype's 'receiveMessage' generally handles 'NFC_IPC_MSG_NAMES' and should check for 'session token checks'. 


However, there is one exception to the above statements - 'NFC:SetSessionToken'. Note that since 'gMessageManager' object is used primarily by Nfc.js to do some book keeping of all <registered content targets , Session Token> for both peer and passive tag dom requests, 'NFC:SetSessionToken' message is handled in 'gMessageManager' receiveMessage.

Comment 2

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

Attachment: Patch.

Comment 3

[Dimi Lee [:dimi]]

Comment on attachment 8404410 [details] [diff] [review]
Patch.

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

::: dom/system/gonk/Nfc.js
@@ -548,5 @@
> -      case "NFC:ReadNDEF":
> -        if (!message.target.assertPermission("nfc-read")) {
> -          debug("NFC message " + message.name +
> -                " from a content process with no 'nfc-read' privileges.");
> -          this.sendNfcErrorResponse(message);

We may still need send this error response if permission check in gMessageManager fail ?

@@ -558,5 @@
> -      case "NFC:SendFile":
> -        if (!message.target.assertPermission("nfc-write")) {
> -          debug("NFC message " + message.name +
> -                " from a content process with no 'nfc-write' privileges.");
> -          this.sendNfcErrorResponse(message);

Ditto

Comment 4

[Dimi Lee [:dimi]]

Comment on attachment 8404410 [details] [diff] [review]
Patch.

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

Checked with Yoshi, sendNfcErrorResponse is not required not.
r+ this patch

Comment 5

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

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fcc7fa9049f3