1000839 - NFC: NfcUtils.encodeNDEF is not used and it's buggy.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

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

I spot some potentail problem could happen in NfcUtils.encodeNDEF.

type and id in NDEFRecord could be null, so type.length or id.length could throw TypeError.

Comment 1

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

When I am working on this I found something even worse.

From NDEF spec, clause 3.2.9, the payloadLength will be of 4 bytes when SR=0, and the transmission order is MSB-first (Big-Endian).

However current encodeNDEF in nfc_utils.js is ......Little-Endian.

// codesnippet
      b.push(payloadLen & 0xff);
      if (payloadLen > 0xff) {
        for (i = 0; i < 3; i++) {
          payloadLen >>>= 8;
          b.push(payloadLen & 0xff);
        }
      }

We didn't discover this problem before because it hasn't been used at all.

Comment 2

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

De-assign myself as this method isn't used at all.
Right now I don't know what's the purpose of this function.

Comment 3

[Kamil Leszczuk [:kamituel]]

I will be using this function in unit tests for 1021267. Also, if we plan to implement NDEF writing at some point, it could be useful then as well.

So, I'll fix and unit test it, leaving it in NfcUtils as it is today.

Comment 4

[Kamil Leszczuk [:kamituel]]

Attachment: Pull request

Fixed endianess, added proper unit tests and small refactor for encodeNDEF().

Comment 5

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8450007 [details] [review]
Pull request

I think it's possible to make code more clear. Feel free to set review again if you fixed them or you've any opinion, thanks.

Comment 6

[Kamil Leszczuk [:kamituel]]

Comment on attachment 8450007 [details] [review]
Pull request

At first my goal was to only fix bug discovered by Yoshi, but sure, I can rewrite the whole method ;)

This time I've changed a bit more - fixing almost all your remarks. The one left is the usage of for loop to encode payload length field, but I rewrote it not to use temporary array, so it's okay.

Comment 7

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8450007 [details] [review]
Pull request

Okay, review+ is here. Thank you very much!

Comment 8

[Kamil Leszczuk [:kamituel]]

Thanks!

Comment 9

[Carsten Book [:Tomcat]]

seems the travis build failed - https://travis-ci.org/mozilla-b2g/gaia/builds/29158411 - could you take a look at that ?

Comment 10

[Kamil Leszczuk [:kamituel]]

Travis failed with message:

> mkdir: cannot create directory `xulrunner-sdk-30': File exists

This pull request modifies only two files, fixing one method and tests for it, and this method isn't used anywhere in the Gaia yet. So it's pretty much impossible for it to break build.

So I'd say I'm 99.99% positivie this patch has nothing to do with Travis failing. I have no permissions to rerun Travis, unfortunately.

Comment 11

[Kamil Leszczuk [:kamituel]]

I'm not sure what action should I do when Travis fails like that?

But if it's okay, please merge.

Comment 12

[Carsten Book [:Tomcat]]

(In reply to Kamil Leszczuk [:kamituel] from comment #11)
> I'm not sure what action should I do when Travis fails like that?
> 
> But if it's okay, please merge.

seems this was related also to some timeouts.

merged now into master

https://github.com/mozilla-b2g/gaia/commit/e86a7a32f4c5fba45714bca4b021cc5182162530