Closed Bug 933585 Opened 6 years ago Closed 6 years ago

B2G NFC: Use Uint8Array for MozNdefRecord

Categories

(Firefox OS Graveyard :: NFC, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
1.3 Sprint 5 - 11/22

People

(Reporter: allstars.chh, Assigned: arno)

References

Details

(Whiteboard: [FT:RIL])

Attachments

(3 files, 7 obsolete files)

In Bug 674741 MozNdefRecord still uses DOMString for the payload, 
we should use Uint8Array for the payload.
Attachment #827670 - Flags: review?(khuey)
Attachment #827673 - Flags: review?(khuey)
Attached patch Patch (v1-3) NFC Worker changes (obsolete) — Splinter Review
Attachment #827677 - Flags: review?(allstars.chh)
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.
Attachment #827686 - Flags: review?(khuey)
Attachment #827695 - Flags: review?(alive)
Assignee: nobody → arno
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.
Attachment #827673 - Flags: review?(khuey) → review-
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.
Attachment #827686 - Flags: review?(khuey) → review-
Fix for tracing of heap objects.
Attachment #827673 - Attachment is obsolete: true
Attachment #828288 - Flags: review?(khuey)
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);
}
Attachment #827677 - Flags: review?(allstars.chh)
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.
Attachment #828288 - Flags: review?(khuey) → review+
When do we have nfc_manager.js already?
(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 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.
Attachment #827695 - Flags: review?(alive) → feedback+
(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.
(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
(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.
Fixed nit: limit to 80 characters per line
Attachment #827670 - Attachment is obsolete: true
Attachment #829589 - Flags: review?(khuey)
Fixed nits.
Attachment #828288 - Attachment is obsolete: true
Attachment #829594 - Flags: review?(khuey)
Attached patch Patch (v3-3) NFC Worker changes (obsolete) — Splinter Review
Fix nits.
Attachment #827677 - Attachment is obsolete: true
Attachment #829602 - Flags: review?(allstars.chh)
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.
Attachment #829602 - Flags: review?(allstars.chh) → review+
Attachment #829602 - Attachment is obsolete: true
Comment on attachment 827686 [details] [diff] [review]
Patch (v1-4) Fix for ObjectWrapper

This will be fixed by Bug 933497.
Attachment #827686 - Attachment is obsolete: true
Depends on: 933497
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.
Attachment #827695 - Attachment is obsolete: true
blocking-b2g: --- → 1.3+
Whiteboard: [FT:RIL]
It seems better to finish this bug before sprint5. If you think it isn't reasonable, please update target milestone.
Target Milestone: --- → 1.3 Sprint 5 - 11/22
NFC manager in Bug 860910 depends on this change. Hows the progress on Bug 933497?
Depends on: 860910
Blocks: 860910
No longer depends on: 860910
Summary: B2G NFC: Use Uint8Array for MozNdefRecord → B2G NFC: Use Uint8Array for MozNdefRecord r=khuey
Summary: B2G NFC: Use Uint8Array for MozNdefRecord r=khuey → B2G NFC: Use Uint8Array for MozNdefRecord
Attachment #829589 - Attachment description: Patch (v3-1) WebIDL changes for Uint8Array → Patch (v3-1) WebIDL changes for Uint8Array r=khuey
Attachment #829594 - Attachment description: Patch (v3-2) C++ implementation for WebIDL → Patch (v3-2) C++ implementation for WebIDL r=khuey
NFC is not a committed feature for 1.3, so this should not block the release.
blocking-b2g: 1.3+ → 1.3?
Pre-result post: try running: https://tbpl.mozilla.org/?tree=Try&rev=fa9e98f0f8ef
Keywords: checkin-needed
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)
Flags: needinfo?(ehung)
Keywords: checkin-needed
Whoops, wrong bug.
Flags: needinfo?(ehung)
NFC isn't a committed feature, so clearing nom.
blocking-b2g: 1.3? → ---
You need to log in before you can comment on or make changes to this bug.