933585 - B2G NFC: Use Uint8Array for MozNdefRecord

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

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

In Bug 674741 MozNdefRecord still uses DOMString for the payload, 
we should use Uint8Array for the payload.

Comment 1

[arno]

Attachment: Patch (v1-1) WebIDL changes for Uint8Array

Comment 2

[arno]

Attachment: Patch (v1-2) C++ implementation for WebIDL

Comment 3

[arno]

Attachment: Patch (v1-3) NFC Worker changes

Comment 4

[arno]

Attachment: Patch (v1-4) Fix for ObjectWrapper

This is a patch that Kyle generated for Bug 933497. Should be fixed properly in m-c but it is needed to make NFC work.

Comment 5

[arno]

Attachment: Patch (v1-5) NFC manager changes for Uint8Array

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 827673 [details] [diff] [review]
Patch (v1-2) C++ implementation for WebIDL

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

You need to trace your JS::Heap member variables.  See e.g. http://hg.mozilla.org/mozilla-central/annotate/ce0d975320e7/content/canvas/src/ImageData.cpp#l26  Don't forget to NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 827686 [details] [diff] [review]
Patch (v1-4) Fix for ObjectWrapper

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

Well, first of all I can't sign off on my own patch ;-)

But bholley wants to do something different here, so lets deal with this in Bug 933497.

Comment 8

[arno]

Attachment: Patch (v2-2) C++ implementation for WebIDL

Fix for tracing of heap objects.

Comment 9

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

Comment on attachment 827677 [details] [diff] [review]
Patch (v1-3) NFC Worker changes

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

Hi, Arno

Please re-format your patch:
- Update the title of your patch
  Remove the [PATCH] prefix, and add "Bug 933585: ...." 
- Use 8 lines of context
  if you're using git, use git format-patch -k -U8 commit to remove the '[PATCH]' and have 8 lines of context.

I think other patches also have this problem, please re-generate them again.

Please also address the nits and send r? to me again. 

Thanks.

::: dom/system/gonk/nfc_consts.js
@@ +16,4 @@
>  /* Copyright © 2013, Deutsche Telekom, Inc. */
>  
>  // Set to true to debug all NFC layers
> +this.DEBUG_ALL = true;

default to false

::: dom/system/gonk/nfc_worker.js
@@ +104,4 @@
>     * Unmarshals a NDEF message
>     */
>    unMarshallNdefMessage: function unMarshallNdefMessage() {
> +      let records = [];

indent is 2 spaces, not 4.
And try to allocate records if numOfRecords > 0,
as the original patch tried to do here.

@@ +108,5 @@
> +      let numOfRecords = Buf.readInt32();
> +      debug("numOfRecords = " + numOfRecords);
> +
> +      for (let i = 0; i < numOfRecords; i++) {
> +        let tnf        = Buf.readInt32();

Buf.readInt32() & 0xff;

@@ +187,1 @@
>        let typeLength = record.type.length;

record.type ? record.type.length : 0;

Same for the id and payload below

@@ +197,1 @@
>        let idLength = record.id.length;

here

@@ +207,1 @@
>        let payloadLength = record.payload && record.payload.length;

and here.

@@ +370,4 @@
>    let sessionId = Buf.readInt32();
>    let techCount = Buf.readInt32();
>    for (let count = 0; count < techCount; count++) {
> +    var t = Buf.readUint8();

nit, s/var/let/

@@ +372,5 @@
>    for (let count = 0; count < techCount; count++) {
> +    var t = Buf.readUint8();
> +    if (t in NFC_TECHS) {
> +      techs.push(NFC_TECHS[t]);
> +    }

Now the for-loop will run techCount * NFC_TECHS.length times.

Or simpler,

let tech = NFC_TECHS[Buf.readUint8()];
if (tech) {
  techs.push(tech);
}

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 828288 [details] [diff] [review]
Patch (v2-2) C++ implementation for WebIDL

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

r=me with nits fixed.

::: dom/nfc/MozNdefRecord.cpp
@@ +66,5 @@
>  /* static */
>  already_AddRefed<MozNdefRecord>
>  MozNdefRecord::Constructor(const GlobalObject& aGlobal,
> +  uint8_t aTnf, const Uint8Array& aType, const Uint8Array& aId, const Uint8Array& aPayload,
> +  ErrorResult& aRv)

nit: 80 char lines

@@ +74,5 @@
>      aRv.Throw(NS_ERROR_FAILURE);
>      return nullptr;
>    }
> +
> +  nsRefPtr<MozNdefRecord> ndefrecord = new MozNdefRecord(aGlobal.GetContext(), win, aTnf, aType, aId, aPayload);

here too

@@ +83,4 @@
>    return ndefrecord.forget();
>  }
>  
> +MozNdefRecord::MozNdefRecord(JSContext* aCx, nsPIDOMWindow* aWindow, uint8_t aTnf, const Uint8Array& aType, const Uint8Array& aId, const Uint8Array& aPayload)

and here

::: dom/nfc/MozNdefRecord.h
@@ +17,4 @@
>  
>  #include "nsIDocument.h"
>  
> +#include "mozilla/dom/MozNdefRecordBinding.h"

I think you can include this in the .cpp file.  There doesn't appear to be anything in the header that would need it.

@@ +36,4 @@
>  
>  public:
>  
> +  MozNdefRecord(JSContext* aCx, nsPIDOMWindow* aWindow, uint8_t aTnf, const Uint8Array& aType, const Uint8Array& aId, const Uint8Array& aPlayload);

nit: 80 char lines

@@ +48,4 @@
>    virtual JSObject* WrapObject(JSContext* aCx,
>                                 JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
>  
> +  static already_AddRefed<MozNdefRecord> Constructor(const GlobalObject& aGlobal, uint8_t aTnf, const Uint8Array& aType, const Uint8Array& aId, const Uint8Array& aPayload, ErrorResult& aRv);

here too.

Comment 11

[(inactive after 6/18) Alive Kuo [:alive]]

When do we have nfc_manager.js already?

Comment 12

[arno]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #9)
> Comment on attachment 827677 [details] [diff] [review]

Yoshi: I will do the reformatting. A couple of questions/comments:

> ::: dom/system/gonk/nfc_worker.js
> @@ +104,4 @@
> >     * Unmarshals a NDEF message
> >     */
> >    unMarshallNdefMessage: function unMarshallNdefMessage() {
> > +      let records = [];
> 
> And try to allocate records if numOfRecords > 0,
> as the original patch tried to do here.

numOfRecords should always be >= 1 for a valid NDEF Message, so I suggest to leave it as is.

> @@ +187,1 @@
> >        let typeLength = record.type.length;
> 
> record.type ? record.type.length : 0;

Should we impose that type, id, and payload always reference a Uint8Array, even if it is of length 0? This would make the overall code-base much more robust.

> @@ +372,5 @@
> >    for (let count = 0; count < techCount; count++) {
> > +    var t = Buf.readUint8();
> > +    if (t in NFC_TECHS) {
> > +      techs.push(NFC_TECHS[t]);
> > +    }
> 
> Now the for-loop will run techCount * NFC_TECHS.length times.

Would you mind explaining this? The statement "if (t in NFC_TECHS)..." should be optimized to an associative lookup; exactly what your proposed change does.

Comment 13

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 827695 [details] [diff] [review]
Patch (v1-5) NFC manager changes for Uint8Array

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

I can only give f+ since we don't have nfc_manager yet.

::: apps/system/js/nfc_manager.js
@@ +31,5 @@
>     * NDEF format
>     */
>    var nfc = {
> +  
> +    fromUTF8: function(str) {

Give function a name

@@ +39,5 @@
> +      }
> +      return buf;
> +    },
> +
> +    toUTF8: function(a) {

Give function a name

@@ +47,5 @@
> +      }
> +      return str;
> +    },
> +
> +    equalArrays: function(a1, a2) {

Give function a name

@@ +92,4 @@
>      uris: new Array(),
>  
>      init: function() {
> +      this.rtd_text = nfc.fromUTF8('T');

I don't understand why you are referring yourself but not using this.

Comment 14

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

(In reply to arno from comment #12)
> > ::: dom/system/gonk/nfc_worker.js
> > @@ +104,4 @@
> > >     * Unmarshals a NDEF message
> > >     */
> > >    unMarshallNdefMessage: function unMarshallNdefMessage() {
> > > +      let records = [];
> > 
> > And try to allocate records if numOfRecords > 0,
> > as the original patch tried to do here.
> 
> numOfRecords should always be >= 1 for a valid NDEF Message, so I suggest to
> leave it as is.
> 

You're the NFC expert here, however for many people like me, are not. When we are looking into this patch, we don't know even if the NFC Forum has defined numRecords MUST > 1, nor we know if the daemon will actually send a NdefMessage with 0 records inside.

Even one day nfc_worker does received a NdefMessage with 0 record, the comment I gave will return null, but your original patch will return [], which when we try to read ndefRecords[0].type will throw TypeError cause some problems and some time to debug.


> > @@ +187,1 @@
> > >        let typeLength = record.type.length;
> > 
> > record.type ? record.type.length : 0;
> 
> Should we impose that type, id, and payload always reference a Uint8Array,
> even if it is of length 0? This would make the overall code-base much more
> robust.
> 
The check is the same reason for above, 
if record.type is null, record.type.length will throw ReferenceError.

This function (writeNDEF) is called from Nfc.js,
I don't know if Nfc.js or somewhere else has done the check, but if they don't,
nfc_worker will throw here and we might take a few hours to find out here.

I propose to make the patch robustive.

> > @@ +372,5 @@
> > >    for (let count = 0; count < techCount; count++) {
> > > +    var t = Buf.readUint8();
> > > +    if (t in NFC_TECHS) {
> > > +      techs.push(NFC_TECHS[t]);
> > > +    }
> > 
> > Now the for-loop will run techCount * NFC_TECHS.length times.
> 
> Would you mind explaining this? The statement "if (t in NFC_TECHS)..."
> should be optimized to an associative lookup; exactly what your proposed
> change does.

Yes, but I propose to make the code more clear.
I don't know how much time JS Engine will spend on the if ( in )
But if our patch could reduce effort for JS Engine and make the code easier and more clear.
We should do it.

Comment 15

[arno]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #14)
> (In reply to arno from comment #12)
> > > ::: dom/system/gonk/nfc_worker.js
> > > @@ +104,4 @@
> > > And try to allocate records if numOfRecords > 0,
> > > as the original patch tried to do here.
> > 
> > numOfRecords should always be >= 1 for a valid NDEF Message, so I suggest to
> > leave it as is.
> > 
> 
> You're the NFC expert here, however for many people like me, are not. When
> we are looking into this patch, we don't know even if the NFC Forum has
> defined numRecords MUST > 1, nor we know if the daemon will actually send a
> NdefMessage with 0 records inside.

The NDEF message that is unmarshalled is sent by nfcd, therefore it must be a valid NDEF message with at least one NDEF record. Assume that the payload read by nfcd contains an invalid NDEF message then nfcd cannot even send an NDEF message because it will fail to parse it.

> Even one day nfc_worker does received a NdefMessage with 0 record, the
> comment I gave will return null, but your original patch will return [],
> which when we try to read ndefRecords[0].type will throw TypeError cause
> some problems and some time to debug.

Someone familiar with NDEF will do exactly that: ndefRecords[0].type and will not guard for ndefRecords[0].type == undefined

> > > @@ +187,1 @@
> > > >        let typeLength = record.type.length;
> > > 
> > > record.type ? record.type.length : 0;
> > 
> > Should we impose that type, id, and payload always reference a Uint8Array,
> > even if it is of length 0? This would make the overall code-base much more
> > robust.
> > 
> The check is the same reason for above, 
> if record.type is null, record.type.length will throw ReferenceError.

The code here will get its parameters via the MozNdefRecord WebIDL so again it cannot be null since the WebIDL forces a Uint8Array parameter.

> > > @@ +372,5 @@
> > > >    for (let count = 0; count < techCount; count++) {
> > > > +    var t = Buf.readUint8();
> > > > +    if (t in NFC_TECHS) {
> > > > +      techs.push(NFC_TECHS[t]);
> > > > +    }
> > > 
> > > Now the for-loop will run techCount * NFC_TECHS.length times.
> > 
> > Would you mind explaining this? The statement "if (t in NFC_TECHS)..."
> > should be optimized to an associative lookup; exactly what your proposed
> > change does.
> 
> Yes, but I propose to make the code more clear.
> I don't know how much time JS Engine will spend on the if ( in )

so you are suggesting to 'dumb down' the code in case the JS engine does not do a good job?

You and I have a different philosophy: adding unnecessary guards and 'dumbing down' code makes the code less readable and IMHO less robust. But since you are the reviewer I will comply with your wishes.

Arno

Comment 16

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

(In reply to arno from comment #15)

>...
I am okay if you think WebIDL and daemon should do the check, so in Gecko Impl you don't have to worry about those conditions violated specification.

Just a reminder for RIL we do have some bugs caused by ReferenceError, TypeError, ... before.
I just hope you don't have to suffer this again.

> so you are suggesting to 'dumb down' the code in case the JS engine does not
> do a good job?
> 
Yes, we even try not to create too many objects even JS could do good jobs in GC.

You can see in WebNfc I asked Sidd try to reuse the message objects.

Comment 17

[arno]

Attachment: Patch (v3-1) WebIDL changes for Uint8Array r=khuey

Fixed nit: limit to 80 characters per line

Comment 18

[arno]

Attachment: Patch (v3-2) C++ implementation for WebIDL r=khuey

Fixed nits.

Comment 19

[arno]

Attachment: Patch (v3-3) NFC Worker changes

Fix nits.

Comment 20

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

Comment on attachment 829602 [details] [diff] [review]
Patch (v3-3) NFC Worker changes

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

Add r=me

::: dom/system/gonk/nfc_consts.js
@@ +15,5 @@
>  
>  /* Copyright © 2013, Deutsche Telekom, Inc. */
>  
>  // Set to true to debug all NFC layers
> +this.DEBUG_ALL = true;

default to false.

Comment 21

[arno]

Attachment: Patch (v4-3) NFC Worker changes r=allstars.chh

Comment 22

[arno]

Comment on attachment 827686 [details] [diff] [review]
Patch (v1-4) Fix for ObjectWrapper

This will be fixed by Bug 933497.

Comment 23

[arno]

Comment on attachment 827695 [details] [diff] [review]
Patch (v1-5) NFC manager changes for Uint8Array

Garner will add the Uint8Array changes to his patch for nfc_manager.js.

Comment 24

[Ken Chang[:kenkai]]

It seems better to finish this bug before sprint5. If you think it isn't reasonable, please update target milestone.

Comment 25

[Garner Lee]

NFC manager in Bug 860910 depends on this change. Hows the progress on Bug 933497?

Comment 26

[Jason Smith [:jsmith]]

NFC is not a committed feature for 1.3, so this should not block the release.

Comment 27

[Garner Lee]

Pre-result post: try running: https://tbpl.mozilla.org/?tree=Try&rev=fa9e98f0f8ef

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/374158c7f9a6
https://hg.mozilla.org/integration/b2g-inbound/rev/b237796ae6e5
https://hg.mozilla.org/integration/b2g-inbound/rev/e10be8b04544

Comment 29

[Garner Lee]

Gecko side webidl: check-in needed.

Travis-CI tests: keyboard test bugged, but Settings UI change is unlikely to be related: https://travis-ci.org/mozilla-b2g/gaia/builds/15023166 (re-running anyway)

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/374158c7f9a6
https://hg.mozilla.org/mozilla-central/rev/b237796ae6e5
https://hg.mozilla.org/mozilla-central/rev/e10be8b04544

Comment 31

[Garner Lee]

Whoops, wrong bug.

Comment 32

[Jason Smith [:jsmith]]

NFC isn't a committed feature, so clearing nom.