Last Comment Bug 769960 - Refactor the terrifying code in nsSafebrowsingApplication.js
: Refactor the terrifying code in nsSafebrowsingApplication.js
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Safe Browsing (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 17
Assigned To: Justin Dolske [:Dolske]
:
Mentors:
: 382635 (view as bug list)
Depends on: 778855 779107
Blocks: 775851 778606
  Show dependency treegraph
 
Reported: 2012-06-30 15:26 PDT by Justin Dolske [:Dolske]
Modified: 2014-05-27 12:25 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v.0 (WIP) (35.34 KB, patch)
2012-06-30 15:26 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Patch v.1 (43.58 KB, patch)
2012-06-30 19:55 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Patch v.2 (45.55 KB, patch)
2012-07-03 20:37 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Patch v.3 (45.66 KB, patch)
2012-07-03 21:56 PDT, Justin Dolske [:Dolske]
fryn: review+
Details | Diff | Splinter Review
Patch v.4 (47.60 KB, patch)
2012-07-19 19:23 PDT, Justin Dolske [:Dolske]
gpascutto: feedback+
Details | Diff | Splinter Review
Patch v.5 (47.44 KB, patch)
2012-07-27 23:08 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review

Description Justin Dolske [:Dolske] 2012-06-30 15:26:26 PDT
Created attachment 638154 [details] [diff] [review]
Patch v.0 (WIP)

Over in bug 749519 myk was a mean, mean person and made me look at some safe browsing code. After recovering from my shock and repulsion at the code, I decided it would be fun and relaxing to rewrite the hell out of it.

First rough-cut attached (untested, unrun, likely broken). Replaces ~750 lines of insanity with a nice ~200 line JSM.
Comment 1 Frank Yan (:fryn) 2012-06-30 15:50:25 PDT
Comment on attachment 638154 [details] [diff] [review]
Patch v.0 (WIP)

Review of attachment 638154 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/safebrowsing/SafeBrowsing.jsm
@@ +91,5 @@
> +
> +  getURLPref: function(prefName) {
> +    const MOZ_OFFICIAL_BUILD = false
> +#ifdef OFFICIAL_BUILD
> +    MOZ_OFFICIAL_BUILD = true;

I think you'll have to use #else like the old code in globalstore.js, since the const declaration above will lock the value to false.
Comment 2 Justin Dolske [:Dolske] 2012-06-30 17:22:49 PDT
(In reply to Frank Yan (:fryn) from comment #1)
 
> I think you'll have to use #else like the old code in globalstore.js, since
> the const declaration above will lock the value to false.

Awesome, I was afraid it would be hard to find a reviewer. :D
Comment 3 Justin Dolske [:Dolske] 2012-06-30 19:55:11 PDT
Created attachment 638166 [details] [diff] [review]
Patch v.1

This seems to be working.

I moved sb-loader.js to browser-safebrowsing.js, and will likely eventually (separate bug) move some other safe browsing stuff from browser.js into there.

17 files changed, 201 insertions(+), 806 deletions(-)

*excellent*
Comment 4 Justin Dolske [:Dolske] 2012-06-30 19:58:39 PDT
Oh, and turns out mfinkle already did a rewrite of this over in mobile land, but instead of sharing his toys has been hoarding it all to himself.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/SafeBrowsing.js

I've excised a bunch more, though, so we'll probably want to end up moving this JSM to toolkit (?) so we can all share. Will look at doing that in a followup.
Comment 5 Justin Dolske [:Dolske] 2012-07-02 15:29:07 PDT
(Oops, meant to CC :gcp not :gps)
Comment 6 Gian-Carlo Pascutto [:gcp] 2012-07-02 16:09:47 PDT
Mark Finkle's rewrite doesn't work correctly. The SafeBrowsing tests we have right now will tell you a lot of things, but not if it's actually doing anything :P

The problem is that there's no (not even an approximate) implementation or emulation of the server side, so all the tests just shove the updates in the UrlClassifier directly. This doesn't tell you anything whether you're actually communicating with the server correctly.

I am working on fixing much of this, but it will be a few weeks before there's anything that can help you.

Tread carefully, here be dragons.
Comment 7 Dão Gottwald [:dao] 2012-07-03 06:43:44 PDT
Comment on attachment 638166 [details] [diff] [review]
Patch v.1

>--- /dev/null
>+++ b/browser/base/content/browser-safebrowsing.js

>+    var uri = getBrowser().currentURI;

>+    var pageUrl = getBrowser().currentURI.asciiSpec;

nit: gBrowser.currentURI

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js

>   _delayedStartup: function(isLoadingBlank, mustLoadSidebar) {
>     let tmp = {};
>     Cu.import("resource:///modules/TelemetryTimestamps.jsm", tmp);
>     let TelemetryTimestamps = tmp.TelemetryTimestamps;
>     TelemetryTimestamps.add("delayedStartupStarted");
>     gDelayedStartupTimeoutId = null;
> 
>+#ifdef MOZ_SAFE_BROWSING
>+    Cu.import("resource://gre/modules/SafeBrowsing.jsm", window);
>+#endif

>--- /dev/null
>+++ b/browser/components/safebrowsing/SafeBrowsing.jsm

>+SafeBrowsing.init();

This looks like it should be initialized in nsBrowserGlue.js (_onProfileStartup).

Which of the functions should really be exported? updateListManager and controlUpdateChecking appear to exist for internal use only, for instance.
Comment 8 Justin Dolske [:Dolske] 2012-07-03 20:37:29 PDT
Created attachment 638953 [details] [diff] [review]
Patch v.2

Took a closer look at if this was actually working, fixed a few things and filled in some XXX's.

Something's still broken, though. Normally, after a minute or two of idle, nsUrlClassifierDBServiceWorker::SetupUpdate() does its stuff a flood of data pours in. But with my patch SetupUpdate() goes, and then the request fails:

1885568192[1003351a0]: Update URL is (snip)
580395008[11c4ceda0]: nsUrlClassifierDBServiceWorker::SetupUpdate
1885568192[1003351a0]: HTTP request returned failure code.
1885568192[1003351a0]: OnStopRequest (status 80004004)

Not sure what I've done wrong, I'm comparing various bits of internal state with/without the patch and so far nothing looks different... Hmm.
Comment 9 Justin Dolske [:Dolske] 2012-07-03 21:56:32 PDT
Created attachment 638960 [details] [diff] [review]
Patch v.3

Ah. There we go. I thought I had retained the ordering of calls to the stuff over in toolkit, but some logging showed that wasn't actually the case. Reordered some stuff in my init(), and now I see the DB updating after a couple minutes.

Confirmed with a random entry from http://www.phishtank.com/.
Comment 10 Dão Gottwald [:dao] 2012-07-04 01:40:55 PDT
Please address comment 7. Implicit initialization when importing a module for the first time is frowned upon.
Comment 11 Frank Yan (:fryn) 2012-07-06 17:22:04 PDT
Comment on attachment 638960 [details] [diff] [review]
Patch v.3

Review of attachment 638960 [details] [diff] [review]:
-----------------------------------------------------------------

I agree with Dão about comment 7, but I don't see why we need to run the SafeBrowsing.jsm file to completion that early in our startup process.
If we must pull the init call out of the JSM into browser.js, we could simply have the JSM itself boolean-check whether it's already been initialized.

::: browser/components/safebrowsing/SafeBrowsing.jsm
@@ +20,5 @@
> +  let debug = true; // XXX hook me up to a pref
> +  if (!debug)
> +    return;
> +
> +  if (!stuff)

This will never happen. If there are no arguments, stuff will be an empty Array.
If we don't want to log anything when stuff has length zero, then we should check for !stuff.length.

@@ +139,5 @@
> +    const localePref = "general.useragent.locale";
> +
> +    let locale = Services.prefs.getCharPref(localePref);
> +    try {
> +      // Dumb. This API only works if pref is localied or has a user value.

"localied": Freudian slip? ;)
Comment 12 Frank Yan (:fryn) 2012-07-06 17:23:46 PDT
To clarify, I mean I agree with Dão in principle on the following:

(In reply to Dão Gottwald [:dao] from comment #10)
> Implicit initialization when importing a module
> for the first time is frowned upon.
Comment 13 Justin Dolske [:Dolske] 2012-07-19 19:23:26 PDT
Created attachment 644137 [details] [diff] [review]
Patch v.4

Jared wisely suggested that this would be safe to land near the beginning of a cycle (that's now!) than the end (during previous work!). So let's get this rolling again.

Address previous review comments. Made it init() more like other modules, and split up the pref reading since the URL stuff isn't exactly dynamic (and I may refactor that out later, too).

I'm leaving the init() in delayedStartup, since that seems closer to the existing code (and there's already enough churn here). I'm also not worrying about symbol visibility, can look at that in the next step of refactoring and collapsing into toolkit code.

GCP: Do these changes (SafeBrowsing.jsm in particular) look ok to you?
Comment 14 Gian-Carlo Pascutto [:gcp] 2012-07-23 08:24:35 PDT
Comment on attachment 644137 [details] [diff] [review]
Patch v.4

Review of attachment 644137 [details] [diff] [review]:
-----------------------------------------------------------------

Even though it feels a bit weird given on the number of XXX left over, I'm going to feedback+ this because it's a spectacular reduction and the end result is something that's easier to understand (and fix the XXX's in). Please see the remarks for things that can make this easier to deal with for the brave knights that follow in your footsteps.

As far as I can tell it still works.

You'll want a module peer for r+, I'd be quite willing to pretend I'm one for the C++ part of SafeBrowsing, but for the JS parts that'd be pushing it.

::: browser/app/profile/firefox.js
@@ +720,5 @@
>  
>  // Prevent loading of pages identified as malware
>  pref("browser.safebrowsing.malware.enabled", true);
>  
> +// Debug logging to error console

There's components/url-classifier/content/moz/debug.js which (AFAIK) dumps to the console, and the C++ parts log with PR_LOG and several subsections there.

It looks like this adds yet another place where you must toggle a flag if you want logging for SB to be enabled, and that might make things harder in the long run.

::: browser/components/safebrowsing/SafeBrowsing.jsm
@@ +106,5 @@
> +      return;
> +    }
> +
> +    // XXX Seems like all these prefs could be hardcoded here, they don't need to be user-adjustable.
> +

If these allow replacing the Google URL's by custom ones, they most definitely have to stay user-adjustable. I'd like to be able to point them to a custom provider while we run our tests, for example.

@@ +133,5 @@
> +#endif
> +
> +    let url = Services.prefs.getCharPref(prefName);
> +
> +    // XXX Allow no-default pref "browser.safebrowsing.clientid" to override this?

I see no point in this.

@@ +137,5 @@
> +    // XXX Allow no-default pref "browser.safebrowsing.clientid" to override this?
> +    let clientName = MOZ_OFFICIAL_BUILD ? "navclient-auto-ffox" : Services.appinfo.name;
> +
> +    // XXX Allow no-default pref "browser.safebrowsing.clientver" to override this?
> +    let clientVersion = Services.appinfo.version;

Same, I see no point in making it configurable.
Comment 15 Justin Dolske [:Dolske] 2012-07-25 17:46:01 PDT
(In reply to Gian-Carlo Pascutto (:gcp) from comment #14)

> You'll want a module peer for r+, I'd be quite willing to pretend I'm one
> for the C++ part of SafeBrowsing, but for the JS parts that'd be pushing it.

You're probably the closest thing to an eligible reviewer, since no one has touched this code since dcamp did in 'aught seven. I'm a browser and toolkit peer, so if you and fryn are OK with the changes, that's official enough. :)


> There's components/url-classifier/content/moz/debug.js which (AFAIK) dumps
> to the console, and the C++ parts log with PR_LOG and several subsections
> there.
> 
> It looks like this adds yet another place where you must toggle a flag if
> you want logging for SB to be enabled, and that might make things harder in
> the long run.

Bug 775851 will take care of debug.js there. I'm not too concerned about having 2 logging mechanisms (PR_LOG and JS), but we could look at unifying the controlling flag in a later bug if it's a pain.


> > +    // XXX Seems like all these prefs could be hardcoded here, they don't need to be user-adjustable.
> 
> If these allow replacing the Google URL's by custom ones, they most
> definitely have to stay user-adjustable. I'd like to be able to point them
> to a custom provider while we run our tests, for example.

I'll clarify; what I meant to say is something along the lines of having the _defaults_ hardcoded, while still allowing a way to override when useful (like tests). Actually, I'll just remove the comment, it's not a big deal and acn be dealt with in a future cleanup if I'm so motivated to.


> > +    // XXX Allow no-default pref "browser.safebrowsing.clientid" to override this?
> 
> I see no point in this.

Nuked.


> > +    // XXX Allow no-default pref "browser.safebrowsing.clientver" to override this?
>
> Same, I see no point in making it configurable.

Nuked.
Comment 16 Justin Dolske [:Dolske] 2012-07-27 23:08:19 PDT
Created attachment 646814 [details] [diff] [review]
Patch v.5

Had a bad run of luck with memory leaks, not completely sure what was happening. Had orange on Try, fixed some things, was still leaking locally even without (!) my patch, and now seems good. Seems the global |listManager| may have been to blame?

All green on last Try run: https://tbpl.mozilla.org/?tree=Try&rev=4539b9a441ba
Comment 18 Justin Dolske [:Dolske] 2012-07-28 14:28:56 PDT
18 files changed, 289 insertions(+), 806 deletions(-) (64% fewer lines) \o/
Comment 19 Ryan VanderMeulen [:RyanVM] 2012-07-28 18:33:58 PDT
https://hg.mozilla.org/mozilla-central/rev/3b312c31d1cd
Comment 20 Mark Finkle (:mfinkle) (use needinfo?) 2012-07-28 21:10:53 PDT
Is there any reason we can't move SafeBrowsing.jsm to toolkit? Mobile is likely to just fork this file otherwise.
Comment 21 Philip Chee 2012-07-28 21:25:21 PDT
> Is there any reason we can't move SafeBrowsing.jsm to toolkit? Mobile is likely to just
> fork this file otherwise.
Thunderbird is already using a very old fork of safebrowsing. I'm sure they would prefer to have one less pile of code that they have to remember to update now that they are a community project.
Comment 22 Philip Chee 2012-07-28 22:00:48 PDT
> +  Cu.import("resource://gre/modules/SafeBrowsing.jsm", tmp);
This should be "resource:///modules/SafeBrowsing.jsm")
Comment 23 Justin Dolske [:Dolske] 2012-07-30 15:32:52 PDT
(In reply to Mark Finkle (:mfinkle) from comment #20)
> Is there any reason we can't move SafeBrowsing.jsm to toolkit?

Indeed, that's exactly my intention. See bug 778608. :)
Comment 24 Justin Dolske [:Dolske] 2012-07-31 19:13:15 PDT
Adding 778606 just so _this_ bug can serve as a bit of a metabug to track changes associated with the refactoring.
Comment 25 Philip Chee 2012-12-20 07:56:06 PST
*** Bug 382635 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.