Closed Bug 933588 Opened 11 years ago Closed 11 years ago

B2G NFC: Rewrite NFC Worker in C++

Categories

(Firefox OS Graveyard :: NFC, defect)

Product:
Firefox OS Graveyard
Component:
NFC
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(tracking-b2g:backlog)

Status:
RESOLVED FIXED
Milestone:
2.1 S1 (1aug)
Project Flags:
tracking-b2g backlog

People

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

References

Details

(Whiteboard: [p=13])

Attachments

(3 files, 22 obsolete files)

Part 3: update test cases.
1.25 KB, patch
dimi
: review+
Details | Diff | Splinter Review
Part 1: NFC Service v8
38.13 KB, patch
khuey
: review+
Details | Diff | Splinter Review
Part 2: NFC Message Handler v9
31.26 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
Assignee: nobody → allstars.chh
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
Attached patch Part 1: NFC Service (obsolete) — — Splinter Review
Attachment #8374733 - Attachment is obsolete: true
Attachment #8375405 - Flags: review?(fabrice)
Attachment #8375405 - Flags: review?(dlee)
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)
Attached patch Part 2: NFC Message Handler v2 (obsolete) — — Splinter Review
Addressed Dimi's comments/
Attachment #8375405 - Attachment is obsolete: true
Attachment #8375405 - Flags: review?(fabrice)
Attachment #8376969 - Flags: review?(fabrice)
Attachment #8376969 - Flags: review?(dlee)
(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.
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)
(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.
Attached patch Part 1: NFC Service v2. (obsolete) — — Splinter Review
rename sendToWorker to sendToNfcService by fabrice's comment. Also move some code from Part2 to Part 1.
Attachment #8375404 - Attachment is obsolete: true
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)
Attached patch Part 2: NFC Message Handler v3 (obsolete) — — Splinter Review
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+
(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.
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.
Blocks: b2g-NFC-2.0
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)
Target Milestone: 2.0 S1 (9may) → 2.0 S2 (23may)
Attached patch Part 2: NFC Message Handler v4 (obsolete) — — Splinter Review
Modified after Part 1 changed. I'll ask r? to Dimi again when Part 1 is r+.
Attachment #8378115 - Attachment is obsolete: true
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)
Attached patch Part 1: NFC Service v4 (obsolete) — — Splinter Review
rebase
Attachment #8423605 - Attachment is obsolete: true
Attachment #8423605 - Flags: review?(khuey)
Attachment #8439814 - Attachment description: NFC Message Handler v4.1 → Part 2: NFC Message Handler v4.1
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
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.
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)
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.
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)
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.
(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)
Attached patch Part 1: NFC Service v6 (obsolete) — — Splinter Review
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
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)
(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)
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-
Attached patch Part 1: NFC Service v7 (obsolete) — — Splinter Review
Nfc also implements nsINfcEventListener.
Attachment #8455227 - Attachment is obsolete: true
Attached patch Part 1: NFC Service v7.1 (obsolete) — — Splinter Review
forgot to add nsINfcEventListener in the Nfc.classInfo
Attachment #8456808 - Attachment is obsolete: true
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.
using mListener, however still has the same problem
Attachment #8456810 - Attachment is obsolete: true
Attachment #8456810 - Flags: feedback?(khuey)
Move Cu.cloneInto back.
Attachment #8456813 - Attachment is obsolete: true
Attachment #8457916 - Flags: review?(khuey)
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 :/
(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. :)
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+
Add else part.
Attachment #8457917 - Attachment is obsolete: true
Attachment #8464515 - Flags: review+
Attachment #8455232 - Flags: review?(dlee) → review+
Try looks green https://tbpl.mozilla.org/?tree=Try&rev=8b127addae5d Also marionette-webapi on emulator-JB on local have been passed.
Whiteboard: [p=8] → [p=13]
Target Milestone: 2.0 S6 (18july) → 2.1 S1 (1aug)
Depends on: 1050764
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.

Attachment

General

Creator:
Created:
Updated:
Size: