Closed
Bug 933588
Opened 11 years ago
Closed 10 years ago
B2G NFC: Rewrite NFC Worker in C++
Categories
(Firefox OS Graveyard :: NFC, defect)
Tracking
(tracking-b2g:backlog)
People
(Reporter: allstars.chh, Assigned: allstars.chh)
References
Details
(Whiteboard: [p=13])
Attachments
(3 files, 22 obsolete files)
(deleted),
patch
|
dimi
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
allstars.chh
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → allstars.chh
Updated•11 years ago
|
Blocks: backlog-RIL/Net/Conn
Assignee | ||
Comment 1•11 years ago
|
||
Assignee | ||
Comment 2•11 years ago
|
||
Assignee | ||
Comment 3•11 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 | ||
Updated•11 years ago
|
Attachment #8375404 -
Flags: review?(fabrice)
Assignee | ||
Updated•11 years ago
|
Attachment #8375405 -
Flags: review?(fabrice)
Attachment #8375405 -
Flags: review?(dlee)
Assignee | ||
Comment 6•11 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 7•11 years ago
|
||
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•11 years ago
|
||
Addressed Dimi's comments/
Attachment #8375405 -
Attachment is obsolete: true
Attachment #8375405 -
Flags: review?(fabrice)
Assignee | ||
Updated•11 years ago
|
Attachment #8376969 -
Flags: review?(fabrice)
Attachment #8376969 -
Flags: review?(dlee)
Assignee | ||
Comment 9•11 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•11 years ago
|
Attachment #8376969 -
Flags: review?(dlee) → review+
Comment 10•11 years ago
|
||
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 11•11 years ago
|
||
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•11 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•11 years ago
|
||
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•11 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•11 years ago
|
||
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+
Comment 17•11 years ago
|
||
(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•11 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•11 years ago
|
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-
Updated•11 years ago
|
blocking-b2g: --- → backlog
Updated•11 years ago
|
Target Milestone: --- → 2.0 S1 (9may)
Assignee | ||
Updated•11 years ago
|
Target Milestone: 2.0 S1 (9may) → 2.0 S2 (23may)
Assignee | ||
Updated•11 years ago
|
Whiteboard: [p=8]
Assignee | ||
Comment 22•11 years ago
|
||
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•11 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)
Updated•10 years ago
|
Target Milestone: 2.0 S2 (23may) → 2.0 S3 (6june)
Assignee | ||
Comment 24•10 years ago
|
||
rebase
Attachment #8423605 -
Attachment is obsolete: true
Attachment #8423605 -
Flags: review?(khuey)
Assignee | ||
Updated•10 years ago
|
Attachment #8439814 -
Attachment description: NFC Message Handler v4.1 → Part 2: NFC Message Handler v4.1
Assignee | ||
Comment 26•10 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•10 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 30•10 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)
Comment 31•10 years ago
|
||
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•10 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•10 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.
Comment 35•10 years ago
|
||
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•10 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•10 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•10 years ago
|
||
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 41•10 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•10 years ago
|
||
Comment 43•10 years ago
|
||
(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•10 years ago
|
||
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 46•10 years ago
|
||
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•10 years ago
|
||
Nfc also implements nsINfcEventListener.
Attachment #8455227 -
Attachment is obsolete: true
Assignee | ||
Comment 52•10 years ago
|
||
forgot to add nsINfcEventListener in the Nfc.classInfo
Attachment #8456808 -
Attachment is obsolete: true
Assignee | ||
Comment 53•10 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•10 years ago
|
||
using mListener, however still has the same problem
Attachment #8456810 -
Attachment is obsolete: true
Attachment #8456810 -
Flags: feedback?(khuey)
Assignee | ||
Comment 56•10 years ago
|
||
Move Cu.cloneInto back.
Attachment #8456813 -
Attachment is obsolete: true
Attachment #8457916 -
Flags: review?(khuey)
Assignee | ||
Updated•10 years ago
|
Attachment #8457774 -
Attachment is obsolete: true
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•10 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•10 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 62•10 years ago
|
||
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•10 years ago
|
||
Add else part.
Attachment #8457917 -
Attachment is obsolete: true
Attachment #8464515 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8455232 -
Flags: review?(dlee)
Updated•10 years ago
|
Attachment #8455232 -
Flags: review?(dlee) → review+
Assignee | ||
Comment 64•10 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•10 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•10 years ago
|
Whiteboard: [p=8] → [p=13]
Target Milestone: 2.0 S6 (18july) → 2.1 S1 (1aug)
Comment 66•10 years ago
|
||
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
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Updated•10 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
You need to log in
before you can comment on or make changes to this bug.
Description
•