1055446 - NFC: log visibly errors in nfc relateded files

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

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

The nfc_manager.js, ndef_utils.js, nfc_handover_manager.js,(or maybe more) all use debug to print error when an error is happening or exception is thrown.

Please use console.error or dump to print these messages. These error message should be shown whether the DEBUG is false.

Comment 1

[Krzysztof Mioduszewski[:tauzen]]

NfcManager is using only dump: [1]. Do you want to remove the console.log usage like here in [2]? 


[1] - https://github.com/mozilla-b2g/gaia/blob/db1825994c47a1f7faef2d10a74222c52a5a04ee/apps/system/js/nfc_manager.js#L59
[2] - https://github.com/mozilla-b2g/gaia/blob/db1825994c47a1f7faef2d10a74222c52a5a04ee/apps/system/js/nfc_handover_manager.js#L130

Comment 2

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

I am not sure which one is prefered on Gaia-side, whether dump or console.XXX.
But I do hope these errors will be shown on logcat even without enabling DEBUG flag in those files. So developers and QAs can easily know where the problem is.

Comment 3

[Krzysztof Mioduszewski[:tauzen]]

Ok, understood. I've looked through gaia code and I don't see any clear pattern. Will ask on IRC and fix this.

Comment 4

[(inactive after 6/18) Alive Kuo [:alive]]

I doubt this. We need to check case by case if the error something really bad.
Otherwise gaia will have lots of 'errors' and people will have difficulty to find out what's important.

Comment 5

[Krzysztof Mioduszewski[:tauzen]]

Yoshi taking into account comment 4 I will go through all NFC related files and log using console.log errors caught in catch. On IRC Alive suggested that DOM Request onerror isn't really an error situation, so I'll check each onerror and decide case be case if we should log it or not. Is this ok with you?

Additionally as suggested by Alive on IRC, I'll change from dump to console.log.

Comment 6

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

Okay for me, thanks. That helped a lot.
As you know NFC-Sharing is not working very often, and everytime I got logs from QA, it's nothing there.

But Do show the error message when you catch an exception. 
I've seen some code 

catch(e){
  this.debug(e);
}

If it's an exception, I think we need to print out this, whether it has 1 or 100 exceptions.

And maybe you could discuss how to help QA to enable more logs on Gaia NFC side.

Comment 7

[(inactive after 6/18) Alive Kuo [:alive]]

I don't think dom request.onerror is a real error. But I am fine if something is a real fault and it will not annoy others.

If the change starts to annoy *even* nobody is doing nfc pairing we will back it out.
BTW, I tend to think you still need to turn on debug when you want to know what happens since you cannot console.log near/disconnect stuff...

Comment 8

[Krzysztof Mioduszewski[:tauzen]]

Hello Alison, please see Yoshis comment 6. Maybe you have suggestions which I could incorporate in this patch, which could help you to test NFC easier?

Comment 9

[Krzysztof Mioduszewski[:tauzen]]

Sorry, clicked too quickly. Allison please check comment 8 ;).

Comment 10

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

Hi Gaia developers and reviewer

Developers need to provide a way for QA to figure out what went wrong.
The current situation is, QA doesn't know what went wrong, she just saw an error dialog popup 
"The file cannot be sent/received".

And she checked from logcat, nothing is showing.
QA might not know how to do coding, don't know how to apply your patch, don't know which file should be debug-enabled.
In the end she just can file a bug, "File cannot be shared", but no useful information can be provided from Gaia side (QA know how to enable log from BT/NFC from Gecko side)

So I'd request Gaia-devs could make this situation more efficient.

Right now the flow is usually:

1. QA found out NFC-Sharing cannot work, however they cannot know what happened because nothing is showing in logcat.
2. QA called me
3. I need to know theit Gaia commmit, checkout the Gaia part, and enable DEBUG in those files 
4. Check out which part went wrong, it could be NFC Gecko, BT Gecko, NFC gaia or the Bluetooth Transfer from Gaia, ... etc.
5. I discuss with QA to how to file this bug, and CC the devs, and in the comment I'll upload the log I observed and perhaps the problems I found, to help you speed up to figure out the problem.

But unfortunaly this took QA and me a great amount of time and the situation is even not better now.
And because the developers/QA live in different timezone I also don't want to see 
several days passed dev/QA are still discussing how to enable logs.

So I would request YOU ALL to help to improve this situation. And this will help you to understand more quickly what and why the problem is. 

Thank you.

Comment 11

[Garner Lee]

I think it makes sense to tweak real errors into unconditional printed errors, and not just channel everything through debug() --> console.log/console.error/dump on the Gaia side.

For non-developer usage, perhaps "debug" should actually be a settings controlled log level output given the number of components NFC might interact with by necessity (NFC + Bluetooth + WiFi + Secure Elements + ...). And/or, some kind of user system dialog if we know the user might be really interested in a exception/throw hard error.

Comment 12

[Krzysztof Mioduszewski[:tauzen]]

I agree on logging certain errors openly, I will implement it in this bug as I wrote in comment 5. I'm considering to also add filename in the log message so it will be quicker to pinpoint the exact source of the problem. 

From my perspective, I still don't think that logging only errors will be enough. I think that we should think about enabling NFC debug easily by QA - switching DEBUG flags to true in all NFC related files. Two ideas which come to my mind (as a suggestions, I'm not sure it's possible the implement them):
1. Online version. Add "NFC output in ADB" setting in Settings/Developer Debug section (Bluetooth and WiFI is already there). We could use SettingsObserver in NfcManager and on enabled logging we could use NfcManager to enable debug in other components. We could even introduce a request to Gecko which could set the debug in NFC gecko impl.
2. Gaia build version. Introduce a pref for NFC debug in Gaia. Since we're moving the initialisation to bootstrap.js maybe we could read the pref value there and pass it as an argument while instantiating certain modules. This would require more work from QA (change pref, flash gaia, retest).

Comment 13

[arno]

I agree with Krzysztof that only logging errors will not help to isolate problems. Some runtime feature to enable NFC logging would be very helpful. I would suggest to include BluetoothTransfer logging as well as it is a crucial component in NFC sharing.

Comment 14

[Vincent Chang[:vchang][changyihsin]]

Thanks Yoshi to raise this problem, having the runtime feature to enable NFC log should be very useful. Except for gaia/gecko proposal in comment 12, we should also have this feature for nfcd as well.

Comment 15

[(inactive after 6/18) Alive Kuo [:alive]]

I prefer settings proposal to turn on/off the whole nfc debugging. Please do, thanks.

Comment 16

[Dimi Lee [:dimi]]

(In reply to Vincent Chang[:vchang] from comment #14)
> Thanks Yoshi to raise this problem, having the runtime feature to enable NFC
> log should be very useful. Except for gaia/gecko proposal in comment 12, we
> should also have this feature for nfcd as well.

bug 1057918 was created for nfcd part

Comment 17

[Krzysztof Mioduszewski[:tauzen]]

Hello Yoshi, could you advise on the next steps here? I assume we should file one bug for gecko part and one bug for adding the setting and it's handling in NfcManager. Do you agree with this?

Also, should I land this bug (the fixes describe in comment 5). I started to work on this here https://github.com/tauzen/gaia/tree/Bug1055446_debug_cleanup. Since adding the setting will take some time, I think that this bug could be landed now, so it will be easier for QA. What do you think?

Comment 18

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

(In reply to Krzysztof Mioduszewski[:kmioduszewski][:tauzen] from comment #17)
> Hello Yoshi, could you advise on the next steps here? I assume we should
> file one bug for gecko part and one bug for adding the setting and it's
> handling in NfcManager. Do you agree with this?
> 
Yes, also Alive agrees on this. That's the most important part ~
But we have only one week left, if you cannot finish in-time, please ask Wesley/Vincent.

Also I think starting next week, QA might start some testing for v2.1
Any solution can help to improve the situation in v2.0 is a good solution to me :P

For gecko part I will file another bug (maybe next week) but it's not that important as QA can turn on DEBUG in https://wiki.mozilla.org/B2G/QA/Tips_And_Tricks#NFC

> Also, should I land this bug (the fixes describe in comment 5). 
Sure, please do. :)

Comment 19

[Krzysztof Mioduszewski[:tauzen]]

Attachment: pull-request-1055446.txt

I've raised a new bug to add debug setting here Bug 1058088. 

Hello Greg, could you review this patch?

Comment 20

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8478356 [details] [review]
pull-request-1055446.txt

Hello, I've leave some messages on GitHub page. Set review again if you have done the changes or need to discuss with me, thanks.

Comment 21

[Krzysztof Mioduszewski[:tauzen]]

Comment on attachment 8478356 [details] [review]
pull-request-1055446.txt

Hi, Greg. I've fixed the issues which you pointed out on github. Gaia-try build is still in progress but it seems that there won't be any errors related to these changes. Could you take a look once more?

Comment 22

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8478356 [details] [review]
pull-request-1055446.txt

Hello, sorry for the delay reviewing (and sorry for the conflicting...). After the discussion, I think it's good to land it. Thanks.

Comment 23

[Krzysztof Mioduszewski[:tauzen]]

No problem, the conflict was easy to resolve ;). Thanks for the review.

Comment 24

[Carsten Book [:Tomcat]]

master: https://github.com/mozilla-b2g/gaia/commit/205095d504d4f876f38ec12ba375c52481cc3ba3