SafeBrowsing logs to terminal about a bunch of getLists calls, a few seconds after startup

RESOLVED FIXED in Firefox 43

Status

()

Toolkit
Safe Browsing
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dholbert, Assigned: gcp)

Tracking

Trunk
mozilla43
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Starting recently, I'm seeing the following terminalspew just after startup, in Firefox Nightly builds (fresh or non-fresh profile, doesn't seem to matter):
{
SafeBrowsing: 13:28:40 GMT-0700 (PDT): getLists: urlclassifier.phishTable
SafeBrowsing: 13:28:40 GMT-0700 (PDT): getLists: urlclassifier.malwareTable
SafeBrowsing: 13:28:40 GMT-0700 (PDT): getLists: urlclassifier.downloadBlockTable
SafeBrowsing: 13:28:40 GMT-0700 (PDT): getLists: urlclassifier.downloadAllowTable
SafeBrowsing: 13:28:40 GMT-0700 (PDT): getLists: urlclassifier.trackingTable
SafeBrowsing: 13:28:40 GMT-0700 (PDT): getLists: urlclassifier.trackingWhitelistTable
}

It looks like this is coming from this log() line:
http://hg.mozilla.org/mozilla-central/diff/f1b54a9c9f9b/toolkit/components/url-classifier/SafeBrowsing.jsm#l1.13
which was added in bug 1107372.

gcp, do we need this logging to be on globally (in our actual official builds)? I'm guessing it might've been a debugging aid used when writing the patch, and it got left in accidentally perhaps?
(Reporter)

Updated

3 years ago
Flags: needinfo?(gpascutto)
(Reporter)

Updated

3 years ago
Summary: SafeBrowsing logs a bunch of getLists calls just after startup → SafeBrowsing logs to terminal about a bunch of getLists calls, a few seconds after startup
(Assignee)

Comment 1

3 years ago
That should've had an if (debug) yes.
Assignee: nobody → gpascutto
Flags: needinfo?(gpascutto)
(Assignee)

Comment 2

3 years ago
Created attachment 8660759 [details] [diff] [review]
Only do SafeBrowsing debug logging when debug is enabled
Attachment #8660759 - Flags: review?(francois)
(Reporter)

Comment 3

3 years ago
Are you sure we benefit from doing this universally in debug builds? (not behind an off-by-default pref or debug flag?)

Right now it looks like this will just be 6 lines of new startup noise that every build (or every debug build) will log at every single startup. Is this logging important/useful enough to merit that?  (Perhaps we should only be logging if something failed?)

Note that there's an ongoing effort to reduce debug-build logging which erahm spearheaded for a while (and made valiant progress), and [unless this logging is important to print out] this seems to be slipping back against that effort a bit.
(Assignee)

Comment 4

3 years ago
(In reply to Daniel Holbert [:dholbert] from comment #3)
> Are you sure we benefit from doing this universally in debug builds? 

No, which is why that's not what this patch is doing? The debug flag is the one from SafeBrowsing itself: browser.safebrowsing.debug
(Reporter)

Comment 5

3 years ago
Oh, thanks! I should've looked at the patch. I misread the "if (debug)" and "debug as enabled" comments as being about debug-vs.-opt builds.

Glad we're in agreement, and thanks for the quick action here. :)
Comment on attachment 8660759 [details] [diff] [review]
Only do SafeBrowsing debug logging when debug is enabled

Review of attachment 8660759 [details] [diff] [review]:
-----------------------------------------------------------------

Actually, could you use the log() function from https://hg.mozilla.org/integration/mozilla-inbound/rev/07a0d26e2b7d instead?

It allows us to toggle logging on/off at runtime, and it addresses the thing you pointed out in https://bugzilla.mozilla.org/show_bug.cgi?id=1203347#c6.
Attachment #8660759 - Flags: review?(francois)
(Assignee)

Comment 7

3 years ago
Created attachment 8661154 [details] [diff] [review]
Only do SafeBrowsing debug logging when debug is enabled
Attachment #8661154 - Flags: review?(francois)
(Assignee)

Updated

3 years ago
Attachment #8660759 - Attachment is obsolete: true
Comment on attachment 8661154 [details] [diff] [review]
Only do SafeBrowsing debug logging when debug is enabled

Review of attachment 8661154 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/url-classifier/SafeBrowsing.jsm
@@ +22,5 @@
> +    return;
> +  }
> +
> +  var d = new Date();
> +  let msg = "hashcompleter: " + d.toTimeString() + ": " + stuff.join(" ");

This "hashcompleter: " should be "SafeBrowsing: ".
Attachment #8661154 - Flags: review?(francois) → review+
(Assignee)

Comment 9

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d60f93efd7afdcc6bf085dfd4d290e460102f578
Bug 1204367 - Only do SafeBrowsing debug logging when debug is enabled. r=francois
https://hg.mozilla.org/mozilla-central/rev/d60f93efd7af
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.