881672 - Enabling a DUMP function for all Gaia apps

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

We should have a generic way to enable/disable DEBUG traces in Gaia.

The approach can be enabling a DUMP function for all Gaia apps
:
 • one pref to rule them all (e.g. `debug.gaia.enabled') — we could even make it true by default on non-production builds;
 • one shared library (`shared/js/debug/js') to expose a DUMP object that we’d use like a `console.log()' or a `dump()';
 • replace all current `console.log()' calls in Gaia by a DUMP() call.

In order to make this debug data easier to grep, this DUMP() function would automatically prepend the application name (e.g. relying on document.location) in the console.log message.

Comment 1

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

This bug comes from the discussion in bug 875679 and the attachment https://bugzilla.mozilla.org/attachment.cgi?id=759685.

Related comments: #5, #6, #7 and #9

Comment 2

[Fabien Cazenave [:kaze]]

Quite a few STK-related issues are blocking (leo+) and are difficult to reproduce on development builds; to fix these blockers, our testers need a way to enable DEBUG traces without having to flash their device.

Of course, this patch would help debugging all Gaia apps — not only the STK features; but as it’d help debugging open leo+ issues, I tend to think we should address this bug ASAP, leo+ or not.

Comment 3

[Jason Smith [:jsmith]]

One thing I would like to see here is an approach that QA can take full advantage of during daily testing. The global preference is definitely a good idea to consider.

Comment 4

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Adding shared Gaia debug class

Examples in system and settings here:

 System: https://github.com/frsela/gaia/compare/STK;Bug881672_SystemApp
 Settings: https://github.com/frsela/gaia/compare/STK;Bug881672_SettingsApp

Kaze, I suppose each app should be modified in a different patch but ¿one independent bug per app? or we can collect all patches here?

Comment 5

[Fabien Cazenave [:kaze]]

I think it’s safer to open one patch per app and make them depend on this bug. Obviously we want the Settings and System patches first.

Comment 6

[Fabien Cazenave [:kaze]]

Comment on attachment 761327 [details]
Adding shared Gaia debug class

I’m not sure this is the best approach. Let’s discuss that on your pull request.

Comment 7

[Fabien Cazenave [:kaze]]

Comment on attachment 761327 [details]
Adding shared Gaia debug class

Clearing the review flag, please r? me again when ready.

Comment 8

[Dietrich Ayala (:dietrich)]

Is this enabled for 3rd party apps too? Could a 3rd party app dumpflood the phone?

Comment 9

[Fabien Cazenave [:kaze]]

No, unless the 3rd-party app includes the shared debug.js lib and has settings privileges.
However, as long as console.log() is enabled, console flooding is possible (and easy).

Maybe we need a way to disable console.[log|warn|error] calls, but that’s outside of the scope of this patch.

Comment 10

[Alex Keybl [:akeybl]]

Let's patch our local builds, but not take any extra risk for 1.1 at this point.

Comment 11

[Fabien Cazenave [:kaze]]

Comment on attachment 761327 [details]
Adding shared Gaia debug class

r=me, please squash your commits before merging.

Comment 12

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Landed: 5d6fbaa19afc6e15aa8449d6ba5e4a3cb680b0ec

Comment 13

[Fabien Cazenave [:kaze]]

(In reply to Alex Keybl [:akeybl] from comment #10)
> Let's patch our local builds, but not take any extra risk for 1.1 at this
> point.

The problem is, patching local builds didn’t work so far. That’s why Fernando proposed a way to enable these traces directly from Gaia, and I agree it’s necessary to have these before refactoring the whole STK/ICC code base — which is a leo+ task.

This library doesn’t add any risk because it’s not used by any other Gaia app at the moment. I would like to uplift it to v1-train in order to enable STK/ICC testing.

Comment 14

[Noemí Freire (:noemi)]

So we re-nominate this bug to leo, as mentioned, it is important to quickly uplift it to v1-train in order to test STK/ICC patches

Comment 15

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 5d6fbaa19afc6e15aa8449d6ba5e4a3cb680b0ec to:
v1-train: 74684bda85753fcedcceb128ef1dca9ff3071f54

Comment 16

[John Ford [:jhford] CET/CEST Berlin Time]

1.1hd: 74684bda85753fcedcceb128ef1dca9ff3071f54