Currently we ship a copy of the blocklist with the application however we only use it for new profiles where there is no existing blocklist.xml. This is fine for new users and regular users who upgrade however for users who last used Firefox some time ago or if the blocklist update system is broken in some way it means we can end up using a much older blocklist than we have available. I want to make it so the shipped blocklist includes a last updated timestamp in it, on the startup of a new version of the app we'll compare that timestamp to the modification time of the profile blocklist and if the shipped version is newer copy it to the profile.

This is something that's been lying in my patch queue for a while, I'll not have time to finish it but might be useful.

This bug in effect now blocks a Mac topcrasher (bug 951906), which makes it much more urgent. See bug 988490 comment #29. Bug 951906's crashes are mostly on startup, and are caused by an extension. The extension is now blocked (see bug 988490), but the block isn't yet in the built-in block list. It will need to get there before most of the victims of bug 951906 will see any relief. But it won't have any effect until this bug is fixed.

Unbitrotted and added tests. This compares the timestamp stored in the XML in the app-shipped blocklist against the file modification time of the profile blocklist. If the app-shipped version is newer we just copy it to the profile. The blocklist service now only loads from the profile copy. Running this through try now but it passes locally. The test is a little awkward as it has to modify the app dir but I think it does so safely.

Comment on attachment 8404241 [details] [diff] [review] patch rev 1 *sigh windows*

It's better to close the file after reading it

Comment on attachment 8404384 [details] [diff] [review] patch rev 2 Review of attachment 8404384 [details] [diff] [review]: ----------------------------------------------------------------- You caught me during conferences/travel :\ ::: toolkit/mozapps/extensions/AddonManager.jsm @@ +527,5 @@ > this.TelemetryTimestamps.add(name, value); > }, > > + validateBlocklist: function AMI_validateBlocklist() { > + Components.utils.import("resource://gre/modules/FileUtils.jsm"); defineLazyModuleGetter, like the rest of 'em. @@ +555,5 @@ > + try { > + fileStream.init(appBlocklist, FileUtils.MODE_RDONLY, FileUtils.PERMS_FILE, 0); > + let parser = Cc["@mozilla.org/xmlextras/domparser;1"]. > + createInstance(Ci.nsIDOMParser); > + var doc = parser.parseFromStream(fileStream, "UTF-8", I'm pretty sure Henri will hire a hitman if I let parseFromStream() into the tree again... can you convert this to use parser.parseFromString() ? ::: toolkit/mozapps/extensions/nsBlocklistService.js @@ -634,5 @@ > - return; > - } > - var appFile = FileUtils.getFile(KEY_APPDIR, [FILE_BLOCKLIST]); > - if (appFile.exists()) { > - this._loadBlocklistFromFile(appFile); Hm, this had the benefit that if for some reason the blocklist file was deleted in the profile dir, it would at least still load a slightly outdated copy. Now that isn't the case. I think that's a trait that would be beneficial to keep. ::: toolkit/mozapps/extensions/test/xpcshell/test_overrideblocklist.js @@ +64,5 @@ > + createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "1"); > + > + let appBlocklist = FileUtils.getFile(KEY_APPDIR, [FILE_BLOCKLIST]); > + if (appBlocklist.exists()) { > + appBlocklist.moveTo(gAppDir, "blocklist.old"); Heh... I can imagine the odd person grumbling about this, due to a read-only app-dir (since we support running tests on downloaded builds/builds without --enable-tests). So you better detect that here and bail early without it being a test failure.

FYI, beta 8 is going to build today (Monday).

Comment on attachment 8404384 [details] [diff] [review] patch rev 2 Review of attachment 8404384 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/extensions/AddonManager.jsm @@ +592,5 @@ > + return; > + > + // Otherwise copy the application shipped blocklist to the profile > + try { > + appBlocklist.copyTo(profileBlocklist.parent, FILE_BLOCKLIST); Don't we need to trigger an update of the add-on DB (to reflect new @@ +637,5 @@ > Services.prefs.setCharPref(PREF_EM_LAST_PLATFORM_VERSION, > Services.appinfo.platformVersion); > Services.prefs.setIntPref(PREF_BLOCKLIST_PINGCOUNTVERSION, > (appChanged === undefined ? 0 : -1)); > + this.validateBlocklist(); I'd rather keep all the logic about this inside the blocklist service. Maybe just tell the blocklist service that the app was upgraded, and have the blocklist service do the check the first time it opens the blocklist after an upgrade? I'm not sure if we'll ever be able to make the first blocklist load async, but if so it will be easier if we keep this together with the rest of the blocklist loading code.

Not touching the blocklist service makes for a safer patch

(In reply to :Irving Reid from comment #8) > Comment on attachment 8404384 [details] [diff] [review] > patch rev 2 > > Review of attachment 8404384 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/mozapps/extensions/AddonManager.jsm > @@ +592,5 @@ > > + return; > > + > > + // Otherwise copy the application shipped blocklist to the profile > > + try { > > + appBlocklist.copyTo(profileBlocklist.parent, FILE_BLOCKLIST); > > Don't we need to trigger an update of the add-on DB (to reflect new That already happens when the app version changes (otherwise the tests would fail).

Comment on attachment 8406210 [details] [diff] [review] patch rev 3 Review of attachment 8406210 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/extensions/AddonManager.jsm @@ +560,5 @@ > + > + let data = ""; > + let str = {}; > + let read = 0; > + do { Nit: Trailing whitespace. THE HORROR

Comment on attachment 8406210 [details] [diff] [review] patch rev 3 [Approval Request Comment] Bug caused by (feature/regressing bug #): Pre-existing condition User impact if declined: We can't block startup crashers using the blocklist Testing completed (on m-c, etc.): Landed in m-c two days ago Risk to taking this patch (and alternatives if risky): Should be low risk, all important operations are surrounded with try...catch and the worst case of the new function failing leaves us in the same state we're in now. String or IDL/UUID changes made by this patch: None

Comment on attachment 8406210 [details] [diff] [review] patch rev 3 I cannot take this feature for 29. Sorry but it is too late in the beta cycle (beta 9 gtb is today). We need more testing on this.