388652 - remove code that supports deprecated "/lookup" request in SafeBrowsing protocol

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Description

[Marria Nazif]

We would like to remove support for enhanced mode, while moving to the new protocol.

Comment 1

[Jesse Ruderman]

What is being removed?  Does the new protocol make it unnecessary, or not have the feature at all, or what?

Comment 2

[Marria Nazif]

The new protocol only touches the "regular" mode protocol (see bug 387196).  Technically we could keep enhanced mode as is with the new protocol.  However, we were hoping to take advantage of the code refactor to clean up and remove the less common code path.

Comment 3

[Jesse Ruderman]

I'm guessing "enhanced mode" refers to real-time protection and "regular mode" refers to the 30-minutes-lagged protection that's currently the default.  I'd like to see real-time protection remain, but with Firefox simply asking for updates to the blocklist each time a new site is visited rather than sending the URL of the site to Google.

Comment 4

[Marria Nazif]

That is correct, "enhanced mode" is real-time protection.  I agree that it would be nice to modify the "regular mode" to request updates more regularly, but it won't happen for the initial release.

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Morphing slightly to make it clear that this bug is about removing the support for /lookup.

Jesse: could you file a new bug about morphing that pref to change the update frequency? I think that's a really good idea. Even if the request results in a throttle-response, we can alert the user to the fact that they're browsing with an out of date list in the enhanced mode.

Comment 6

[Jesse Ruderman]

Ok.  I filed bug 401811, "Replace 'Check by asking Google about each site I visit' with a more-frequent-update option".

Comment 7

[Johnathan Nightingale [:johnath]]

Re-nom'ng to bump priority.  As bug 412497 demonstrates, some portion of this bug really needs to be fixed.  We might view removing the enhanced mode code as more of a "nice to have, code cleanliness" thing, but the current state of affairs is that the pref UI still exposes the option to do live checking, even though we no longer support it.  We could just pave over the UI in a separate bug and leave the backend cleanup for later, but dcamp argues in 412497 that cleaning up the whole thing is probably a pretty easy fix.

My recommendation is to mark this P2 because I don't think the final release should ship with this UI exposed, and to resolve 412497 as a dup of this bug. 

Comment 8

[Mike Connor [:mconnor]]

We should remove the prefs ASAP, I didn't realize this wasn't done already.

Comment 9

[Dave Camp (:dcamp)]

Attachment: v1

This patch removes the pref and some related code in the safebrowsing component.

Comment 10

[Johnathan Nightingale [:johnath]]

Can we remove browser/components/safebrowsing/content/warning-overlay.xul (and associated jar.mn entry) as well?  Seems like they aren't needed any more, and there might even be a perf win for getting rid of all that xul overlay at startup/winopen.

Comment 11

[Tony Chang (Google)]

Comment on attachment 301553 [details] [diff] [review]
v1

Did the request backoff logic move elsewhere?

I agree with Johnathan that we should remove the overlay .xul file and the strings used in it, but I'm ok with multiple patches to delete code.  We can probably also remove some unused JS code in url-classifier jslib.

I think we can also drop phishing-afterload-displayer.js and perhaps most of browser-view.js.

Comment 12

[Dave Camp (:dcamp)]

(In reply to comment #11)
> (From update of attachment 301553 [details] [diff] [review])
> Did the request backoff logic move elsewhere?

The request backoff for updates is still in listmanager.js, this just removes backoff for the (no-longer-there) individual lookup requests.

Comment 13

[Axel Hecht [:Pike]]

Fine from an l10n point of view, just string removals, no context changes for strings around. Fine to go without late-l10n.

Comment 14

[Dave Camp (:dcamp)]

Attachment: patch without string removals

Axel suggested that I land the string removals after making sure the rest of the patch doesn't need backing out after landing, so I'm attaching the two separate patches here.

Comment 15

[Dave Camp (:dcamp)]

Attachment: string removals

Comment 16

[Reed Loden [:reed]]

Landed without string removals in case I have to back it out.

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.276; previous revision: 1.275
done
Checking in browser/components/preferences/security.js;
/cvsroot/mozilla/browser/components/preferences/security.js,v  <--  security.js
new revision: 1.15; previous revision: 1.14
done
Checking in browser/components/preferences/security.xul;
/cvsroot/mozilla/browser/components/preferences/security.xul,v  <--  security.xul
new revision: 1.22; previous revision: 1.21
done
Checking in browser/components/safebrowsing/content/controller.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/controller.js,v  <--  controller.js
new revision: 1.11; previous revision: 1.10
done
Checking in browser/components/safebrowsing/content/globalstore.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/globalstore.js,v  <--  globalstore.js
new revision: 1.20; previous revision: 1.19
done
Checking in browser/components/safebrowsing/content/list-warden.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/list-warden.js,v  <--  list-warden.js
new revision: 1.17; previous revision: 1.16
done
Checking in browser/components/safebrowsing/content/malware-warden.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/malware-warden.js,v  <--  malware-warden.js
new revision: 1.8; previous revision: 1.7
done
Checking in browser/components/safebrowsing/content/phishing-warden.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/phishing-warden.js,v  <--  phishing-warden.js
new revision: 1.28; previous revision: 1.27
done
Checking in browser/components/safebrowsing/content/reporter.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/reporter.js,v  <--  reporter.js
new revision: 1.6; previous revision: 1.5
done
Checking in browser/components/safebrowsing/content/sb-loader.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/sb-loader.js,v  <--  sb-loader.js
new revision: 1.26; previous revision: 1.25
done
Removing browser/components/safebrowsing/content/tr-fetcher.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/tr-fetcher.js,v  <--  tr-fetcher.js
new revision: delete; previous revision: 1.11
done
Checking in browser/components/safebrowsing/src/nsSafebrowsingApplication.js;
/cvsroot/mozilla/browser/components/safebrowsing/src/nsSafebrowsingApplication.js,v  <--  nsSafebrowsingApplication.js
new revision: 1.11; previous revision: 1.10
done

Comment 17

[Reed Loden [:reed]]

Checking in browser/locales/en-US/chrome/browser/preferences/security.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/preferences/security.dtd,v  <--  security.dtd
new revision: 1.8; previous revision: 1.7
done