Closed Bug 1297614 Opened 8 years ago Closed 7 years ago

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

Categories

(Toolkit :: Safe Browsing, defect, P2)

Product:
Toolkit
Component:
Safe Browsing
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: tnguyen, Assigned: tnguyen)

References

Details

(Whiteboard: #sbv4-m8)

Attachments

(2 files)

Remove unused code in toolkit/components/url-classifier/content
90.68 KB, patch
Details | Diff | Splinter Review
Bug 1297614 - Refactor and remove dead code in toolkit/components/url-classifier/content
59 bytes, text/x-review-board-request
francois
: review+
Details 
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)
If we have dead code, we should remove it :)
Priority: -- → P3
See Also: → 1343843
Blocks: 1376591
Assignee: nobody → tnguyen
Status: NEW → ASSIGNED
Priority: P3 → P2
MozReview-Commit-ID: GzVGS1ZTRGL
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
Whiteboard: #sbv4-m8
Another profile:
https://perfht.ml/2tbZIvJ
Summary: Remove unused code in toolkit/components/url-classifier/content → Refactor and remove dead code in toolkit/components/url-classifier/content
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
Attachment #8883208 - Flags: review?(francois) → review-
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)
Thanks for your review, 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 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`
Attachment #8883208 - Flags: review?(francois) → review-
(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 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
Attachment #8883208 - Flags: review?(francois) → review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fdc36d2f4626
Refactor and remove dead code in toolkit/components/url-classifier/content r=francois
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fdc36d2f4626
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1403473
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: