1297614 - Refactor and remove dead code in toolkit/components/url-classifier/content

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Description

[Thomas Nguyen (:tnguyen)]

Some parts in the path are not used currently and could be removed, such as trtable.js (or preference.js, could change to use a native pref service one)

Comment 1

[François Marier [:francois]]

If we have dead code, we should remove it :)

Comment 3

[Thomas Nguyen (:tnguyen)]

Attachment: Remove unused code in toolkit/components/url-classifier/content

MozReview-Commit-ID: GzVGS1ZTRGL

Comment 4

[Thomas Nguyen (:tnguyen)]

Old code profile:
https://perfht.ml/2tbzauL
New code profile:
https://perfht.ml/2sHmWqc
I am running profiler in a pretty fast Mac device (i7) and the createInstance reduces 7ms->1ms

Comment 5

[Thomas Nguyen (:tnguyen)]

Another profile:
https://perfht.ml/2tbZIvJ

Comment 6

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1297614 - Refactor and remove dead code in toolkit/components/url-classifier/content

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

Comment 7

[François Marier [:francois]]

Comment on attachment 8883208 [details]
Bug 1297614 - Refactor and remove dead code in toolkit/components/url-classifier/content

https://reviewboard.mozilla.org/r/154102/#review159354

This is a really nice cleanup!

Just a few things to address before we land.

::: toolkit/components/url-classifier/content/listmanager.js:53
(Diff revision 1)
>   * @constructor
>   */
>  this.PROT_ListManager = function PROT_ListManager() {
>    log("Initializing list manager");
> -  this.prefs_ = new G_Preferences();
> -  this.updateInterval = this.prefs_.getPref("urlclassifier.updateinterval", 30 * 60) * 1000;
> +  try {
> +    this.updateInterval = Services.prefs.getIntPref("urlclassifier.updateinterval")  * 1000;

This pref doesn't exist anymore. We can remove the `try` block and just set it to the default value directly.

::: toolkit/components/url-classifier/content/listmanager.js
(Diff revision 1)
>    this.requestBackoffs_ = {};
>    this.dbService_ = Cc["@mozilla.org/url-classifier/dbservice;1"]
>                     .getService(Ci.nsIUrlClassifierDBService);
>  
> -
> +  Services.obs.addObserver(this, "quit-application");
> -  this.hashCompleter_ = Cc["@mozilla.org/url-classifier/hashcompleter;1"]

Did you mean to remove the hashCompleter initialization here?

::: toolkit/components/url-classifier/content/listmanager.js:118
(Diff revision 1)
> +/**
> + * quit-application callback
> + * Delete all of our data tables which seem to leak otherwise.
> + */
> +
> +PROT_ListManager.prototype.observe = function(aSubject, aTopic, aData) {

I think we also need to cancel the `this.updateCheckers_[updateUrl]` timers here to make sure they don't fire while we're shutting down the listmanager.

Might be as simple as calling `stopUpdateCheckers()`.

::: toolkit/components/url-classifier/content/listmanager.js:261
(Diff revision 1)
> +
>        if (nextUpdate) {
>          updateDelay = Math.min(maxDelayMs, Math.max(0, nextUpdate - Date.now()));
>          log("Next update at " + nextUpdate);
>        }
>        log("Next update " + updateDelay + "ms from now");

This is not related to this bug per se, but we could make this debug line even more useful by converting this to seconds:

    log("Next update " + updateDelay/60000 + " min from now");
    
It's annoying having to make these calculations by hand when you're looking at the debug log :)

::: toolkit/components/url-classifier/content/listmanager.js:531
(Diff revision 1)
>    // As long as the delay is something sane (5 min to 1 day), update
>    // our delay time for requesting updates. We always use a non-repeating
>    // timer since the delay is set differently at every callback.
>    if (delay > maxDelayMs) {
>      log("Ignoring delay from server (too long), waiting " +
>          maxDelayMs + "ms");

We could also convert this log to minutes:

    log("Ignoring delay from server (too long), waiting " + maxDelayMs/60000 + " min");

::: toolkit/components/url-classifier/content/listmanager.js:535
(Diff revision 1)
>      log("Ignoring delay from server (too long), waiting " +
>          maxDelayMs + "ms");
>      delay = maxDelayMs;
>    } else if (delay < minDelayMs) {
>      log("Ignoring delay from server (too short), waiting " +
>          this.updateInterval + "ms");

ditto

::: toolkit/components/url-classifier/content/listmanager.js:538
(Diff revision 1)
>    } else if (delay < minDelayMs) {
>      log("Ignoring delay from server (too short), waiting " +
>          this.updateInterval + "ms");
>      delay = this.updateInterval;
>    } else {
>      log("Waiting " + delay + "ms");

ditto

Comment 8

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8883208 [details]
Bug 1297614 - Refactor and remove dead code in toolkit/components/url-classifier/content

https://reviewboard.mozilla.org/r/154102/#review159354

> This pref doesn't exist anymore. We can remove the `try` block and just set it to the default value directly.

Sure, I thought we still want to keep it, particularly in case we want to test something. 
Removed the try block.

> Did you mean to remove the hashCompleter initialization here?

Yep, nsUrlClassifierHashCompleter is turned into a service and we don't have to set it up early here (it may take time for component manager to setup things)

Comment 9

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8883208 [details]
Bug 1297614 - Refactor and remove dead code in toolkit/components/url-classifier/content

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

Comment 10

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8883208 [details]
Bug 1297614 - Refactor and remove dead code in toolkit/components/url-classifier/content

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

Comment 11

[Thomas Nguyen (:tnguyen)]

Thanks for your review, Francois

Comment 12

[François Marier [:francois]]

Comment on attachment 8883208 [details]
Bug 1297614 - Refactor and remove dead code in toolkit/components/url-classifier/content

https://reviewboard.mozilla.org/r/154102/#review159354

> Sure, I thought we still want to keep it, particularly in case we want to test something. 
> Removed the try block.

You're right, it is used in tests: https://searchfox.org/mozilla-central/search?q=urlclassifier.updateinterval&case=false&regexp=false&path=

However, we now also disable Safe Browsing entirely in these tests, so that pref is redundant. We should remove all mentions of it in the codebase.

Comment 13

[François Marier [:francois]]

Comment on attachment 8883208 [details]
Bug 1297614 - Refactor and remove dead code in toolkit/components/url-classifier/content

https://reviewboard.mozilla.org/r/154102/#review159412

::: toolkit/components/url-classifier/content/listmanager.js:21
(Diff revisions 1 - 3)
>  //      that the listmanagers tables are properly written on updates
>  
>  // Lower and upper limits on the server-provided polling frequency
>  const minDelayMs = 5 * 60 * 1000;
>  const maxDelayMs = 24 * 60 * 60 * 1000;
> +const DEFAULT_UPDATE_INTERVAL = 30 * 60 * 1000;

Let's match the style of the other constants: `defaultUpdateIntervalMs`

Comment 14

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8883208 [details]
Bug 1297614 - Refactor and remove dead code in toolkit/components/url-classifier/content

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

Comment 15

[Thomas Nguyen (:tnguyen)]

(In reply to François Marier [:francois] from comment #12)
> Comment on attachment 8883208 [details]
> Bug 1297614 - Refactor and remove dead code in
> toolkit/components/url-classifier/content
> 
> https://reviewboard.mozilla.org/r/154102/#review159354
> 
> > Sure, I thought we still want to keep it, particularly in case we want to test something. 
> > Removed the try block.
> 
> You're right, it is used in tests:
> https://searchfox.org/mozilla-central/search?q=urlclassifier.
> updateinterval&case=false&regexp=false&path=
> 
> However, we now also disable Safe Browsing entirely in these tests, so that
> pref is redundant. We should remove all mentions of it in the codebse
I see, thanks

I also would like to update a few more dead codes
- remove safeLookup and its callback. I believe it's not used anywhere (even addon,...). Indeed it uses deprecated version of dbservice.lookup() and will not work
- Remove nsIUrlClassifierTable.idl which is not used
- Remove unnessary of BindToObject which may cause 1 ms in profile

https://perf-html.io/public/cfe5c76c2f54845efece5f125c6c841c034e3a9c/calltree/?hiddenThreads=&implementation=js&range=2.4996_2.6945&search=safeb&thread=0&threadOrder=0-2-3-1

Comment 16

[Thomas Nguyen (:tnguyen)]

I posted wrong link, it should be 
https://perf-html.io/public/cfe5c76c2f54845efece5f125c6c841c034e3a9c/calltree/?callTreeFilters=prefixjs-DUdDUiDXjDYb&hiddenThreads=&implementation=js&range=2.4996_2.6945&thread=0&threadOrder=0-2-3-1

Comment 17

[François Marier [:francois]]

Comment on attachment 8883208 [details]
Bug 1297614 - Refactor and remove dead code in toolkit/components/url-classifier/content

https://reviewboard.mozilla.org/r/154102/#review159738

Deleting all of this ancient code is very exciting :)

Make sure you give this a good run through Try (including the firefox-ui-functional) to make sure nothing depends on what we are removing.

::: toolkit/components/url-classifier/content/listmanager.js:121
(Diff revision 4)
> +  if (aTopic == "quit-application") {
> +    for (var name in this.tablesData) {
> +      delete this.tablesData[name];
> +    }
> +    Services.obs.removeObserver(this, "quit-application");
> +    this.stopUpdateCheckers();

I'm not sure if it makes a difference, but maybe we should move this before the for loop to make sure that we stop update timers before we delete the table data.

::: toolkit/components/url-classifier/content/listmanager.js:248
(Diff revision 4)
>            + " provided by " + provider);
>  
>        // Use the initialUpdateDelay + fuzz unless we had previous updates
>        // and the server told us when to try again.
>        let updateDelay = initialUpdateDelay;
>        let targetPref = "browser.safebrowsing.provider." + provider + ".nextupdatetime";

nit: `nextUpdatePref` would be a better name here

Comment 18

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8883208 [details]
Bug 1297614 - Refactor and remove dead code in toolkit/components/url-classifier/content

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154102/diff/4-5/

Comment 19

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8883208 [details]
Bug 1297614 - Refactor and remove dead code in toolkit/components/url-classifier/content

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154102/diff/5-6/

Comment 20

[Thomas Nguyen (:tnguyen)]

Try result
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed0e6a60f58730144e8df79d3bca53808d54fc1f&selectedJob=112161239
And new profile (fast i7 device)
https://perfht.ml/2tjNmlp

Comment 21

[Thomas Nguyen (:tnguyen)]

We have failure https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed0e6a60f58730144e8df79d3bca53808d54fc1f&selectedJob=112159924
which is filed in bug 1374268

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fdc36d2f4626
Refactor and remove dead code in toolkit/components/url-classifier/content r=francois

Comment 23

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

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