Closed Bug 1237198 Opened 8 years ago Closed 8 years ago

[experiment] Block SWFs on the content blocking list hosted on the Shavar service 0.6.9

Categories

(Core Graveyard :: Plug-ins, defect)

defect
Not set
normal

Tracking

(firefox44 wontfix, firefox45 wontfix, firefox46 affected, firefox47 fixed, firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox44 --- wontfix
firefox45 --- wontfix
firefox46 --- affected
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: cpeterson, Assigned: tschneider)

References

Details

(Whiteboard: experiment)

User Story

This beta experiment will test multiple blocklist configurations for invisible plugin content in Firefox Beta 46. We will compare the different lists' effects on stability and performance metrics like crash rate and page load time. The experiment's blocklists are hosted on GitHub:

https://github.com/mozilla-services/shavar-plugin-blocklist

The beta experiment will measure three cohorts:

1. Block no plugin content. This is the control group.

2. Block some plugin content used for fingerprinting and super cookies.

3. Additionally block some plugin content used to watch the visibility of page elements. This type of plugin content sometimes continuously polls the page elements, causing inefficient DOM invalidations. A better solution for watching elements will be the proposed Intersection Observer API (bug 1243846).

Attachments

(1 file, 18 obsolete files)

30.59 KB, patch
Details | Diff | Splinter Review
We can use the current Tracking Protection Shavar list as a placeholder until we have the real Flash blocklist in Shavar.
This first implementation blocks all plugin content on the current Tracking Protection Shavar list. This is not limited to small ("utility") content, but blocks everything that matches. It uses the same UI as for blocked-for-security content. For some reason it only works when e10s is disabled. Otherwise I'm not able to query the URI classifier service. Not quite sure who I can ask for help here.

Chris: Do we want to hard code the size requirements (<= 5x5) or do we make this just part of the rule set when creating the blocking list?
Flags: needinfo?(cpeterson)
(In reply to Tobias Schneider [:tobytailor] from comment #1)
> For some reason it only works when e10s is
> disabled. Otherwise I'm not able to query the URI classifier service. Not
> quite sure who I can ask for help here.

I would ask billm or jimm in #e10s.

> Chris: Do we want to hard code the size requirements (<= 5x5) or do we make
> this just part of the rule set when creating the blocking list?

That's a good question. I think we should try to keep all the policy decisions in the block list, not the code. I think this would be a cleaner separation of policy and mechanism. Plus the list will be easier to update than the code if we find a problem in the Release channel.
Flags: needinfo?(cpeterson)
Comment on attachment 8706388 [details] [diff] [review]
Patch 1: Block SWFs on the current Tracking Protection Shavar list.

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

::: dom/base/nsObjectLoadingContent.cpp
@@ +3264,5 @@
>  
> +  if (Preferences::GetBool("plugins.block_for_stability.enabled", false)) {
> +    nsCOMPtr<nsIURIClassifier> classifier =
> +      do_GetService(NS_URICLASSIFIERSERVICE_CONTRACTID);
> +    if (classifier && mURI) {

When is mURI null? If it is a common case, maybe move the mURI check the pref check? This will avoid executing GetBool() and do_GetService().

@@ +3266,5 @@
> +    nsCOMPtr<nsIURIClassifier> classifier =
> +      do_GetService(NS_URICLASSIFIERSERVICE_CONTRACTID);
> +    if (classifier && mURI) {
> +      nsAutoCString tables;
> +      Preferences::GetCString("plugins.block_for_stability.table", &tables);

Instead of loading the table name pref on every plugin load, can we register a pref observer like we do for the phishing and malware tables? Or just cache the table name on startup or just hardcode the name? The phishing and malware table observer code has a comment asking that same question. I doubt anyone but you will ever change this pref. :)

https://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#1164

::: toolkit/components/url-classifier/SafeBrowsing.jsm
@@ +158,4 @@
>      this.debug = Services.prefs.getBoolPref("browser.safebrowsing.debug");
>      this.phishingEnabled = Services.prefs.getBoolPref("browser.safebrowsing.enabled");
>      this.malwareEnabled = Services.prefs.getBoolPref("browser.safebrowsing.malware.enabled");
> +    this.trackingEnabled = Services.prefs.getBoolPref("privacy.trackingprotection.enabled") ||

The variable names "phishingEnabled" and "malwareEnabled" are misleading and scary. :) I know you didn't choose them, so changing them is not your responsibility.
Depends on: 1239179
Comment on attachment 8706388 [details] [diff] [review]
Patch 1: Block SWFs on the current Tracking Protection Shavar list.

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

::: dom/base/nsObjectLoadingContent.cpp
@@ +3275,5 @@
> +          nsAutoCString uri;
> +          mURI->GetSpec(uri);
> +          LOG(("OBJLC [%p]: Blocking \"%s\" for stability", this, uri.get()));
> +          aReason = eFallbackBlocklisted;
> +          return false;

We should add a telemetry probe to measure. Maybe:

* how many plugin instances we block
* how many plugin instances <= 5x5 pixels we don't block
* how many plugin instances >  5x5 pixels (assuming we never block > 5x5)
Comment on attachment 8706388 [details] [diff] [review]
Patch 1: Block SWFs on the current Tracking Protection Shavar list.

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

::: dom/base/nsObjectLoadingContent.cpp
@@ +3273,5 @@
> +        rv = classifier->ClassifyLocalWithTables(mURI, tables, results);
> +        if (NS_SUCCEEDED(rv) && !results.IsEmpty()) {
> +          nsAutoCString uri;
> +          mURI->GetSpec(uri);
> +          LOG(("OBJLC [%p]: Blocking \"%s\" for stability", this, uri.get()));

LOG() just goes to stdout, right? We should log a web console message so web developers know why their SWFs stopped working.

We should probably just log "Blocking \"%s\"" without saying "for stability". We don't want people to think Firefox is what is unstable. :)
Francois, here is Tobias' WIP patch for SWF blocking.
Addressing review comments.
Attachment #8706388 - Attachment is obsolete: true
Attached patch Part 2: Telemetry probes. (obsolete) — Splinter Review
Adding telemetry probes as suggested.
Comment on attachment 8708728 [details] [diff] [review]
Part 1 (v2): Block SWFs on the current Tracking Protection Shavar list.

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

::: dom/base/nsObjectLoadingContent.cpp
@@ +3241,4 @@
>  static bool sPrefsInitialized;
>  static uint32_t sSessionTimeoutMinutes;
>  static uint32_t sPersistentTimeoutDays;
> +static nsAutoCString sBlockTable;

I think nsAutoCStrings are recommended only for use on the stack. (That's why they're called "auto".) They contain inline storage that will be wasted space when storing a large string. Maybe just make sBlockTable a private member variable of nsObjectLoadingContent?

https://developer.mozilla.org/en-US/docs/nsAutoString
Comment on attachment 8708729 [details] [diff] [review]
Part 2: Telemetry probes.

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

::: browser/modules/PluginContent.jsm
@@ +427,5 @@
> +        let pluginRect = plugin.getBoundingClientRect();
> +        if (pluginRect.width <= 5 && pluginRect.height <= 5) {
> +          Services.telemetry.getKeyedHistogramById('PLUGIN_TINY_CONTENT').add(key);
> +        } else {
> +          Services.telemetry.getKeyedHistogramById('PLUGIN_NON_TINY_CONTENT').add(key);

Maybe we don't need PLUGIN_NON_TINY_CONTENT after all, if it is just PLUGIN_ACTIVATION_COUNT - PLUGIN_TINY_CONTENT?

::: toolkit/components/telemetry/Histograms.json
@@ +10040,5 @@
>      "description": "Plugin drawing model. 0 when windowed, otherwise NPDrawingModel + 1."
> +  },
> +  "PLUGIN_BLOCKED_FOR_STABILITY": {
> +    "alert_emails": ["tschneider@mozilla.com"],
> +    "expires_in_version": "never",

The telemetry module owners will probably ask for some expiration version. If we're successful, we won't need plugin activation telemetry forever. :)
Depends on: 1240926
Update to use the Safe Browsing URL classifier in the chrome process.
Attachment #8708728 - Attachment is obsolete: true
Attachment #8708729 - Attachment is obsolete: true
Comment on attachment 8711994 [details] [diff] [review]
Part 1 (v3): Block SWFs using Safe Browsing URL classifier.

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

::: toolkit/components/telemetry/Histograms.json
@@ +10013,5 @@
>      "n_values": 12,
>      "description": "Plugin drawing model. 0 when windowed, otherwise NPDrawingModel + 1."
> +  },
> +  "PLUGIN_BLOCKED_FOR_STABILITY": {
> +    "expires_in_version": "48",

FF 48 is only six weeks away. We expect this plugin telemetry to be interesting for a while, so how about version 52? It will hit the release channel in December 2016.

I think you also need to specify "alert_emails" and maybe "bug_numbers". Please include me in the alert_emails list. :)

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ +87,4 @@
>  #define TRACKING_TABLE_PREF     "urlclassifier.trackingTable"
>  #define TRACKING_WHITELIST_TABLE_PREF "urlclassifier.trackingWhitelistTable"
>  #define FORBIDDEN_TABLE_PREF      "urlclassifier.forbiddenTable"
> +#define BLOCKED_TABLE_PREF      "urlclassifier.blockedTable"

Align quoted string with its neighbors?
Addressing review comments.
Attachment #8711994 - Attachment is obsolete: true
Comment on attachment 8712248 [details] [diff] [review]
Part 1 (v4): Block SWFs using Safe Browsing URL classifier.

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

::: modules/libpref/init/all.js
@@ +5172,4 @@
>  // Turn rewriting of youtube embeds on/off
>  pref("plugins.rewrite_youtube_embeds", true);
>  
> +pref("plugins.block_for_stability.enabled", true);

We're adding a generic Safe Browsing blocking mechanism, so I would suggest a more generic name for this: browser.safebrowsing.blockedURIs.enabled

The purpose of the blocking can be reflected in the name of the list that's downloaded from the server.

@@ +5172,5 @@
>  // Turn rewriting of youtube embeds on/off
>  pref("plugins.rewrite_youtube_embeds", true);
>  
> +pref("plugins.block_for_stability.enabled", true);
> +pref("urlclassifier.blockedTable", "test-block-simple");

This is fine, just noting that the value will change to "test-block-simple,pluginstability-block-digest256" once the list is served by the shavar server.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ +75,4 @@
>  #define CHECK_FORBIDDEN_PREF    "browser.safebrowsing.forbiddenURIs.enabled"
>  #define CHECK_FORBIDDEN_DEFAULT false
>  
> +#define CHECK_BLOCKED_PREF    "plugins.block_for_stability.enabled"

See comments in all.js
Changing pref names.
Attachment #8712248 - Attachment is obsolete: true
Comment on attachment 8714533 [details] [diff] [review]
Part 1 (v5): Block SWFs using Safe Browsing URL classifier.

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

I just thought of something: what happens if we navigate (i.e. typing into the URL bar) to a blocked URI? Is there an error message or does the UI fall over somehow?

::: dom/base/nsObjectLoadingContent.cpp
@@ +1158,5 @@
> +    mURI->GetSpec(uri);
> +    nsCOMPtr<nsIConsoleService> console(
> +      do_GetService("@mozilla.org/consoleservice;1"));
> +    if (console) {
> +      nsString message = NS_LITERAL_STRING("Blocking \"") +

I think we should be more explicit as to the reason why this is being blocked so that developers are not confused and looking at things like mixed content, CSP, etc. Maybe something along the lines of "Blocking example.com since it was found on an internal Firefox blocklist."

Also, should this be a translatable string?

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ +75,5 @@
>  #define CHECK_FORBIDDEN_PREF    "browser.safebrowsing.forbiddenURIs.enabled"
>  #define CHECK_FORBIDDEN_DEFAULT false
>  
> +#define CHECK_BLOCKED_PREF    "browser.safebrowsing.blockedURIs.enabled"
> +#define CHECK_BLOCKED_DEFAULT false

As far as I can see in all.js, this is enabled by default.
Updated table list.
Attachment #8714533 - Attachment is obsolete: true
Blocks: 1248813
User Story: (updated)
User Story: (updated)
User Story: (updated)
Summary: Block SWFs on the content blocking list hosted on the Shavar service → [experiment] Block SWFs on the content blocking list hosted on the Shavar service 0.6.9
Whiteboard: experiment
Changed blocking message and disabled blocking by default.
Attachment #8723155 - Attachment is obsolete: true
Attachment #8730375 - Flags: review?(francois)
Comment on attachment 8730375 [details] [diff] [review]
Part 1 (v7): Block SWFs using Safe Browsing URL classifier.

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

One important thing (which I may have forgotten to mention, sorry) is missing from this patch. All of the new lists (test-blocked-simple and plugin-block-digest256 and plugin2-block-digest256) must be added to this pref:

urlclassifier.disallow_completions
https://dxr.mozilla.org/mozilla-central/rev/f0c0480732d36153e8839c7f17394d45f679f87d/modules/libpref/init/all.js#4930

Also, the two lists served by shavar (plugin-block-digest256 and plugin2-block-digest256) need to be added to this pref:

browser.safebrowsing.provider.mozilla.lists
https://dxr.mozilla.org/mozilla-central/rev/f0c0480732d36153e8839c7f17394d45f679f87d/modules/libpref/init/all.js#4941

The rest of the patch all looks good to me.

::: browser/modules/PluginContent.jsm
@@ -422,5 @@
>          break;
>  
>        case "PluginInstantiated":
> -        Services.telemetry.getKeyedHistogramById('PLUGIN_ACTIVATION_COUNT').add(this._getPluginInfo(plugin).pluginTag.niceName);
> -        shouldShowNotification = true;

Did you mean to remove that "shouldShowNotification"?

::: toolkit/components/url-classifier/SafeBrowsing.jsm
@@ +167,4 @@
>        Services.prefs.getBoolPref("privacy.trackingprotection.pbmode.enabled") ||
>        Services.prefs.getBoolPref("plugins.block_for_stability.enabled");
>      this.forbiddenEnabled = Services.prefs.getBoolPref("browser.safebrowsing.forbiddenURIs.enabled");
> +    this.blockedEnabled = Services.prefs.getBoolPref("plugins.block_for_stability.enabled");

That pref has been renamed to `browser.safebrowsing.blockedURIs.enabled`.
Attachment #8730375 - Flags: review?(francois) → review-
Addressed final review comments.
Attachment #8730375 - Attachment is obsolete: true
Attachment #8730498 - Flags: review?(francois)
Comment on attachment 8730498 [details] [diff] [review]
Part 1 (v8): Block SWFs using Safe Browsing URL classifier.

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

I was going to r+ this patch with the one comment, but then I ran my getlists.py script to double-check the list names on the server:

  $ ./get-lists.py 
  mozfull-track-digest256
  mozfullstaging-track-digest256
  mozplugin-block-digest256
  mozplugin2-block-digest256
  mozpub-track-digest256
  mozstd-track-digest256
  mozstd-trackwhite-digest256
  mozstdstaging-track-digest256
  mozstdstaging-trackwhite-digest256

and realized that they are not exactly the ones we asked for. Obviously, I should have caught this earlier, apologies.

I think the best way forward (given how long it would take to change the server config) would be to s/plugin-block/mozplugin-block/g and s/plugin2-block/mozplugin2-block/g everywhere on your patch and just use the names as they are on the server.

::: modules/libpref/init/all.js
@@ +4909,4 @@
>  pref("urlclassifier.phishTable", "goog-phish-shavar,test-phish-simple");
>  pref("urlclassifier.downloadBlockTable", "");
>  pref("urlclassifier.downloadAllowTable", "");
> +pref("urlclassifier.disallow_completions", "test-malware-simple,test-phish-simple,test-unwanted-simple,test-track-simple,test-trackwhite-simple,test-forbid-simple,goog-downloadwhite-digest256,mozstd-track-digest256,mozstd-trackwhite-digest256,mozfull-track-digest256,plugin-block-digest256,plugin2-block-digest256");

That should also have `test-block-simple`.
Attachment #8730498 - Flags: review?(francois) → review-
Updated table names.
Attachment #8730498 - Attachment is obsolete: true
Comment on attachment 8730775 [details] [diff] [review]
Part 1 (v9): Block SWFs using Safe Browsing URL classifier.

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

r+ but you need to remove "mozplugin2-block-digest256" from the "urlclassifier.blockedTable" pref.

::: modules/libpref/init/all.js
@@ +5200,5 @@
>  // Turn rewriting of youtube embeds on/off
>  pref("plugins.rewrite_youtube_embeds", true);
>  
> +pref("browser.safebrowsing.blockedURIs.enabled", false);
> +pref("urlclassifier.blockedTable", "test-block-simple,mozplugin-block-digest256,mozplugin2-block-digest256");

Here we should not have "mozplugin2-block-digest256" otherwise we'll have both mozplugin and mozplugin2 active at the same time.

I believe the intention is to instead give mozplugin2 to only a few users to A/B test, right?

If that's correct, we'll have this for everyone:

pref("urlclassifier.blockedTable", "test-block-simple,mozplugin-block-digest256");

and then this one for the people who get the "B" list:

pref("urlclassifier.blockedTable", "test-block-simple,mozplugin2-block-digest256");
Attachment #8730775 - Flags: review+
Yes, you are right, will update the patch. Thanks!
Final patch.
Attachment #8730775 - Attachment is obsolete: true
(In reply to François Marier [:francois] from comment #21)
>   mozfull-track-digest256
>   mozfullstaging-track-digest256
>   mozplugin-block-digest256
>   mozplugin2-block-digest256
>   mozpub-track-digest256
>   mozstd-track-digest256
>   mozstd-trackwhite-digest256
>   mozstdstaging-track-digest256
>   mozstdstaging-trackwhite-digest256
> 
> and realized that they are not exactly the ones we asked for. Obviously, I
> should have caught this earlier, apologies.

François, what was the list name you expected? I think the "mozplugin2?-block-digest256" list name is what we asked for. It's the list name I referenced in our blocklist repo on GitHub:

https://github.com/mozilla-services/shavar-plugin-blocklist/blob/master/README.md
Flags: needinfo?(francois)
(In reply to Chris Peterson [:cpeterson] from comment #26)
> François, what was the list name you expected? I think the
> "mozplugin2?-block-digest256" list name is what we asked for. It's the list
> name I referenced in our blocklist repo on GitHub:
> 
> https://github.com/mozilla-services/shavar-plugin-blocklist/blob/master/
> README.md

I thought we were going for plugin-block-digest256 and plugin2-block-digest256 and so I was surprised to see the "moz" in there, but maybe I was wrong and everything is as it should be :)
Flags: needinfo?(francois)
Test code will also land disabled on Nightly and Aurora.
Group: mozilla-employee-confidential
User Story: (updated)
Disabled logging, only use simple block lists in test.
Attachment #8730914 - Attachment is obsolete: true
Hopefully uploading the right patch this time.
Attachment #8731937 - Attachment is obsolete: true
Comment on attachment 8731938 [details] [diff] [review]
Part 1 (v12): Block SWFs using Safe Browsing URL classifier.

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

::: dom/base/nsObjectLoadingContent.cpp
@@ +1153,5 @@
>      if (thisNode && thisNode->IsInComposedDoc()) {
>        thisNode->GetComposedDoc()->AddBlockedTrackingNode(thisNode);
>      }
> +  } else if (aStatusCode == NS_ERROR_BLOCKED_URI) {
> +    // Logging is temporarily disabled until after experiment phase.

Let's file a bug for this to make sure we don't forget to re-enable or backout the patch.

Developers will be confused if there's no indication whatsoever of why we blocked some of their resources.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ +75,5 @@
>  #define CHECK_FORBIDDEN_PREF    "browser.safebrowsing.forbiddenURIs.enabled"
>  #define CHECK_FORBIDDEN_DEFAULT false
>  
> +#define CHECK_BLOCKED_PREF    "browser.safebrowsing.blockedURIs.enabled"
> +#define CHECK_BLOCKED_DEFAULT false

Not a big deal, but maybe this should be true to match the default value in the other file?
Attachment #8731938 - Flags: review+
Feature should be disabled by default.
Attachment #8731938 - Attachment is obsolete: true
Francois: I changed browser/components/safebrowsing/content/test/head.js and toolkit/components/url-classifier/tests/UrlClassifierTestUtils.jsm to not use remote lists but still getting errors during testing due to non-local requests being made. See https://treeherder.mozilla.org/#/jobs?repo=try&revision=c07e9b7196d1&selectedJob=18303369.
Attachment #8731941 - Attachment is obsolete: true
Attachment #8732893 - Flags: review?(francois)
Comment on attachment 8732893 [details] [diff] [review]
Part 1 (v14): Block SWFs using Safe Browsing URL classifier.

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

I've been looking into this for a couple of hours and I'm not entirely sure why the reftests are doing network requests given that your stuff is pref'ed off by default.

I have however found a few places where you'd probably want to explicitly set browser.safebrowsing.blockedURIs.enabled = false:

https://dxr.mozilla.org/mozilla-central/rev/6202ade0e6d688ffb67932398e56cfc6fa04ceb3/layout/tools/reftest/reftest-preferences.js#83
https://dxr.mozilla.org/mozilla-central/rev/6202ade0e6d688ffb67932398e56cfc6fa04ceb3/js/src/tests/user.js#26-27
https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette_driver/geckoinstance.py#205
https://hg.mozilla.org/mozilla-central/file/6606ab35633b/testing/talos/talos/config.py#l110

That first one looks somewhat promising, though it shouldn't really matter given that we don't turn this thing on anywhere...

Also, I noticed that the patch is missing the -block- lists in these basic url classifier tests:

https://dxr.mozilla.org/mozilla-central/rev/6202ade0e6d688ffb67932398e56cfc6fa04ceb3/toolkit/components/url-classifier/tests/unit/head_urlclassifier.js
https://dxr.mozilla.org/mozilla-central/rev/6202ade0e6d688ffb67932398e56cfc6fa04ceb3/toolkit/components/url-classifier/tests/unit/test_dbservice.js
https://dxr.mozilla.org/mozilla-central/rev/6202ade0e6d688ffb67932398e56cfc6fa04ceb3/toolkit/components/url-classifier/tests/unit/test_streamupdater.js

It should just be a matter of doing the same thing as for the -forbid- lists.

::: browser/components/safebrowsing/content/test/head.js
@@ +57,2 @@
>  Services.prefs.setBoolPref("browser.safebrowsing.forbiddenURIs.enabled", true);
> +Services.prefs.setBoolPref("browser.safebrowsing.blockedURIs.enabled", true);

I don't think we need to turn it on here because we're not actually testing that feature in the tests that include this file.

::: toolkit/components/url-classifier/tests/UrlClassifierTestUtils.jsm
@@ +8,4 @@
>  const TRACKING_TABLE_PREF = "urlclassifier.trackingTable";
>  const WHITELIST_TABLE_NAME = "mochitest-trackwhite-simple";
>  const WHITELIST_TABLE_PREF = "urlclassifier.trackingWhitelistTable";
> +const BLOCKED_TABLE_NAME = "mochitest-block-simple";

Actually, I don't think we should be modifying this file because there aren't any non-TP Safe Browsing lists. It appears to be only used by the tracking protection tests.
Attachment #8732893 - Flags: review?(francois) → review-
Attachment #8734957 - Flags: review?(francois)
Comment on attachment 8734957 [details] [diff] [review]
Part 1 (v15): Block SWFs using Safe Browsing URL classifier.

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

Thanks for adding the tests!

::: modules/libpref/init/all.js
@@ +5257,5 @@
>  
>  // Disable browser frames by default
>  pref("dom.mozBrowserFramesEnabled", false);
> +
> +pref("browser.safebrowsing.blockedURIs.enabled", false);

nit: if would be great if you could move these next to the other Safe Browsing prefs in this file.

::: toolkit/components/url-classifier/tests/unit/test_dbservice.js
@@ +91,5 @@
>    phishUnexpected[chunk6Urls[i]] = true;
>  }
> +for (var i = 0; i < chunk7Urls.length; i++) {
> +  blockedExpected[chunk7Urls[i]] = true;
> +  // chunk4 urls are expired

That should be "chunk7".

Also, there's a comment just above that block that needs to be updated to:

  // we are going to add chunks 1, 2, 4, 5, and 6 to phish-simple,
  // chunk 2 to malware-simple, chunk 3 to unwanted-simple,
  // chunk 4 to forbid-simple, and chunk 7 to block-simple.
  // Then we'll remove the urls in chunk3 from phish-simple, then
  // expire chunk 1 and chunks 4-7 from phish-simple.
Attachment #8734957 - Flags: review?(francois) → review+
Address nits.
Attachment #8734957 - Attachment is obsolete: true
Tobias, your patch does not apply cleanly to mozilla-inbound. Can you please attach a rebased patch?

…
Hunk #1 FAILED at 24
1 out of 1 hunks FAILED -- saving rejects to file js/src/tests/user.js.rej
patching file layout/tools/reftest/reftest-preferences.js
Hunk #1 FAILED at 80
1 out of 1 hunks FAILED -- saving rejects to file layout/tools/reftest/reftest-preferences.js.rej
patching file modules/libpref/init/all.js
Hunk #1 succeeded at 4944 (offset 2 lines).
Hunk #2 succeeded at 4955 (offset 2 lines).
patching file testing/marionette/client/marionette_driver/geckoinstance.py
Hunk #1 succeeded at 206 with fuzz 2 (offset 1 lines).
patching file testing/talos/talos/config.py
Hunk #1 FAILED at 108
1 out of 1 hunks FAILED -- saving rejects to file testing/talos/talos/config.py.rej
patching file toolkit/components/telemetry/Histograms.json
Hunk #1 succeeded at 10442 with fuzz 2 (offset 56 lines).
…
Flags: needinfo?(tschneider)
Rebased patch.
Attachment #8735593 - Attachment is obsolete: true
Flags: needinfo?(tschneider)
After talking to Tobias on IRC, I changed the "browser.safebrowsing.blockedURIs.enabled" pref's default value back to false before I landed the patch on mozilla-inbound.
https://hg.mozilla.org/mozilla-central/rev/82a22f34e0e8
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1253404
No longer depends on: 1240926
Comment on attachment 8735634 [details] [diff] [review]
Part 1 (v17): Block SWFs using Safe Browsing URL classifier.

Approval Request Comment
[Feature/regressing bug #]: None
[User impact if declined]: We won't be able to run our three-week beta experiment in 47 that was already postponed from 46.
[Describe test coverage new/current, TreeHerder]: This feature has landed in Nightly 48, but is disabled by default. Tobias and I have confirmed that it works when manually enabled.
[Risks and why]: Extremely small chance of some sites breaking, but the experiment will only affect 6.6% of Beta 47 users for three weeks.
[String/UUID change made/needed]: None
Attachment #8735634 - Flags: approval-mozilla-aurora?
Comment on attachment 8735634 [details] [diff] [review]
Part 1 (v17): Block SWFs using Safe Browsing URL classifier.

This is needed for block-plugin experiment, that is to be kicked-off with 47.0b1, Aurora47+
Attachment #8735634 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Tobias, the changeset I landed on mozilla-inbound (82a22f34e0e8) does not apply cleanly to mozilla-aurora. Can you please rebase changelist 82a22f34e0e8 on mozilla-aurora?

Note that the change I landed is not identical to the patch attached to this bug! I changed the "browser.safebrowsing.blockedURIs.enabled" pref's default value back to false (noted in comment 41).
Flags: needinfo?(tschneider)
merging xpcom/base/ErrorList.h
warning: conflicts while merging js/src/tests/user.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging layout/tools/reftest/reftest-preferences.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging testing/talos/talos/config.py! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)
Tomcats-MacBook-Pro-2:mozilla-central Tomcat$

is the conflict btw
Rebased to aurora.
Attachment #8735634 - Attachment is obsolete: true
Flags: needinfo?(tschneider)
Depends on: 1262767
Depends on: 1266889
Depends on: 1268716
Blocks: 1277876
Depends on: 1268120
Depends on: 1354778
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.