Closed Bug 1204367 Opened 4 years ago Closed 4 years ago

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

Categories

(Toolkit :: Safe Browsing, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: dholbert, Assigned: gcp)

References

Details

Attachments

(1 file, 1 obsolete file)

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?
Flags: needinfo?(gpascutto)
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
That should've had an if (debug) yes.
Assignee: nobody → gpascutto
Flags: needinfo?(gpascutto)
Attachment #8660759 - Flags: review?(francois)
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.
(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
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)
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+
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
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.