Closed Bug 993891 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox OS Graveyard :: NFC, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
1.4 S5 (11apr)

People

(Reporter: allstars.chh, Assigned: allstars.chh)

References

Details

(Whiteboard: [p=2])

Attachments

(1 file)

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 ?
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.
Flags: needinfo?(psiddh)
Attached patch Patch.Splinter Review
Assignee: nobody → allstars.chh
Status: NEW → ASSIGNED
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 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
Attachment #8404410 - Flags: review?(dlee) → review+
Whiteboard: [p=2]
Target Milestone: --- → 1.4 S6 (25apr)
https://hg.mozilla.org/mozilla-central/rev/fcc7fa9049f3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: 1.4 S6 (25apr) → 1.4 S5 (11apr)
You need to log in before you can comment on or make changes to this bug.