1058440 - B2G NFC: enable debug when NFC debug is enabled

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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.

Comment 1

[Dimi Lee [:dimi]]

Attachment: Bug Fixing patch v1

This patch enable debug message for NfcContentHelp.js & Nfc.js

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

add checkin-needed as b2g-inbound is close now.

Comment 3

[Dimi Lee [:dimi]]

Attachment: Bug Fixing patch v2

add reviewer in patch description

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Is there a Try link handy for this by chance? :)

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Sorry, try result is here
https://tbpl.mozilla.org/?tree=Try&rev=f90844c80512

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://hg.mozilla.org/integration/b2g-inbound/rev/c345da3eef13

Comment 7

[Zac C (:zac)]

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

[Zac C (:zac)]

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

Comment 9

[Dimi Lee [:dimi]]

Contact app crash is caused by this patch
The root cause is that calling  "let lock = gSettingsService.createLock()" in NfcContentHelper

Comment 10

[Carsten Book [:Tomcat]]

(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

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

[Blocking Requested - why for this release]:
This is the gecko part for Bug 1058088

Comment 12

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: not blocking release, but this is high-want. approval required when patch ready to be uplifted.

Comment 13

[Dimi Lee [:dimi]]

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!

Comment 14

[Reuben Morais [:reuben]]

I don't understand why SettingsService is doing that. It should be removed. I filed bug 1063783.

Comment 15

[Dimi Lee [:dimi]]

Hi Reuben,
Is there any update for bug 1063783 ? Thanks!

Comment 16

[Reuben Morais [:reuben]]

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?

Comment 17

[Dimi Lee [:dimi]]

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.

Comment 18

[Reuben Morais [:reuben]]

(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.

Comment 19

[Dimi Lee [:dimi]]

Attachment: 0001-Bug-1058440-B2G-NFC-enable-debug-when-NFC-debug-is-e.patch

Comment 20

[Dimi Lee [:dimi]]

Attachment: Bug Fixing patch v3

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

Comment 21

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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.

Comment 22

[Dimi Lee [:dimi]]

Attachment: Bug Fixing patch v4

Fix nits

Comment 23

[Dimi Lee [:dimi]]

running try
https://tbpl.mozilla.org/?tree=Try&rev=7320c3ba6fd2

Comment 24

[Dimi Lee [:dimi]]

Attachment: Bug Fixing patch v5

Miss fixing add function name in init function

Comment 25

[Carsten Book [:Tomcat]]

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!

Comment 26

[Dimi Lee [:dimi]]

Attachment: Rebased patch v6

Comment 27

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://hg.mozilla.org/integration/b2g-inbound/rev/ce264541d998

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ce264541d998