Closed Bug 336290 Opened 14 years ago Closed 3 years ago

remove --enable-safe-browsing flag once safebrowsing lands

Categories

(Toolkit :: Safe Browsing, defect, P5)

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: tony, Assigned: jaws)

References

Details

Attachments

(1 file)

After safebrowsing is fully integrated, remove the configure flag and turn on by default in bon echo and trunk.  This is a follow up on to bug 334575 where the configure flag was added.
Reed, this was 3 years back. I'm guessing it's been resolved?
safebrowsing is enabled by default, but the configure flag still exists... we should just remove it.
Assignee: tony → nobody
The current state is:

safe browsing on:
Firefox

safe browsing off:
Fennec Native
Fennec XUL
B2G

While I believe it is our goals and intentions to bring safe browsing to these other platforms, it looks like this configuration flag is still bringing us positive value.

source: https://mxr.mozilla.org/mozilla-central/search?string=MOZ_SAFE_BROWSING&find=confvars.sh&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
Product: Firefox → Toolkit
Priority: -- → P5
Whiteboard: tpe-seceng
Flags: needinfo?(gpascutto)
Assignee: nobody → jaws
Status: NEW → ASSIGNED
OS: Windows XP → All
Hardware: x86 → All
Whiteboard: tpe-seceng
https://reviewboard.mozilla.org/r/44015/#review40799

Something isn't exactly right with this patch as XPCOMUtils.jsm and specialpowersAPI.js fail to get included.

::: old-configure.in
(Diff revision 1)
> -dnl Implicitly enabled by default if building with safe-browsing
> -if test -n "$MOZ_SAFE_BROWSING"; then
> -    MOZ_URL_CLASSIFIER=1
> -fi
>  MOZ_ARG_ENABLE_BOOL(url-classifier,
>  [  --enable-url-classifier Enable url classifier module],

MOZ_URL_CLASSIFIER should probably be default enabled now that MOZ_SAFE_BROWSING is enabled by default.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> MOZ_URL_CLASSIFIER should probably be default enabled now that
> MOZ_SAFE_BROWSING is enabled by default.

We can probably remove MOZ_URL_CLASSIFIER too since it's a requirement of both Safe Browsing and Tracking Protection.
Comment on attachment 8737562 [details]
MozReview Request: Bug 336290 - remove --enable-safe-browsing from configure since its used everywhere. r?gps,gcp

https://reviewboard.mozilla.org/r/44015/#review41027

It is my understanding that any Firefox feature that touches external services needs to have a flag to disable it, as downstream clients like Tor may want to disable it (or replace it with their own mechanisms). This is because pinging external services constitutes a side-channel data leak.

Furthermore, I believe enabling safe browsing by default will have implications for developer builds and possibly Talos (the latter likely already has it disabled by default). IIRC, enabling safe browsing causes the browser to download the safe browsing data files shortly after startup on first run. If we have developer builds doing this, that could have unintended consequences.

What I'm not sure is if this flag needs to be at the build system level or whether it is sufficient to have a preference. I'd like to think a preference is OK. After all, we prefer to have N-1 build system configurations and this patch does remove one variation.
Attachment #8737562 - Flags: review?(gps)
As long as building the code doesn't cause third parties problems, then having the ability to turn features off by preference seems fine.
(In reply to Gregory Szorc [:gps] from comment #8)
> What I'm not sure is if this flag needs to be at the build system level or
> whether it is sufficient to have a preference. I'd like to think a
> preference is OK. After all, we prefer to have N-1 build system
> configurations and this patch does remove one variation.

We have prefs to disable all of Safe Browsing and suppress all of the network requests. As of bug 1259288, they are fully disabled in all of our tests.

These are more likely to keep working than the enabled-by-default configure flags IMHO.
Comment on attachment 8737562 [details]
MozReview Request: Bug 336290 - remove --enable-safe-browsing from configure since its used everywhere. r?gps,gcp

https://reviewboard.mozilla.org/r/44015/#review41317

::: old-configure.in:2801
(Diff revision 1)
>  MOZ_SPELLCHECK=1
>  MOZ_ANDROID_APZ=
>  MOZ_TOOLKIT_SEARCH=1
>  MOZ_UI_LOCALE=en-US
>  MOZ_UNIVERSALCHARDET=1
>  MOZ_URL_CLASSIFIER=

So remove this as well as it's no longer optional?
Attachment #8737562 - Flags: review?(gpascutto) → review+
This was r+ed but did not land?
Flags: needinfo?(jaws)
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/d3ef59a2f50a
remove --enable-safe-browsing from configure since its used everywhere. r=gcp
Flags: needinfo?(jaws)
https://hg.mozilla.org/mozilla-central/rev/d3ef59a2f50a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Blocks: 1300156
You need to log in before you can comment on or make changes to this bug.