Closed
Bug 1058440
Opened 10 years ago
Closed 10 years ago
B2G NFC: enable debug when NFC debug is enabled
Categories
(Firefox OS Graveyard :: NFC, defect)
Tracking
(tracking-b2g:backlog)
People
(Reporter: allstars.chh, Assigned: dimi)
References
Details
Attachments
(1 file, 6 obsolete files)
10.65 KB,
patch
|
dimi
:
review+
|
Details | Diff | Splinter Review |
In Bug 1058088 will add a settings for enabling debug, Gecko should also watch this settings and turn on debug when the setting is turned on by user.
Reporter | ||
Updated•10 years ago
|
Whiteboard: [good first bug]
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → dlee
Assignee | ||
Comment 1•10 years ago
|
||
This patch enable debug message for NfcContentHelp.js & Nfc.js
Attachment #8481216 -
Flags: review?(allstars.chh)
Reporter | ||
Updated•10 years ago
|
Attachment #8481216 -
Flags: review?(allstars.chh) → review+
Reporter | ||
Updated•10 years ago
|
Whiteboard: [good first bug]
Reporter | ||
Comment 2•10 years ago
|
||
add checkin-needed as b2g-inbound is close now.
Keywords: checkin-needed
Assignee | ||
Comment 3•10 years ago
|
||
add reviewer in patch description
Attachment #8481216 -
Attachment is obsolete: true
Attachment #8481230 -
Flags: review+
Reporter | ||
Comment 5•10 years ago
|
||
Sorry, try result is here https://tbpl.mozilla.org/?tree=Try&rev=f90844c80512
Reporter | ||
Comment 6•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/c345da3eef13
Comment 7•10 years ago
|
||
We've identified this as causing the Contacts app to crash on Flame device. Our pushlog is: http://hg.mozilla.org/integration/b2g-inbound/pushloghtml?fromchange=e7174ed15179&tochange=c345da3eef13 It's replicable manually. Can you back this out then fix it?
Comment 8•10 years ago
|
||
logcat: I/Gecko ( 311): IPDL protocol error: Handler for AsyncMessage returned error code I/Gecko ( 311): I/Gecko ( 311): ###!!! [Parent][DispatchAsyncMessage] Error: (msgtype=0x1C00A0,name=PContent::Msg_AsyncMessage) Processing error: message was deserialized, but the handler returned false (indicating failure) I/Gecko ( 311): I/Gecko ( 311): IPDL protocol error: Handler for AsyncMessage returned error code I/Gecko ( 311): I/Gecko ( 311): ###!!! [Parent][DispatchAsyncMessage] Error: (msgtype=0x1C00A0,name=PContent::Msg_AsyncMessage) Processing error: message was deserialized, but the handler returned false (indicating failure) I/Gecko ( 311): I/Gecko ( 311): [Parent 311] WARNING: waitpid failed pid:1822 errno:10: file ../../../gecko/ipc/chromium/src/base/process_util_posix.cc, line 261 I/Gecko ( 311): I/Gecko ( 311): ###!!! [Parent][MessageChannel] Error: Channel error: cannot send/recv I/Gecko ( 311): E/GeckoConsole( 311): Content JS LOG at app://system.gaiamobile.org/js/app_usage_metrics.js:92 in debug: [AppUsage] app://verticalhome.gaiamobile.org/manifest.webapp ran for 16 E/GeckoConsole( 311): Content JS LOG at app://system.gaiamobile.org/js/app_usage_metrics.js:92 in debug: [AppUsage] Saved app usage data
Assignee | ||
Comment 9•10 years ago
|
||
Contact app crash is caused by this patch The root cause is that calling "let lock = gSettingsService.createLock()" in NfcContentHelper
Comment 10•10 years ago
|
||
(In reply to Zac C (:zac) from comment #7) > We've identified this as causing the Contacts app to crash on Flame device. > > Our pushlog is: > http://hg.mozilla.org/integration/b2g-inbound/ > pushloghtml?fromchange=e7174ed15179&tochange=c345da3eef13 > > It's replicable manually. > > Can you back this out then fix it? this was backed out from b2g-inbound
Reporter | ||
Comment 11•10 years ago
|
||
[Blocking Requested - why for this release]: This is the gecko part for Bug 1058088
blocking-b2g: --- → 2.1?
Comment 12•10 years ago
|
||
triage: not blocking release, but this is high-want. approval required when patch ready to be uplifted.
blocking-b2g: 2.1? → backlog
Assignee | ||
Comment 13•10 years ago
|
||
Hi reuben, I have faced a problem that i don't understand very well.If possible, could you help explain this a little bit or let me know if there is someone could help this. The problem is: In child process, I try to read the setting through SettingService, it will then call http://dxr.mozilla.org/mozilla-central/source/dom/settings/SettingsService.js#74 with SystemPrincipal, and after IPC, the parent process receive this event, and it will check the principal http://dxr.mozilla.org/mozilla-central/source/dom/ipc/AppProcessChecker.cpp#160 in that function , the appId of system principal is 0, so it will not pass the check in http://dxr.mozilla.org/mozilla-central/source/dom/ipc/AppProcessChecker.cpp#177 hence the app is crashed. Sorry i don't understand how principal works very well even i look the wiki page https://developer.mozilla.org/en-US/docs/Security_check_basics so could you help explain a little bit about what happened in this case ? thanks!
Flags: needinfo?(reuben.bmo)
Comment 14•10 years ago
|
||
I don't understand why SettingsService is doing that. It should be removed. I filed bug 1063783.
Flags: needinfo?(reuben.bmo)
Assignee | ||
Comment 15•10 years ago
|
||
Hi Reuben, Is there any update for bug 1063783 ? Thanks!
Flags: needinfo?(reuben.bmo)
Comment 16•10 years ago
|
||
Hi Dimi, I'm sorry for taking so long to reply :( We'll need a non-trivial patch to make SettingsService work as intended. Am I right that your code runs once per process in a XPCOM component and has no easy way to grab a DOM window to use navigator.mozSettings?
Flags: needinfo?(reuben.bmo) → needinfo?(dlee)
Assignee | ||
Comment 17•10 years ago
|
||
Hi Reuben, Yes, the code runs once per process in a XPCOM component. And it did also have a non-trial way to get window in that XPCOM component (create another function and pass window object for whoever creates the XPCOM component). I will provide patch for this bug for now. But I would like to know if current behavior of SettingsService is correct or not ? If not, will we fix bug 1063783 in the future ? Because we may want to revert to the original solution once SettingService works fine for content process.
Flags: needinfo?(dlee) → needinfo?(reuben.bmo)
Comment 18•10 years ago
|
||
(In reply to Dimi Lee[:dimi][:dlee] from comment #17) > Yes, the code runs once per process in a XPCOM component. And it did also > have a non-trial way to get window in that XPCOM component (create another > function and pass window object for whoever creates the XPCOM component). I > will provide patch for this bug for now. Oh, great! > But I would like to know if current behavior of SettingsService is correct > or not ? If not, will we fix bug 1063783 in the future ? Because we may want > to revert to the original solution once SettingService works fine for > content process. It is not, and we will fix it.
Flags: needinfo?(reuben.bmo)
Assignee | ||
Comment 19•10 years ago
|
||
Assignee | ||
Comment 20•10 years ago
|
||
This version use window.navigator.mozSetting to get nfc debug flag. For Gaia app without setting permission then the debug message for content side won't show up. And as talked in comment 17 & comment 18, we will roll back to original solution once bug 1063783 is fixed
Attachment #8481230 -
Attachment is obsolete: true
Attachment #8495756 -
Attachment is obsolete: true
Attachment #8495757 -
Flags: review?(allstars.chh)
Reporter | ||
Comment 21•10 years ago
|
||
Comment on attachment 8495757 [details] [diff] [review] Bug Fixing patch v3 Review of attachment 8495757 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/nfc/NfcContentHelper.js @@ +102,5 @@ > > _requestMap: null, > peerEventListener: null, > > + init: function(aWindow) { nit, add function name ::: dom/nfc/gonk/Nfc.js @@ +23,5 @@ > Cu.import("resource://gre/modules/Services.jsm"); > > +XPCOMUtils.defineLazyServiceGetter(this, "gSettingsService", > + "@mozilla.org/settingsService;1", > + "nsISettingsService"); ident @@ +106,5 @@ > this.nfc = nfc; > > + let lock = gSettingsService.createLock(); > + lock.get(NFC.SETTING_NFC_DEBUG, this.nfc); > + nit, spaces/. @@ +318,5 @@ > observe: function observe(subject, topic, data) { > switch (topic) { > + case NFC.TOPIC_MOZSETTINGS_CHANGED: > + let setting = JSON.parse(data); > + if (setting) { nit, ident ::: dom/nfc/nsNfc.js @@ +53,5 @@ > initialize: function(aWindow, aSessionToken) { > this._window = aWindow; > this.session = aSessionToken; > + if (this._nfcContentHelper) { > + this._nfcContentHelper.init(aWindow); Seems the init in mozNfc should be enough. @@ +95,5 @@ > this._window = aWindow; > this.session = aSessionToken; > + if (this._nfcContentHelper) { > + this._nfcContentHelper.init(aWindow); > + } ditto.
Attachment #8495757 -
Flags: review?(allstars.chh) → review+
Assignee | ||
Comment 22•10 years ago
|
||
Fix nits
Attachment #8495757 -
Attachment is obsolete: true
Attachment #8495771 -
Flags: review+
Assignee | ||
Comment 23•10 years ago
|
||
running try https://tbpl.mozilla.org/?tree=Try&rev=7320c3ba6fd2
Assignee | ||
Comment 24•10 years ago
|
||
Miss fixing add function name in init function
Attachment #8495771 -
Attachment is obsolete: true
Attachment #8495800 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 25•10 years ago
|
||
hi Dimi, the patch failed to apply: applying bug_1058440_v5.patch patching file dom/nfc/NfcContentHelper.js Hunk #1 succeeded at 32 with fuzz 1 (offset 0 lines). Hunk #2 FAILED at 74 Hunk #3 FAILED at 95 Hunk #5 succeeded at 401 with fuzz 2 (offset -30 lines). 2 out of 5 hunks FAILED -- saving rejects to file dom/nfc/NfcContentHelper.js.rej patching file dom/nfc/gonk/Nfc.js Hunk #1 FAILED at 16 Hunk #2 FAILED at 92 2 out of 4 hunks FAILED -- saving rejects to file dom/nfc/gonk/Nfc.js.rej patching file dom/nfc/nsINfcContentHelper.idl Hunk #1 FAILED at 21 1 out of 1 hunks FAILED -- saving rejects to file dom/nfc/nsINfcContentHelper.idl.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh bug_1058440_v5.patch could you take a look and maybe rebase against a current tree, thanks!
Flags: needinfo?(dlee)
Keywords: checkin-needed
Assignee | ||
Comment 26•10 years ago
|
||
Attachment #8495800 -
Attachment is obsolete: true
Attachment #8504583 -
Flags: review+
Flags: needinfo?(dlee)
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 27•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/ce264541d998
Keywords: checkin-needed
Comment 28•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ce264541d998
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S7 (24Oct)
Updated•9 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
•