To allow for quick manual testing of Safe Browsing, the test lists are populated with URLs that can be visited to easily check behavior (see [1]). The same functionality should be added for list based Flash blocking. This requires populating the test lists with these URLs and adding HTML and Flash assets to https://github.com/mozilla/itisatrap [1] http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/toolkit/components/url-classifier/SafeBrowsing.jsm#330-395

PR submitted for the HTML and Flash assets: https://github.com/mozilla/itisatrap/pull/22

Comment on attachment 8830993 [details] Bug 1333483 - Hardcode itisatrap manual testing URLs for Flash blocking into the SafeBrowsing module https://reviewboard.mozilla.org/r/107650/#review108794 ::: toolkit/components/url-classifier/SafeBrowsing.jsm:381 (Diff revision 1) > > + const flashDenyURL = "flashblock.itisatrap.org/"; > + const flashDenyExceptURL = "except.flashblock.itistrap.org/"; > + const flashAllowURL = "flashallow.itistrap.org/"; > + const flashAllowExceptURL = "except.flashallow.itistrap.org/"; > + const flashThirdPartyURL = "flashthirdparty.itistrap.org"; Missing trailing slash. ::: toolkit/components/url-classifier/SafeBrowsing.jsm:382 (Diff revision 1) > + const flashDenyURL = "flashblock.itisatrap.org/"; > + const flashDenyExceptURL = "except.flashblock.itistrap.org/"; > + const flashAllowURL = "flashallow.itistrap.org/"; > + const flashAllowExceptURL = "except.flashallow.itistrap.org/"; > + const flashThirdPartyURL = "flashthirdparty.itistrap.org"; > + const flashThirdPartyExceptURL = "except.flashthirdparty.itistrap.org"; ditto

Comment on attachment 8830993 [details] Bug 1333483 - Hardcode itisatrap manual testing URLs for Flash blocking into the SafeBrowsing module https://reviewboard.mozilla.org/r/107650/#review108794 > Missing trailing slash. Good catch

Comment on attachment 8830993 [details] Bug 1333483 - Hardcode itisatrap manual testing URLs for Flash blocking into the SafeBrowsing module https://reviewboard.mozilla.org/r/107650/#review108800 ::: toolkit/components/url-classifier/SafeBrowsing.jsm:382 (Diff revision 2) > + const flashDenyURL = "flashblock.itisatrap.org/"; > + const flashDenyExceptURL = "except.flashblock.itistrap.org/"; > + const flashAllowURL = "flashallow.itistrap.org/"; > + const flashAllowExceptURL = "except.flashallow.itistrap.org/"; > + const flashThirdPartyURL = "flashthirdparty.itistrap.org/"; > + const flashThirdPartyExceptURL = "except.flashthirdparty.itistrap.org/"; `itisatrap.org` (missing A everywhere except the first one)

Comment on attachment 8830993 [details] Bug 1333483 - Hardcode itisatrap manual testing URLs for Flash blocking into the SafeBrowsing module https://reviewboard.mozilla.org/r/107650/#review109002

@francois I had two questions related to this that I think you might be able to answer. First, I believe that there is no problem merging this before Bug 1307604 (which adds the tables referenced in this bug). Is that correct? Second, after |addMozEntries()| is called, are the entries added tables added regardless of whether the tables were added in |readPrefs()| and |controlUpdateChecking()|? Really what I want to know is this: If |SafeBrowsing.flashBlockEnabled = false| (so |listManager.enableUpdate| was not called and |listManager.disableUpdate| may have been called on the lists), will the test URLs added in |addMozEntries()| match when |Classify()| is called? What about if |SafeBrowsing.flashBlockEnabled| is then set to |true| and |listManager.enableUpdate| is called (but |addMozEntries()| is not called again). Will |Classifiy()| return those URLs then?

Comment on attachment 8830993 [details] Bug 1333483 - Hardcode itisatrap manual testing URLs for Flash blocking into the SafeBrowsing module https://reviewboard.mozilla.org/r/107650/#review110484 ::: toolkit/components/url-classifier/SafeBrowsing.jsm:381 (Diff revisions 3 - 4) > > const flashDenyURL = "flashblock.itisatrap.org/"; > const flashDenyExceptURL = "except.flashblock.itisatrap.org/"; > const flashAllowURL = "flashallow.itisatrap.org/"; > const flashAllowExceptURL = "except.flashallow.itisatrap.org/"; > - const flashThirdPartyURL = "flashthirdparty.itisatrap.org/"; > + const flashSubDocURL = "flashsubdocument.itisatrap.org/"; You're using `subdoc` everywhere else. Maybe the test domains should use that too?

(In reply to Kirk Steuber [:bytesized] from comment #9) > Second, after |addMozEntries()| is called, are the entries added tables > added regardless of whether the tables were added in |readPrefs()| and > |controlUpdateChecking()|? As far as I know, the test entries will be added regardless of whether or not the feature is enabled. The list entries of course won't have any effect. These test tables are not associated with an update URL so they won't be scheduled to be updated again.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/c3360defc879 Hardcode itisatrap manual testing URLs for Flash blocking into the SafeBrowsing module r=francois

Comment on attachment 8830993 [details] Bug 1333483 - Hardcode itisatrap manual testing URLs for Flash blocking into the SafeBrowsing module Approval Request Comment [Feature/Bug causing the regression]: This feature is needed for the Shield study to be run on Release 52 (bug 1335232), which we'll use to study the effect of making flash click-to-play by default. [User impact if declined]: Shield study will need to restart the browser in order to work correctly [Is this code covered by automated tests?]: Yes [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: Not for the feature independently. We'll do QE on the study as a whole to make sure all pieces work as expected [List of other uplifts needed for the feature/fix]: bug 1307604 [Is the change risky?]: No [Why is the change risky/not risky?]: Adds single pref to the list of prefs SafeBrowsing tracks. [String changes made/needed]: none

Oops! Pasted in the wrong thing for one question: [User impact if declined]: Shield study will lack easy manual testing that this patch provides.

Comment on attachment 8830993 [details] Bug 1333483 - Hardcode itisatrap manual testing URLs for Flash blocking into the SafeBrowsing module For shield study purpose. Aurora53+.

Comment on attachment 8830993 [details] Bug 1333483 - Hardcode itisatrap manual testing URLs for Flash blocking into the SafeBrowsing module add URLs for flash blocking manual testing, beta52+

Would have been nice if the UI test permafail this patch caused was called out in the Beta uplift (bug 1333303) since we clearly knew about it by the time approvals were requested.

Looks like the fix from bug 1333303 needs more rebasing than I'm willing to attempt and anybody with the knowledge for doing so is already long-gone for the weekend. Backed out. https://hg.mozilla.org/releases/mozilla-beta/rev/f29a51aaae32824ec1202953991cdb098f25b056

Once Bug 1333303 has been uplifted, this should be uplifted as well please.