1377559 - Should store value of preference browser.safebrowsing.debug to reuse

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, enhancement, P2)

Description

[Thomas Nguyen (:tnguyen)]

I see in url-classifier code that preference browser.safebrowsing.debug (in log(...))is read a lot of time, should cache it somewhere, listen to value change, and reuse

Comment 1

[Thomas Nguyen (:tnguyen)]

Attachment: Cache debug pref

MozReview-Commit-ID: 1yWe7wB0ARl

Comment 2

[Thomas Nguyen (:tnguyen)]

Wip patch, I would like to wait for landing bug 1297614 first

Comment 3

[Thomas Nguyen (:tnguyen)]

I would like to do in this bug because, in some profiles, reading pref may take time
https://perf-html.io/public/cfe5c76c2f54845efece5f125c6c841c034e3a9c/calltree/?callTreeFilters=prefixjs-DUdDUiDXjDXk&hiddenThreads=&implementation=js&range=2.4996_2.6945&thread=0&threadOrder=0-2-3-1

Comment 4

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1377559 - Should store value of preference browser.safebrowsing.debug to reuse

Review commit: https://reviewboard.mozilla.org/r/155054/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/155054/

Comment 5

[François Marier [:francois]]

Comment on attachment 8884143 [details]
Bug 1377559 - Should store value of preference browser.safebrowsing.debug to reuse

https://reviewboard.mozilla.org/r/155054/#review160370

::: toolkit/components/url-classifier/SafeBrowsing.jsm:13
(Diff revision 1)
>  const Ci = Components.interfaces;
>  const Cu = Components.utils;
>  
>  Cu.import("resource://gre/modules/Services.jsm");
>  
> +let loggingEnabled = false;

Let's add a constant here, just like in listmanager.js

::: toolkit/components/url-classifier/content/listmanager.js:23
(Diff revision 1)
>  // Lower and upper limits on the server-provided polling frequency
>  const minDelayMs = 5 * 60 * 1000;
>  const maxDelayMs = 24 * 60 * 60 * 1000;
>  const defaultUpdateIntervalMs = 30 * 60 * 1000;
>  
> +const PREF_LOGGING_ENABLED = "browser.safebrowsing.debug";

The `loggingEnabled` variable makes sense to me, but I think the constant should be `PREF_DEBUG_ENABLED` to avoid confusion.

::: toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js:18
(Diff revision 1)
>  const COMPLETE_LENGTH = 32;
>  const PARTIAL_LENGTH = 4;
>  
>  // Upper limit on the server response minimumWaitDuration
>  const MIN_WAIT_DURATION_MAX_VALUE = 24 * 60 * 60 * 1000;
> +const PREF_LOGGING_ENABLED = "browser.safebrowsing.debug";

`PREF_DEBUG_ENABLED`

Comment 6

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8884143 [details]
Bug 1377559 - Should store value of preference browser.safebrowsing.debug to reuse

https://reviewboard.mozilla.org/r/155054/#review160606

Thanks you for your review

Comment 7

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8884143 [details]
Bug 1377559 - Should store value of preference browser.safebrowsing.debug to reuse

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155054/diff/1-2/

Comment 8

[François Marier [:francois]]

Comment on attachment 8884143 [details]
Bug 1377559 - Should store value of preference browser.safebrowsing.debug to reuse

https://reviewboard.mozilla.org/r/155054/#review160812

::: toolkit/components/url-classifier/SafeBrowsing.jsm:196
(Diff revision 2)
>      // skip nextupdatetime and lastupdatetime
>      if (aData.indexOf("lastupdatetime") >= 0 || aData.indexOf("nextupdatetime") >= 0) {
>        return;
>      }
> +
> +    if (aData == PREF_DEBUG_ENABLED") {

This stray `"` is causing a syntax error.

Comment 9

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8884143 [details]
Bug 1377559 - Should store value of preference browser.safebrowsing.debug to reuse

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155054/diff/2-3/

Comment 10

[François Marier [:francois]]

Comment on attachment 8884143 [details]
Bug 1377559 - Should store value of preference browser.safebrowsing.debug to reuse

https://reviewboard.mozilla.org/r/155054/#review161366

Comment 11

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=010554bd58cdd211d1e7a1a122bdee337225ee0d

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f95be8a71102
Should store value of preference browser.safebrowsing.debug to reuse r=francois

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f95be8a71102