1497259 - Limit GV logging in official builds

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P1)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

We should turn off debug logging for official builds of GV so things like URLs are not leaked to the log.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1497259 - 1. Fix log module names; r?droeh

Fix the GV log module names so they are all under the GeckoView group.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1497259 - 2. Don't log auto-fill values; r?droeh

Auto-fill values can contain personal information and must not appear in
the log.

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1497259 - 3. Control GV logging through pref; r?droeh

Use the "geckoview.logging" pref to control GV logging.

Comment 4

[Chris Peterson [:cpeterson]]

The Focus team plans to release a new Focus 7.0.x version soon. We might consider uplifting at least the "Control GV logging through pref" patch to the GV 62 relbranch if it has privacy or performance benefits.

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 9015416 [details]
Bug 1497259 - 1. Fix log module names; r?droeh

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: N/A

User impact if declined: Sensitive URLs and other data can be exposed through Android logs

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Only affects how we do logging in GeckoView

String changes made/needed: None

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 9015416 [details]
Bug 1497259 - 1. Fix log module names; r?droeh

[GeckoView Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for consideration: This bug improves privacy of GeckoView consumers such as Focus

User impact if declined: Sensitive URLs and other data can be exposed to the Android logs.

Fix Landed on Version: 64

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Patch only affects how we do logging in GeckoView

String or UUID changes made by this patch: None

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/50b9d2e6ac5f
1. Fix log module names; r=droeh
https://hg.mozilla.org/integration/autoland/rev/20cd9ac747da
2. Don't log auto-fill values; r=droeh
https://hg.mozilla.org/integration/autoland/rev/0df87bbe161e
3. Control GV logging through pref; r=droeh

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Chris, merge day is on Oct 22nd and moz-release will have beta63 code merge on Oct 15th (start of RC week). If we take this in beta63 this week (Pascal, fyi), will that be sufficient to ensure Focus ultimately gets it as GV63 starting from next week. Thoughts?

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 9015416 [details]
Bug 1497259 - 1. Fix log module names; r?droeh

Removing GV uplift nom, Beta63+ as it's needed for GV/Focus/Klar

Comment 10

[Chris Peterson [:cpeterson]]

(In reply to Ritu Kothari (:ritu) from comment #8)
> Hi Chris, merge day is on Oct 22nd and moz-release will have beta63 code
> merge on Oct 15th (start of RC week). If we take this in beta63 this week
> (Pascal, fyi), will that be sufficient to ensure Focus ultimately gets it as
> GV63 starting from next week. Thoughts?

Focus 8.0 is going to pick up GV 63 (Beta) this week, so we probably don't need to uplift this logging fix to the GV 62 relbranch.

OTOH, the Focus 8.0 release date is not until 11/13 so there might be another Focus 7.0.x dot release before then. If there is, we can revisit this bug after it has landed on GV 64 Nightly and 63 Beta. We can leave this bug marked status-geckoview62=fix-optional to remember it.

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Thanks Chris! If a Focus 7.0.x needs this uplift, I'll be happy to A+.

Comment 12

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/50b9d2e6ac5f
https://hg.mozilla.org/mozilla-central/rev/20cd9ac747da
https://hg.mozilla.org/mozilla-central/rev/0df87bbe161e

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c78886e8b28e
https://hg.mozilla.org/releases/mozilla-beta/rev/6cd7919cc0b8
https://hg.mozilla.org/releases/mozilla-beta/rev/095138ddfce6

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 9015416 [details]
Bug 1497259 - 1. Fix log module names; r?droeh

I was told that we may need a GV62 build soon and that we should consider uplifting this fix. GV62+

Comment 15

[(inactive) Jim Chen [:jchen] [:darchons]]

https://hg.mozilla.org/releases/mozilla-release/rev/d335b16bbb7c162d06b848895dace67b68d776a4
https://hg.mozilla.org/releases/mozilla-release/rev/b24c34189591dd31e16faf40daf7c73f598c23bf
https://hg.mozilla.org/releases/mozilla-release/rev/d66bd740b59ffdcb800dc225cf29fc383d5e1a2b