B2G NFC: Rewrite NFC Worker in C++

RESOLVED FIXED in 2.1 S1 (1aug)

Status

Firefox OS
NFC
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: allstars, Assigned: allstars)

Tracking

(Blocks: 2 bugs)

unspecified
2.1 S1 (1aug)
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(tracking-b2g:backlog)

Details

(Whiteboard: [p=13])

Attachments

(3 attachments, 22 obsolete attachments)

1.25 KB, patch
dimi
: review+
Details | Diff | Splinter Review
38.13 KB, patch
khuey
: review+
Details | Diff | Splinter Review
31.26 KB, patch
allstars
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Updated

4 years ago
Assignee: nobody → allstars.chh

Updated

4 years ago
Blocks: 937504
(Assignee)

Updated

3 years ago
Depends on: 958423
(Assignee)

Updated

3 years ago
Depends on: 959064
(Assignee)

Comment 1

3 years ago
Created attachment 8374733 [details] [diff] [review]
Part 1: NFC Service
(Assignee)

Comment 2

3 years ago
Created attachment 8374734 [details] [diff] [review]
Part 2: NFC Message Handler
(Assignee)

Comment 3

3 years ago
memory diff between without/with nfc_worker

Main Process (pid NNN)
Explicit Allocations

-0.72 MB (100.0%) -- explicit
├──-1.04 MB (143.77%) -- workers/workers()
│  ├──-1.01 MB (139.86%) -- worker(resource://gre/modules/nfc_worker.js, 0xNNN)
│  │  ├──-0.32 MB (44.94%) -- runtime
│  │  │  ├──-0.11 MB (15.03%) ── script-data [-]
│  │  │  ├──-0.06 MB (08.65%) ── atoms-table [-]
│  │  │  ├──-0.06 MB (08.65%) -- code
│  │  │  │  ├──-0.04 MB (06.20%) ── other [-]
│  │  │  │  ├──-0.01 MB (01.73%) ── unused [-]
│  │  │  │  └──-0.01 MB (00.72%) ── baseline [-]
│  │  │  ├──-0.04 MB (05.41%) ── runtime-object [-]
│  │  │  ├──-0.02 MB (03.32%) ── script-sources [-]
│  │  │  ├──-0.02 MB (02.16%) ── gc/marker
│  │  │  └──-0.01 MB (01.73%) ++ (4 tiny)
│  │  ├──-0.26 MB (36.00%) -- zone(0xb0188800)
│  │  │  ├──-0.19 MB (26.40%) -- compartment(web-worker)
│  │  │  │  ├──-0.08 MB (11.34%) -- shapes
│  │  │  │  │  ├──-0.05 MB (07.55%) -- gc-heap
│  │  │  │  │  │  ├──-0.04 MB (05.17%) -- tree
│  │  │  │  │  │  │  ├──-0.03 MB (04.05%) ── global-parented [-]
│  │  │  │  │  │  │  └──-0.01 MB (01.11%) ── non-global-parented [-]
│  │  │  │  │  │  └──-0.02 MB (02.39%) ── base [-]
│  │  │  │  │  └──-0.03 MB (03.78%) ── malloc-heap/compartment-tables
│  │  │  │  ├──-0.08 MB (10.56%) -- objects
│  │  │  │  │  ├──-0.05 MB (07.35%) -- gc-heap
│  │  │  │  │  │  ├──-0.03 MB (04.67%) ── function [-]
│  │  │  │  │  │  └──-0.02 MB (02.68%) ── ordinary [-]
│  │  │  │  │  └──-0.02 MB (03.21%) ── malloc-heap/slots
│  │  │  │  ├──-0.02 MB (02.96%) -- sundries
│  │  │  │  │  ├──-0.02 MB (02.38%) ── malloc-heap [-]
│  │  │  │  │  └──-0.00 MB (00.58%) ── gc-heap [-]
│  │  │  │  └──-0.01 MB (01.54%) ── scripts/gc-heap
│  │  │  ├──-0.06 MB (08.68%) ── unused-gc-things [-]
│  │  │  └──-0.01 MB (00.92%) ++ sundries
│  │  ├──-0.20 MB (28.13%) -- zone(0xb159cc00)
│  │  │  ├──-0.15 MB (21.43%) -- compartment(web-worker)
│  │  │  │  ├──-0.06 MB (07.84%) -- objects
│  │  │  │  │  ├──-0.04 MB (05.75%) -- gc-heap
│  │  │  │  │  │  ├──-0.03 MB (03.71%) ── function [-]
│  │  │  │  │  │  └──-0.01 MB (02.03%) ── ordinary [-]
│  │  │  │  │  └──-0.02 MB (02.09%) ── malloc-heap/slots
│  │  │  │  ├──-0.05 MB (06.45%) -- shapes
│  │  │  │  │  ├──-0.03 MB (04.55%) -- gc-heap
│  │  │  │  │  │  ├──-0.02 MB (03.27%) ── tree/global-parented
│  │  │  │  │  │  └──-0.01 MB (01.28%) ── dict [-]
│  │  │  │  │  └──-0.01 MB (01.89%) ── malloc-heap/compartment-tables
│  │  │  │  ├──-0.03 MB (03.75%) ── scripts/gc-heap
│  │  │  │  └──-0.02 MB (03.40%) -- sundries
│  │  │  │     ├──-0.01 MB (01.99%) ── malloc-heap [-]
│  │  │  │     └──-0.01 MB (01.41%) ── gc-heap [-]
│  │  │  ├──-0.05 MB (06.37%) ── unused-gc-things [-]
│  │  │  └──-0.00 MB (00.33%) ── sundries/gc-heap
│  │  ├──-0.19 MB (26.46%) -- zone(0xb159b800)
│  │  │  ├──-0.17 MB (23.55%) -- strings
│  │  │  │  ├──-0.14 MB (19.10%) -- normal
│  │  │  │  │  ├──-0.09 MB (12.28%) ── gc-heap [-]
│  │  │  │  │  └──-0.05 MB (06.82%) ── malloc-heap [-]
│  │  │  │  └──-0.03 MB (04.45%) ── short-gc-heap [-]
│  │  │  ├──-0.01 MB (01.50%) ── unused-gc-things [-]
│  │  │  ├──-0.01 MB (01.13%) ── jit-codes-gc-heap [-]
│  │  │  └──-0.00 MB (00.28%) ++ (2 tiny)
│  │  └──-0.03 MB (04.32%) ── gc-heap/chunk-admin
│  └──-0.03 MB (03.91%) -- worker(resource://gre/modules/ril_worker.js, 0xNNN)
│     ├──-0.59 MB (82.01%) -- zone(0xb1542000)
│     │  ├──-0.52 MB (72.10%) -- compartment(web-worker)
│     │  │  ├──-0.20 MB (27.75%) -- objects
│     │  │  │  ├──-0.11 MB (14.63%) -- gc-heap
│     │  │  │  │  ├──-0.07 MB (09.26%) ── function [-]
│     │  │  │  │  └──-0.04 MB (05.37%) ── ordinary [-]
│     │  │  │  └──-0.09 MB (13.12%) -- malloc-heap
│     │  │  │     ├──-0.05 MB (07.54%) ── elements/non-asm.js
│     │  │  │     └──-0.04 MB (05.57%) ── slots [-]
│     │  │  ├──-0.18 MB (25.03%) -- shapes
│     │  │  │  ├──-0.11 MB (15.82%) -- gc-heap
│     │  │  │  │  ├──-0.06 MB (08.44%) -- tree
│     │  │  │  │  │  ├──-0.05 MB (06.67%) ── global-parented [-]
│     │  │  │  │  │  └──-0.01 MB (01.77%) ── non-global-parented [-]
│     │  │  │  │  ├──-0.04 MB (04.88%) ── dict [-]
│     │  │  │  │  └──-0.02 MB (02.50%) ── base [-]
│     │  │  │  └──-0.07 MB (09.21%) -- malloc-heap
│     │  │  │     ├──-0.03 MB (03.78%) ── compartment-tables [-]
│     │  │  │     ├──-0.02 MB (02.49%) ── tree-tables [-]
│     │  │  │     ├──-0.01 MB (01.74%) ── tree-shape-kids [-]
│     │  │  │     └──-0.01 MB (01.20%) ── dict-tables [-]
│     │  │  ├──-0.11 MB (14.92%) -- scripts
│     │  │  │  ├──-0.09 MB (11.88%) ── gc-heap [-]
│     │  │  │  └──-0.02 MB (03.04%) ── malloc-heap/data
│     │  │  ├──-0.02 MB (02.30%) ── baseline/stubs/fallback
│     │  │  └──-0.02 MB (02.11%) -- sundries
│     │  │     ├──-0.01 MB (01.77%) ── malloc-heap [-]
│     │  │     └──-0.00 MB (00.34%) ── gc-heap [-]
│     │  ├──-0.06 MB (08.54%) ── unused-gc-things [-]
│     │  └──-0.01 MB (01.37%) -- sundries
│     │     ├──-0.01 MB (01.23%) ── gc-heap [-]
│     │     └──-0.00 MB (00.13%) ── malloc-heap [-]
│     └───0.56 MB (-78.09%) ++ (4 tiny)
└───0.32 MB (-43.77%) -- (7 tiny)
    ├──0.43 MB (-59.29%) ── heap-unclassified
    ├──-0.11 MB (15.12%) -- heap-overhead
    │  ├──-0.15 MB (20.54%) ── page-cache
    │  └───0.04 MB (-5.42%) ── waste
    ├──-0.00 MB (00.54%) ++ window-objects/top(app://system.gaiamobile.org/index.html, id=3)
    ├──0.00 MB (-0.12%) ++ js-non-window
    ├──0.00 MB (-0.03%) ++ xpconnect
    ├──-0.00 MB (00.01%) ── freetype
    └──0.00 MB (-0.01%) ── xpcom/component-manager

Other Measurements

0.00 MB (100.0%) -- blob-urls
└──0.00 MB (100.0%) -- owner(app://system.gaiamobile.org/index.html)
   ├──-0.12 MB (100.0%) ++ (5 tiny)
   └──0.12 MB (100.0%) ── blob:0fa63164-bc26-4529-8d15-3ed21457416e [+]

-1.47 MB (100.0%) -- decommitted
├──-1.48 MB (100.27%) ── workers/workers()/worker(resource://gre/modules/nfc_worker.js, 0xNNN)/gc-heap/decommitted-arenas
└───0.00 MB (-0.27%) ── js-non-window/gc-heap/decommitted-arenas

-0.00 MB (100.0%) -- js-main-runtime
├──-0.00 MB (115.63%) -- zones
│  ├──-0.00 MB (126.05%) ── unused-gc-things
│  └───0.00 MB (-10.42%) ++ (2 tiny)
└───0.00 MB (-15.63%) ++ (2 tiny)

0 (100.0%) -- js-main-runtime-compartments
└──0 (100.0%) ++ user

-0.00 MB (100.0%) -- js-main-runtime-gc-heap-committed
├──-0.00 MB (99.22%) ── unused/gc-things
└──-0.00 MB (00.78%) ── used/arena-admin

0.00 MB (100.0%) -- window-objects
└──0.00 MB (100.0%) ++ layout

-0.02 MB ── heap-allocated
-0.13 MB ── heap-committed
  -0.39% ── heap-overhead-ratio
     -10 ── page-faults-hard
  -3,481 ── page-faults-soft
-1.57 MB ── resident
-0.81 MB ── resident-unique
-1.17 MB ── vsize
(Assignee)

Comment 4

3 years ago
Created attachment 8375404 [details] [diff] [review]
Part 1: NFC Service
Attachment #8374733 - Attachment is obsolete: true
(Assignee)

Comment 5

3 years ago
Created attachment 8375405 [details] [diff] [review]
Part 2: NFC Message Handler
Attachment #8374734 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8375404 - Flags: review?(fabrice)
(Assignee)

Updated

3 years ago
Attachment #8375405 - Flags: review?(fabrice)
Attachment #8375405 - Flags: review?(dlee)
(Assignee)

Comment 6

3 years ago
Comment on attachment 8375405 [details] [diff] [review]
Part 2: NFC Message Handler

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

::: dom/system/gonk/nfc_consts.js
@@ -23,4 @@
>  this.DEBUG_CONTENT_HELPER = false || DEBUG_ALL;
>  this.DEBUG_NFC = false || DEBUG_ALL;
>  
>  // Current version

Below consts are used by nfc_worker.js,
I'll also remove these.

::: dom/system/gonk/nfc_worker.js
@@ -357,5 @@
> -  let status       = Buf.readInt32();
> -  let majorVersion = Buf.readInt32();
> -  let minorVersion = Buf.readInt32();
> -  debug("NFC_NOTIFICATION_INITIALIZED status:" + status);
> -  if ((majorVersion != NFC_MAJOR_VERSION) || (minorVersion != NFC_MINOR_VERSION)) {

I'll also check the versions in NfcMessageHandler.
Comment on attachment 8375405 [details] [diff] [review]
Part 2: NFC Message Handler

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

::: dom/system/gonk/NfcMessageHandler.cpp
@@ +58,5 @@
> +  }
> +
> +  return result;
> +}
> +

mPendingReq do not have to be a queue ?

@@ +139,5 @@
> +  aOptions.mType = NS_ConvertUTF8toUTF16(kConfigResponse);
> +  aOptions.mStatus = aParcel.readInt32();
> +  return true;
> +}
> +

Suggest for request functions, use const reference for CommandOptions,
for response functions, use const reference for Parcel.

::: dom/system/gonk/NfcOptions.h
@@ +49,5 @@
> +
> +  if (aOther.mRecords.WasPassed()) {
> +    mozilla::dom::Sequence<mozilla::dom::NDEFRecord> const & currentValue = aOther.mRecords.InternalValue();
> +    uint32_t length = currentValue.Length();
> +    mRecordCount = currentValue.Length();

You can use length directly here

::: dom/webidl/NfcOptions.webidl
@@ +40,5 @@
> +
> +  boolean isReadOnly;
> +  boolean canBeMadeReadOnly;
> +  long maxSupportedLength;
> +};

Maybe we will need to add those into DummyBinding.webidl ?
Attachment #8375405 - Flags: review?(dlee)
(Assignee)

Comment 8

3 years ago
Created attachment 8376969 [details] [diff] [review]
Part 2: NFC Message Handler v2

Addressed Dimi's comments/
Attachment #8375405 - Attachment is obsolete: true
Attachment #8375405 - Flags: review?(fabrice)
(Assignee)

Updated

3 years ago
Attachment #8376969 - Flags: review?(fabrice)
Attachment #8376969 - Flags: review?(dlee)
(Assignee)

Comment 9

3 years ago
(In reply to Dimi Lee[:dimi][:dlee] from comment #7)
> ::: dom/webidl/NfcOptions.webidl
> @@ +40,5 @@
> > +
> > +  boolean isReadOnly;
> > +  boolean canBeMadeReadOnly;
> > +  long maxSupportedLength;
> > +};
> 
> Maybe we will need to add those into DummyBinding.webidl ?

DummyBindings is not neccesary anymore, see Bug 968665.

Updated

3 years ago
Attachment #8376969 - Flags: review?(dlee) → review+
Comment on attachment 8375404 [details] [diff] [review]
Part 1: NFC Service

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

::: dom/system/gonk/Nfc.js
@@ +434,5 @@
>     *        A text message type.
>     * @param message [optional]
>     *        An optional message object to send.
>     */
>    sendToWorker: function sendToWorker(nfcMessageType, message) {

nit: this could be renamed since we have no real worker anymore.

::: dom/system/gonk/NfcService.cpp
@@ +146,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  nsCOMPtr<nsIRunnable> runnable = new WorkerRunnable();
> +  mWorkerThread->Dispatch(runnable, nsIEventTarget::DISPATCH_NORMAL);

I'm probably missing something, but WorkerRunnable() does nothing in it's Run() method so how are we sending the aOptions around? (we're not using this parameter anywhere).
Attachment #8375404 - Flags: review?(fabrice)
Comment on attachment 8376969 [details] [diff] [review]
Part 2: NFC Message Handler v2

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

Yoshi, I don't know the nfc protocols and api well enough to review that.
Attachment #8376969 - Flags: review?(fabrice)
(Assignee)

Comment 12

3 years ago
(In reply to Fabrice Desré [:fabrice] from comment #10)
> ::: dom/system/gonk/NfcService.cpp
> @@ +146,5 @@
> > +{
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +
> > +  nsCOMPtr<nsIRunnable> runnable = new WorkerRunnable();
> > +  mWorkerThread->Dispatch(runnable, nsIEventTarget::DISPATCH_NORMAL);
> 
> I'm probably missing something, but WorkerRunnable() does nothing in it's
> Run() method so how are we sending the aOptions around? (we're not using
> this parameter anywhere).

Hi Fabrice:
aOptions passing is actually done in Part 2, because it involves marshalling/unmarshalling NFC binary data.

My original Part 1 is focus on how the data is transfered between Nfc.js (Main Thread on Chrome Process) to nfcd.

I'll reformat these two patches to make them more clearer,
also I will split marshalling/unmarshalling part into another part and let Dimi to review it.

Thanks for your comment.
(Assignee)

Comment 13

3 years ago
Created attachment 8378113 [details] [diff] [review]
Part 1: NFC Service v2.

rename sendToWorker to sendToNfcService by fabrice's comment.

Also move some code from Part2 to Part 1.
Attachment #8375404 - Attachment is obsolete: true
(Assignee)

Comment 14

3 years ago
Comment on attachment 8378113 [details] [diff] [review]
Part 1: NFC Service v2.

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

Hi, Fabrice

This patch is adjusted by your last comment.

Also this code uses the same pattern used in "rewrite wifi_worker in C++" and "rewrite net_worker in C++"
And I've moved the NFCD protocol things to Part 2.
I hope the patch here won't be too complicated for you to review.

Can you kindly help to review it?
If you need me to split into more parts please let me know.

Thanks
Attachment #8378113 - Flags: review?(fabrice)
(Assignee)

Comment 15

3 years ago
Created attachment 8378115 [details] [diff] [review]
Part 2: NFC Message Handler v3

This is the same patch with Part 2. v2.

Just I leave only NFCD protcol specific code in this part, and move NfcService related code to Part 1 as Fabrice commented.
Attachment #8376969 - Attachment is obsolete: true
Attachment #8378115 - Flags: review+
(Assignee)

Updated

3 years ago
Depends on: 960510
(Assignee)

Comment 16

3 years ago
fabrice, review ping?
(In reply to Yoshi Huang[:allstars.chh] from comment #16)
> fabrice, review ping?

I'm really sorry Yoshi, I had no time to look at that and I should have notified you. I'll try to do it this week, but if you have other reviewers don't block on me.
(Assignee)

Comment 18

3 years ago
Comment on attachment 8378113 [details] [diff] [review]
Part 1: NFC Service v2.

No worries, Fabrice.

Hi, Kyle
Could you help to review this for me?
Or if you don't have time, please let me know, I'll find another reviewer. :)

Thanks
Attachment #8378113 - Flags: review?(fabrice) → review?(khuey)
I might be able to review this on my long flight this week.

Updated

3 years ago
Blocks: 949293
Comment on attachment 8378113 [details] [diff] [review]
Part 1: NFC Service v2.

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

::: dom/system/gonk/NfcMessageHandler.cpp
@@ +11,5 @@
> +bool
> +NfcMessageHandler::Marshall(Parcel& aParcel, const CommandOptions& aOptions)
> +{
> +  bool result;
> +  return result;

When you land this you should squash these two patches together.  Obviously this doesn't work by itself ;-)

::: dom/system/gonk/NfcOptions.h
@@ +1,1 @@
> +#ifndef NfcOptions_h

No license header?

@@ +9,5 @@
> +struct NDEFRecordStruct
> +{
> +  uint8_t mTnf;
> +  uint32_t mTypeLength;
> +  nsAutoArrayPtr<uint8_t> mType;

This might be cleaner if you used nsTArray<uint8_t> to replace mFoo and mFooLength.

@@ +27,5 @@
> +  if (aOther.prop.WasPassed()) {                      \
> +    prop = aOther.prop.Value();                       \
> +  } else {                                            \
> +    prop = defaultValue;                              \
> +  }

I think we should add a method to Optional<T> that returns Value() if it was passed or else a provided default value.  Perhaps const T& ValueOrDefault(const T&)?

@@ +38,5 @@
> +      prop = aOther.prop.Value();                     \
> +    }                                                 \
> +  } else {                                            \
> +    prop = defaultValue;                              \
> +  }

Why are you handling null like this?  This seems like something that should either happen at the WebIDL layer or not at all.

@@ +46,5 @@
> +  COPY_OPT_STRING_FIELD(mRequestId, EmptyString())
> +  COPY_OPT_FIELD(mPowerLevel, 0)
> +  COPY_OPT_FIELD(mTechType, 0)
> +
> +  if (aOther.mRecords.WasPassed()) {

Seems like we should be writing an operator= for something instead of all this code.

@@ +105,5 @@
> +  int32_t mMinorVersion;
> +  int32_t mTechListLength;
> +  nsAutoArrayPtr<uint8_t> mTechList;
> +  int32_t mRecordCount;
> +  nsAutoArrayPtr<NDEFRecordStruct> mRecords;

Same things here about using nsTArray.

::: dom/system/gonk/NfcService.cpp
@@ +37,5 @@
> +
> +namespace mozilla {
> +namespace nfc {
> +
> +static StaticRefPtr<NfcService> gNfcService;

You can just use a raw pointer here.  The XPCOM service manager will hold a strong reference to your service until XPCOM shutdown.

@@ +42,5 @@
> +
> +NS_IMPL_ISUPPORTS1(NfcService, nsINfcService)
> +
> +// Runnable used to call SendCommand on the worker thread.
> +class WorkerRunnable : public nsRunnable

Please give this a more descriptive name.  Maybe NfcCommandRunnable?

@@ +85,5 @@
> +    JS::Rooted<JS::Value> value(cx);
> +    RootedDictionary<NfcEventOptions> event(cx);
> +    if (!event.Init(cx, value)) {
> +      return NS_ERROR_TYPE_ERR;
> +    }

Why do you need to call Init at all?  IIRC that is only for constructing a dictionary from a JS value or JSON string.

@@ +112,5 @@
> +      }
> +
> +      for (int i = 0; i < mEvent.mTechListLength; i++) {
> +        nsString& elem = *event.mTechList.Value().AppendElement();
> +        elem = NS_ConvertUTF8toUTF16(NfcTechString[mEvent.mTechList[i]]);

Where are these parameters validated?  What stops mEvent.mTechList[i] from being larger than the length of NfcTechString?  I can't find the code that does that, but even if it does exist, you should add an assertion here for the length.

Rather than converting them to wide strings you should just store them as wide strings.

@@ +224,5 @@
> +NS_IMETHODIMP
> +NfcService::Start(nsINfcEventListener* aListener)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aListener);

You should also assert that we have not been started already.

@@ +226,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aListener);
> +
> +  nsresult rv = NS_NewThread(getter_AddRefs(mWorkerThread));

Please use NS_NewNamedThread and give this a useful name.

@@ +240,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +NfcService::Shutdown()

What calls this?

@@ +279,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  mozilla::AutoSafeJSContext cx;
> +  JS::RootedValue val(cx);

You need to enter a compartment here.  But it's not actually clear how to get the right compartment (of mListener) here :(

I think maybe you should make the listener a WebIDL callback instead of an XPIDL one, and then you could just let the WebIDL binding layer deal with compartments for you.

::: dom/system/gonk/NfcService.h
@@ +4,5 @@
> +
> +#ifndef NfcService_h
> +#define NfcService_h
> +
> +#include "mozilla/dom/NfcOptionsBinding.h"

Forward declare mozilla::dom::NfcEventOptions.

@@ +9,5 @@
> +#include "mozilla/ipc/Nfc.h"
> +#include "mozilla/ipc/UnixSocket.h"
> +#include "nsCOMPtr.h"
> +#include "nsINfcService.h"
> +#include "nsThread.h"

and nsIThread

@@ +13,5 @@
> +#include "nsThread.h"
> +#include "NfcMessageHandler.h"
> +
> +namespace mozilla {
> +namespace nfc {

Drop the nfc namespace.  You're already prefixed the classes with Nfc.
Attachment #8378113 - Flags: review?(khuey) → review-
blocking-b2g: --- → backlog
Target Milestone: --- → 2.0 S1 (9may)
(Assignee)

Updated

3 years ago
Target Milestone: 2.0 S1 (9may) → 2.0 S2 (23may)
(Assignee)

Updated

3 years ago
Whiteboard: [p=8]
(Assignee)

Comment 21

3 years ago
Created attachment 8423605 [details] [diff] [review]
Part 1: NFC Service v3.
Attachment #8378113 - Attachment is obsolete: true
(Assignee)

Comment 22

3 years ago
Created attachment 8423606 [details] [diff] [review]
Part 2: NFC Message Handler v4

Modified after Part 1 changed.
I'll ask r? to Dimi again when Part 1 is r+.
Attachment #8378115 - Attachment is obsolete: true
(Assignee)

Comment 23

3 years ago
Comment on attachment 8423605 [details] [diff] [review]
Part 1: NFC Service v3.

Hi, Kyle
Thanks for the previous review, and sorry for send another r? so late,
I was interrupted by other NFC bugs :( 

I have addresed your comments, see my comments below.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #20)
> ::: dom/system/gonk/NfcMessageHandler.cpp
> @@ +11,5 @@
> > +bool
> > +NfcMessageHandler::Marshall(Parcel& aParcel, const CommandOptions& aOptions)
> > +{
> > +  bool result;
> > +  return result;
> 
> When you land this you should squash these two patches together.  Obviously
> this doesn't work by itself ;-)
> 

Yeah I would merge the teo patches when they are r+.

> ::: dom/system/gonk/NfcOptions.h
> @@ +1,1 @@
> > +#ifndef NfcOptions_h
> 
> No license header?
> 
Fixed.

> @@ +9,5 @@
> > +struct NDEFRecordStruct
> > +{
> > +  uint8_t mTnf;
> > +  uint32_t mTypeLength;
> > +  nsAutoArrayPtr<uint8_t> mType;
> 
> This might be cleaner if you used nsTArray<uint8_t> to replace mFoo and
> mFooLength.
> 
Fixed, use nsTArray now.

> @@ +27,5 @@
> > +  if (aOther.prop.WasPassed()) {                      \
> > +    prop = aOther.prop.Value();                       \
> > +  } else {                                            \
> > +    prop = defaultValue;                              \
> > +  }
> 
> I think we should add a method to Optional<T> that returns Value() if it was
> passed or else a provided default value.  Perhaps const T&
> ValueOrDefault(const T&)?
> 
This pattern is also used by Wifi, network when they are rewritten by C++.
I'd file a follow up bug for this, is this okay for you?


> @@ +38,5 @@
> > +      prop = aOther.prop.Value();                     \
> > +    }                                                 \
> > +  } else {                                            \
> > +    prop = defaultValue;                              \
> > +  }
> 
> Why are you handling null like this?  This seems like something that should
> either happen at the WebIDL layer or not at all.
> 

This was used by older NFC code, now we don't have to handle this.
(The older patch was used by 'config' command, which is sent from Gaia by CustomEvent, now after Bug 970251 is fixed, all commands sent from Gecko have requestId now)

> @@ +46,5 @@
> > +  COPY_OPT_STRING_FIELD(mRequestId, EmptyString())
> > +  COPY_OPT_FIELD(mPowerLevel, 0)
> > +  COPY_OPT_FIELD(mTechType, 0)
> > +
> > +  if (aOther.mRecords.WasPassed()) {
> 
> Seems like we should be writing an operator= for something instead of all
> this code.
> 

Sorry I don't understand this comment, can you check my new patch and see if I have addressed it?


> @@ +105,5 @@
> > +  int32_t mMinorVersion;
> > +  int32_t mTechListLength;
> > +  nsAutoArrayPtr<uint8_t> mTechList;
> > +  int32_t mRecordCount;
> > +  nsAutoArrayPtr<NDEFRecordStruct> mRecords;
> 
> Same things here about using nsTArray.
> 
Fixed.


> ::: dom/system/gonk/NfcService.cpp
> @@ +37,5 @@
> > +
> > +namespace mozilla {
> > +namespace nfc {
> > +
> > +static StaticRefPtr<NfcService> gNfcService;
> 
> You can just use a raw pointer here.  The XPCOM service manager will hold a
> strong reference to your service until XPCOM shutdown.
> 

Fixed.

> @@ +42,5 @@
> > +
> > +NS_IMPL_ISUPPORTS1(NfcService, nsINfcService)
> > +
> > +// Runnable used to call SendCommand on the worker thread.
> > +class WorkerRunnable : public nsRunnable
> 
> Please give this a more descriptive name.  Maybe NfcCommandRunnable?
> 

Fixed.

> @@ +85,5 @@
> > +    JS::Rooted<JS::Value> value(cx);
> > +    RootedDictionary<NfcEventOptions> event(cx);
> > +    if (!event.Init(cx, value)) {
> > +      return NS_ERROR_TYPE_ERR;
> > +    }
> 
> Why do you need to call Init at all?  IIRC that is only for constructing a
> dictionary from a JS value or JSON string.
> 

Fixed. removed Init(..)

> @@ +112,5 @@
> > +      }
> > +
> > +      for (int i = 0; i < mEvent.mTechListLength; i++) {
> > +        nsString& elem = *event.mTechList.Value().AppendElement();
> > +        elem = NS_ConvertUTF8toUTF16(NfcTechString[mEvent.mTechList[i]]);
> 
> Where are these parameters validated?  What stops mEvent.mTechList[i] from
> being larger than the length of NfcTechString?  I can't find the code that
> does that, but even if it does exist, you should add an assertion here for
> the length.
> 

These parameters will be set in Part 2.
I also add an asserting for check the length.

> Rather than converting them to wide strings you should just store them as
> wide strings.
> 

Fixed, store in wide strings.

> @@ +224,5 @@
> > +NS_IMETHODIMP
> > +NfcService::Start(nsINfcEventListener* aListener)
> > +{
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +  MOZ_ASSERT(aListener);
> 
> You should also assert that we have not been started already.
> 

Fixed.

> @@ +226,5 @@
> > +{
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +  MOZ_ASSERT(aListener);
> > +
> > +  nsresult rv = NS_NewThread(getter_AddRefs(mWorkerThread));
> 
> Please use NS_NewNamedThread and give this a useful name.
> 

Fixed. call it NfcThread (I tried to name it NfcMarshallerThread but it exceeds to max length 16 chars).

> @@ +240,5 @@
> > +  return NS_OK;
> > +}
> > +
> > +NS_IMETHODIMP
> > +NfcService::Shutdown()
> 
> What calls this?
> 

Fixed, Nfc.js will call nfcService.shutdown() now.

> @@ +279,5 @@
> > +{
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +
> > +  mozilla::AutoSafeJSContext cx;
> > +  JS::RootedValue val(cx);
> 
> You need to enter a compartment here.  But it's not actually clear how to
> get the right compartment (of mListener) here :(
> 
> I think maybe you should make the listener a WebIDL callback instead of an
> XPIDL one, and then you could just let the WebIDL binding layer deal with
> compartments for you.
> 
I am not sure about this, 
this pattern is also used by Wifi (Bug 864932) and Network worker (Bug 864931),
if it is still not correct, should I file a followup bug and fix this for NFC, Wifi and Network?


> ::: dom/system/gonk/NfcService.h
> @@ +4,5 @@
> > +
> > +#ifndef NfcService_h
> > +#define NfcService_h
> > +
> > +#include "mozilla/dom/NfcOptionsBinding.h"
> 
> Forward declare mozilla::dom::NfcEventOptions.
> 
Fixed.

> @@ +9,5 @@
> > +#include "mozilla/ipc/Nfc.h"
> > +#include "mozilla/ipc/UnixSocket.h"
> > +#include "nsCOMPtr.h"
> > +#include "nsINfcService.h"
> > +#include "nsThread.h"
> 
> and nsIThread
> 

Fixed.

> @@ +13,5 @@
> > +#include "nsThread.h"
> > +#include "NfcMessageHandler.h"
> > +
> > +namespace mozilla {
> > +namespace nfc {
> 
> Drop the nfc namespace.  You're already prefixed the classes with Nfc.

Removed all nfc namespace.

Thanks again for your prompt review.
Attachment #8423605 - Flags: review?(khuey)
Target Milestone: 2.0 S2 (23may) → 2.0 S3 (6june)
(Assignee)

Comment 24

3 years ago
Created attachment 8439813 [details] [diff] [review]
Part 1: NFC Service v4

rebase
Attachment #8423605 - Attachment is obsolete: true
Attachment #8423605 - Flags: review?(khuey)
(Assignee)

Comment 25

3 years ago
Created attachment 8439814 [details] [diff] [review]
Part 2: NFC Message Handler v4.1

rebase
Attachment #8423606 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8439814 - Attachment description: NFC Message Handler v4.1 → Part 2: NFC Message Handler v4.1
(Assignee)

Comment 26

3 years ago
Now the patches will cause linking error on m-c now, 

B2G_Nexus4/prebuilts/gcc/linux-x86/arm/arm-linux-androideabi-4.7/bin/../lib/gcc/arm-linux-androideabi/4.7/../../../../arm-linux-androideabi/bin/ld: error: hidden symbol '_ZNK7android6Parcel9readInt32Ev' is not defined locally

Now I found out it's caused by Bug 1001320 Part 5, which uses '#pragma GCC visibility push(hidden)', and from GCC Wiki [1], '#pragma GCC visibility is stronger than -fvisibility; it affects extern declarations as well. -fvisibility only affects definitions, so that existing code can be recompiled with minimal changes'.

I'll upload another Part 3 patch for fixing this linking error.

[1]: https://gcc.gnu.org/wiki/Visibility
(Assignee)

Comment 27

3 years ago
Comment on attachment 8439813 [details] [diff] [review]
Part 1: NFC Service v4

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

::: dom/system/gonk/NfcMessageHandler.h
@@ +5,5 @@
> +#ifndef NfcMessageHandler_h
> +#define NfcMessageHandler_h
> +
> +namespace android {
> +class Parcel;

adding MOZ_EXPORT here could fix the linking error mentioned in Comment 26.
(Assignee)

Comment 28

3 years ago
Created attachment 8442705 [details] [diff] [review]
Part 1: NFC Service v5
Attachment #8439813 - Attachment is obsolete: true
(Assignee)

Comment 29

3 years ago
Created attachment 8442707 [details] [diff] [review]
Part 2: NFC Message Handler v5
Attachment #8439814 - Attachment is obsolete: true
(Assignee)

Comment 30

3 years ago
Comment on attachment 8442705 [details] [diff] [review]
Part 1: NFC Service v5

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

Hi, Kyle
This version just rebases to latest m-c and adds 'MOZ_EXPORT' as explained in Comment 27,
sorry I didn't make it before you leave Taipei,
can you help to review this ?

Thank you ~
Attachment #8442705 - Flags: review?(khuey)
update target milestone to current one.
Target Milestone: 2.0 S3 (6june) → 2.0 S5 (4july)
Comment on attachment 8442705 [details] [diff] [review]
Part 1: NFC Service v5

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

Some questions, but this looks reasonable.

I assume you've tested this in a debug build to make sure the assertions are correct?

::: dom/system/gonk/NfcMessageHandler.cpp
@@ +10,5 @@
> +bool
> +NfcMessageHandler::Marshall(Parcel& aParcel, const CommandOptions& aOptions)
> +{
> +  bool result;
> +  return result;

I hope there's another patch that does something here ...

::: dom/system/gonk/NfcOptions.h
@@ +8,5 @@
> +#include "mozilla/dom/NfcOptionsBinding.h"
> +
> +namespace mozilla {
> +
> +struct NDEFRecordStruct

Why can't we just use the generated DOM dictionaries here?  Is it because of the JS values?

@@ +18,5 @@
> +};
> +
> +struct CommandOptions
> +{
> +  CommandOptions(const mozilla::dom::NfcCommandOptions& aOther) {

Everything inside this function should be indented another 2 spaces.

::: dom/system/gonk/NfcService.cpp
@@ +54,5 @@
> +  }
> +
> +  NS_IMETHOD Run()
> +  {
> +    MOZ_ASSERT(!NS_IsMainThread());

You should add a function that asserts that do_GetCurrentThread == NfcService::mThread, and use that in all of these runnables.

@@ +87,5 @@
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +
> +    mozilla::AutoSafeJSContext cx;
> +    JS::Rooted<JS::Value> value(cx);

Unless I'm missing something 'value' is unused.

@@ +90,5 @@
> +    mozilla::AutoSafeJSContext cx;
> +    JS::Rooted<JS::Value> value(cx);
> +    RootedDictionary<NfcEventOptions> event(cx);
> +
> +    // the copy constructor is private.

Which is quite unfortunate because this code is ugly :/

@@ +190,5 @@
> +      const uint8_t* data = mData->mData.get();
> +      uint32_t parcelSize = ((data[offset + 0] & 0xff) << 24) |
> +                            ((data[offset + 1] & 0xff) << 16) |
> +                            ((data[offset + 2] & 0xff) <<  8) |
> +                             (data[offset + 3] & 0xff);

This worries me a lot.  What stops a malicious NFC peer from constructing a packet with a parcelSize that is much larger than mData->mData.Length() and causing us to read off the end of the buffer?  I think there needs to be some bound checking here.

::: dom/system/gonk/NfcService.h
@@ +28,5 @@
> +  static already_AddRefed<NfcService> FactoryCreate();
> +
> +  void DispatchNfcEvent(const mozilla::dom::NfcEventOptions& aOptions);
> +
> +  void ReceiveSocketData(nsAutoPtr<mozilla::ipc::UnixSocketRawData>& aData) MOZ_OVERRIDE;

nit: mark this as virtual

@@ +29,5 @@
> +
> +  void DispatchNfcEvent(const mozilla::dom::NfcEventOptions& aOptions);
> +
> +  void ReceiveSocketData(nsAutoPtr<mozilla::ipc::UnixSocketRawData>& aData) MOZ_OVERRIDE;
> +private:

nit: \n above private
Attachment #8442705 - Flags: review?(khuey)
(Assignee)

Comment 33

3 years ago
Thanks for the review, Kyle :)

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #32)
> Some questions, but this looks reasonable.
> 
> I assume you've tested this in a debug build to make sure the assertions are
> correct?
>
Yes

> ::: dom/system/gonk/NfcMessageHandler.cpp
> @@ +10,5 @@
> > +bool
> > +NfcMessageHandler::Marshall(Parcel& aParcel, const CommandOptions& aOptions)
> > +{
> > +  bool result;
> > +  return result;
> 
> I hope there's another patch that does something here ...
> 
Yes, Part 2 will add more impl in this funciton, I tried to seperate them to make them easier to review, however if it's actually making you confused, next version I'll only leave the declaration in the header, and move the definition to Part 2.

> ::: dom/system/gonk/NfcOptions.h
> @@ +8,5 @@
> > +#include "mozilla/dom/NfcOptionsBinding.h"
> > +
> > +namespace mozilla {
> > +
> > +struct NDEFRecordStruct
> 
> Why can't we just use the generated DOM dictionaries here?  Is it because of
> the JS values?
> 
As I remember when I implemented this, dictionary seems to have to be used on main thread, at that time I remembered I met some crashes when trying to parse the dictionary on NfcService thread.
Not sure if dictionary can be manipulated on other thread now, but I will try to not to use the NDEFRecordStruct.

And I'll address your other comments.

Thank you.
(Assignee)

Comment 34

3 years ago
Right now I met 
"JavaScript Error: "Accessing TypedArray data over Xrays is slow, and forbidden in order to encourage performant code. To copy TypedArrays across origin boundaries, consider using Components.utils.cloneInto()."

I guess it's from Bug 987163, I'll add another (Part 3) patch to address this.
Looks like this is being actively worked on. Let's defer the target milestone.
Target Milestone: 2.0 S5 (4july) → 2.0 S6 (18july)
(Assignee)

Comment 36

3 years ago
Comment on attachment 8442705 [details] [diff] [review]
Part 1: NFC Service v5

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

::: dom/system/gonk/Nfc.js
@@ +491,3 @@
>     */
>    onmessage: function onmessage(event) {
> +    let message = event;

let message = Cu.cloneInto(event, this);
Could fix the error from Comment 34.
(Assignee)

Comment 37

3 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #32)
> ::: dom/system/gonk/NfcOptions.h
> @@ +8,5 @@
> > +#include "mozilla/dom/NfcOptionsBinding.h"
> > +
> > +namespace mozilla {
> > +
> > +struct NDEFRecordStruct
> 
> Why can't we just use the generated DOM dictionaries here?  Is it because of
> the JS values?
> 

Hi Kyle
I find now I can use the generator DOM on a non-main thread, however for the generated DOM dict, its copy constructor is private, so I have to construct those memebers manually when I pass it to a nsRunnable, that seems not have advantage over using plain struct, and even in the nsRunnable, I still have to call member.WasPassed(), member.Value(), ....

Can you help to explain again why we should use the generated DOM dict directly on a I/O thread?

Also can you help to check if the Comment 36 is the correct usage for cloneInfo ? I am not sure if I can use the 'this' object as the 2nd parameter.

Thank you
Flags: needinfo?(khuey)
(In reply to Yoshi Huang[:allstars.chh] from comment #37)
> Can you help to explain again why we should use the generated DOM dict
> directly on a I/O thread?

I am asking if you can, not saying that you should.  It sounds like the answer is no.

> Also can you help to check if the Comment 36 is the correct usage for
> cloneInfo ? I am not sure if I can use the 'this' object as the 2nd
> parameter.

The problem there is that DispatchNfcEvent is using the safe JS context and not the correct compartment.  Bobby, what's the right way to enter the compartment of a JS implemented service?
Flags: needinfo?(khuey) → needinfo?(bobbyholley)
(Assignee)

Comment 39

3 years ago
Created attachment 8455227 [details] [diff] [review]
Part 1: NFC Service v6

addressed Kyle's comments.

Given we are still waiting for bholley to reply for the Cu.cloneInto problem, I didn't include the fix in Comment 36 here.
Attachment #8442705 - Attachment is obsolete: true
(Assignee)

Comment 40

3 years ago
Created attachment 8455228 [details] [diff] [review]
Part 2: NFC Message Handler v6
Attachment #8442707 - Attachment is obsolete: true
(Assignee)

Comment 41

3 years ago
Comment on attachment 8455227 [details] [diff] [review]
Part 1: NFC Service v6

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

Kyle,
I have addressed your previous comments and tested this on device and passed the marionette-webapi tests on my local build.
Can you help to review this again for me ?

Thank you.
Attachment #8455227 - Flags: review?(khuey)
(Assignee)

Comment 42

3 years ago
Created attachment 8455232 [details] [diff] [review]
Part 3: update test cases.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #38)
> The problem there is that DispatchNfcEvent is using the safe JS context and
> not the correct compartment.  Bobby, what's the right way to enter the
> compartment of a JS implemented service?

If I'm understanding your question correctly, probably something like:

nsCOMPtr<nsIFoo> svc = do_GetService(contractID);
nsCOMPtr<nsIXPConnectWrappedJS> wjs = do_QueryInterface(svc);
MOZ_ASSERT(wjs);

nsCOMPtr<nsIGlobalObject> global = xpc::GetNativeForGlobal(js::GetGlobalForObjectCrossCompartment(wjs->GetJSObject()));
if (NS_WARN_IF(!global))
  return false;

AutoJSAPI jsapi;
if (NS_WARN_IF(!jsapi.Init(global)))
  return false;
JSContext *cx = jsapi.cx();
Flags: needinfo?(bobbyholley)
Comment on attachment 8455227 [details] [diff] [review]
Part 1: NFC Service v6

Try bholley's suggestion to remove the cloneInto please.
Attachment #8455227 - Flags: review?(khuey)
(Assignee)

Comment 45

3 years ago
Created attachment 8456006 [details] [diff] [review]
WIP. Part 4: TypedArray cross compartment problem

Hi, Kyle and Bholley
This is the patch as suggested by bholley, however with this patch I'll still get the error in Comment 34.

Can you help to check where I did it wrong ?

Thank you.
Attachment #8456006 - Flags: feedback?(khuey)
Attachment #8456006 - Flags: feedback?(bobbyholley)
Comment on attachment 8456006 [details] [diff] [review]
WIP. Part 4: TypedArray cross compartment problem

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

This looks like what I suggested in general. Is this a common pattern? If so, we should add a helper for it.

::: dom/system/gonk/NfcService.cpp
@@ +325,3 @@
>    JS::RootedValue val(cx);
>  
>    if (!ToJSValue(cx, aOptions, &val)) {

This only puts the options in the scope of the service, not the event itself. Is that the problem?
Attachment #8456006 - Flags: feedback?(bobbyholley) → feedback+
(In reply to Bobby Holley (:bholley) from comment #46)
> This only puts the options in the scope of the service, not the event
> itself. Is that the problem?

Almost certainly.
Comment on attachment 8456006 [details] [diff] [review]
WIP. Part 4: TypedArray cross compartment problem

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

Try using the listener instead of the service here.
Attachment #8456006 - Flags: feedback?(khuey) → feedback-
(Assignee)

Comment 49

3 years ago
Created attachment 8456808 [details] [diff] [review]
Part 1: NFC Service v7

Nfc also implements nsINfcEventListener.
Attachment #8455227 - Attachment is obsolete: true
(Assignee)

Comment 50

3 years ago
Created attachment 8456809 [details] [diff] [review]
Part 2: NFC Message Handler v7

rebase
Attachment #8455228 - Attachment is obsolete: true
(Assignee)

Comment 51

3 years ago
Created attachment 8456810 [details] [diff] [review]
WIP. Part 4: TypedArray cross compartment problem v2
Attachment #8456006 - Attachment is obsolete: true
(Assignee)

Comment 52

3 years ago
Created attachment 8456813 [details] [diff] [review]
Part 1: NFC Service v7.1

forgot to add nsINfcEventListener in the Nfc.classInfo
Attachment #8456808 - Attachment is obsolete: true
(Assignee)

Comment 53

3 years ago
Comment on attachment 8456810 [details] [diff] [review]
WIP. Part 4: TypedArray cross compartment problem v2

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

Kyle,
I have updated Part 1, to make Nfc implements nsINfcEventListener now.

However I still have the same problem. (Comment 34).

Also I don't quite understand what Bholley and you meant in Comment 46,
how do I put the val into the scope of Nfc?

Sorry for asking this again :(

Can you help to shed me some light here?

Thank you
Attachment #8456810 - Flags: feedback?(khuey)
No, that's not what I meant.  Find me on IRC today.
(Assignee)

Comment 55

3 years ago
Created attachment 8457774 [details] [diff] [review]
WIP. Part 4: TypedArray cross compartment problem v3

using mListener, however still has the same problem
Attachment #8456810 - Attachment is obsolete: true
Attachment #8456810 - Flags: feedback?(khuey)
(Assignee)

Comment 56

3 years ago
Created attachment 8457916 [details] [diff] [review]
Part 1: NFC Service v8

Move Cu.cloneInto back.
Attachment #8456813 - Attachment is obsolete: true
Attachment #8457916 - Flags: review?(khuey)
(Assignee)

Comment 57

3 years ago
Created attachment 8457917 [details] [diff] [review]
Part 2: NFC Message Handler v8
Attachment #8456809 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8457774 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Blocks: 1040026
(Assignee)

Updated

3 years ago
Blocks: 933141
(Assignee)

Updated

3 years ago
Blocks: 907252
(Assignee)

Updated

3 years ago
Blocks: 897312
Comment on attachment 8457916 [details] [diff] [review]
Part 1: NFC Service v8

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

r=me
Attachment #8457916 - Flags: review?(khuey) → review+
Sorry for the delay.  Got caught up in 2.0 homescreen regressions :/
(Assignee)

Comment 60

3 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #59)
> Sorry for the delay.  Got caught up in 2.0 homescreen regressions :/
No worries, Kyle. :)
(Assignee)

Comment 61

3 years ago
Comment on attachment 8457917 [details] [diff] [review]
Part 2: NFC Message Handler v8

Mostly are the same, except some type changes after Part 1 changed.
Attachment #8457917 - Flags: review?(dlee)
Comment on attachment 8457917 [details] [diff] [review]
Part 2: NFC Message Handler v8

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

::: dom/system/gonk/NfcMessageHandler.cpp
@@ +58,5 @@
> +  } else if (!strcmp(type, kCloseRequest)) {
> +    result = CloseRequest(aParcel, aOptions);
> +    mPendingReqQueue.AppendElement(eNfcRequest_Close);
> +  }
> +

handle else here or give result a default value to prevent uninitiailized return value
same below
Attachment #8457917 - Flags: review?(dlee) → review+
(Assignee)

Comment 63

3 years ago
Created attachment 8464515 [details] [diff] [review]
Part 2: NFC Message Handler v9

Add else part.
Attachment #8457917 - Attachment is obsolete: true
Attachment #8464515 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8455232 - Flags: review?(dlee)

Updated

3 years ago
Attachment #8455232 - Flags: review?(dlee) → review+
(Assignee)

Comment 64

3 years ago
Try looks green
https://tbpl.mozilla.org/?tree=Try&rev=8b127addae5d

Also marionette-webapi on emulator-JB on local have been passed.
(Assignee)

Comment 65

3 years ago
https://hg.mozilla.org/integration/b2g-inbound/rev/fc3b90212225
https://hg.mozilla.org/integration/b2g-inbound/rev/85744a1b9708
https://hg.mozilla.org/integration/b2g-inbound/rev/127ca52e4cce
(Assignee)

Updated

3 years ago
Whiteboard: [p=8] → [p=13]
Target Milestone: 2.0 S6 (18july) → 2.1 S1 (1aug)
https://hg.mozilla.org/mozilla-central/rev/fc3b90212225
https://hg.mozilla.org/mozilla-central/rev/85744a1b9708
https://hg.mozilla.org/mozilla-central/rev/127ca52e4cce
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
Depends on: 1057258

Updated

3 years ago
Depends on: 1050764
(Assignee)

Updated

3 years ago
Blocks: 1059168

Updated

2 years ago
Depends on: 1134088
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
You need to log in before you can comment on or make changes to this bug.