Closed
Bug 1204367
Opened 9 years ago
Closed 9 years ago
SafeBrowsing logs to terminal about a bunch of getLists calls, a few seconds after startup
Categories
(Toolkit :: Safe Browsing, defect)
Toolkit
Safe Browsing
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: dholbert, Assigned: gcp)
References
Details
Attachments
(1 file, 1 obsolete file)
3.62 KB,
patch
|
francois
:
review+
|
Details | Diff | Splinter Review |
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•9 years ago
|
Flags: needinfo?(gpascutto)
Reporter | ||
Updated•9 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•9 years ago
|
||
That should've had an if (debug) yes.
Assignee: nobody → gpascutto
Flags: needinfo?(gpascutto)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8660759 -
Flags: review?(francois)
Reporter | ||
Comment 3•9 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•9 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•9 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 6•9 years ago
|
||
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•9 years ago
|
||
Attachment #8661154 -
Flags: review?(francois)
Assignee | ||
Updated•9 years ago
|
Attachment #8660759 -
Attachment is obsolete: true
Comment 8•9 years ago
|
||
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•9 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
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•