Closed Bug 1058440 Opened 5 years ago Closed 5 years ago

B2G NFC: enable debug when NFC debug is enabled

Categories

(Firefox OS Graveyard :: NFC, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(tracking-b2g:backlog)

RESOLVED FIXED
2.1 S7 (24Oct)
tracking-b2g backlog

People

(Reporter: allstars.chh, Assigned: dimi)

References

Details

Attachments

(1 file, 6 obsolete files)

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.
Whiteboard: [good first bug]
Assignee: nobody → dlee
Attached patch Bug Fixing patch v1 (obsolete) — Splinter Review
This patch enable debug message for NfcContentHelp.js & Nfc.js
Attachment #8481216 - Flags: review?(allstars.chh)
Attachment #8481216 - Flags: review?(allstars.chh) → review+
Whiteboard: [good first bug]
add checkin-needed as b2g-inbound is close now.
Keywords: checkin-needed
Attached patch Bug Fixing patch v2 (obsolete) — Splinter Review
add reviewer in patch description
Attachment #8481216 - Attachment is obsolete: true
Attachment #8481230 - Flags: review+
Is there a Try link handy for this by chance? :)
Keywords: checkin-needed
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?
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
Contact app crash is caused by this patch
The root cause is that calling  "let lock = gSettingsService.createLock()" in NfcContentHelper
(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
[Blocking Requested - why for this release]:
This is the gecko part for Bug 1058088
blocking-b2g: --- → 2.1?
triage: not blocking release, but this is high-want. approval required when patch ready to be uplifted.
blocking-b2g: 2.1? → backlog
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)
I don't understand why SettingsService is doing that. It should be removed. I filed bug 1063783.
Flags: needinfo?(reuben.bmo)
Hi Reuben,
Is there any update for bug 1063783 ? Thanks!
Flags: needinfo?(reuben.bmo)
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)
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)
(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)
Attached patch Bug Fixing patch v3 (obsolete) — Splinter Review
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)
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+
Attached patch Bug Fixing patch v4 (obsolete) — Splinter Review
Fix nits
Attachment #8495757 - Attachment is obsolete: true
Attachment #8495771 - Flags: review+
Attached patch Bug Fixing patch v5 (obsolete) — Splinter Review
Miss fixing add function name in init function
Attachment #8495771 - Attachment is obsolete: true
Attachment #8495800 - Flags: review+
Keywords: checkin-needed
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
Attached patch Rebased patch v6Splinter Review
Attachment #8495800 - Attachment is obsolete: true
Attachment #8504583 - Flags: review+
Flags: needinfo?(dlee)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ce264541d998
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S7 (24Oct)
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.