Closed Bug 430120 Opened 16 years ago Closed 16 years ago

Update blocklist URL to include same info as update URL

Categories

(Toolkit :: Add-ons Manager, defect)

x86
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9

People

(Reporter: alex, Assigned: mossop)

References

Details

(Keywords: verified1.8.1.15)

Attachments

(2 files, 2 obsolete files)

Currently the extensions.blocklist.url is 

  https://addons.mozilla.org/blocklist/1/%APP_ID%/%APP_VERSION%/

and the app.update.url is

  https://aus2.mozilla.org/update/3/%PRODUCT%/%VERSION%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/update.xml

We need extensions.blocklist.url to contain all of the same information, which means we need to add the following to it:

 %PRODUCT%
 %VERSION%
 %BUILD_ID%
 %BUILD_TARGET%
 %LOCALE%
 %CHANNEL%
 %OS_VERSION%
 %DISTRIBUTION%
 %DISTRIBUTION_VERSION%

This will require updates to the Firefox 3 pref, and modifications to the web service that supports the blocklist URL.
we've been wanting to move to this ping instead of the AUS ping for a while now to be able to keep track of what our usage looks like, to watch out for weird anomalies, etc -- now that the earlier bug has been fixed & this looks like it's giving us reliable feedback, would like to move to blocklist pings across the board, but need this other locale, channel, OS, etc info. would be very very very good to get this into fx3. (and, actually, into any updates of fx2 we do.)
I can work on the necessary changes to the prefs and blocklist service.
Assignee: nobody → dtownsend
Should this be blocking 1.9 or fx3?
BTW - I am all for this.  I think we have other uses for this information aside from just metrics.  For example -- business logic for specific platforms, locales, etc. that extends past the original scope of the blocklist XML.
Flags: blocking-firefox3+
Attached patch patch rev 1 (obsolete) — Splinter Review
This is an initial patch that allows the blocklist service to replace the same parameters that the update service does. It includes a unit test to verify all the parameters are correctly replaced.

This patch can be safely checked in now and then once the blocklist service is updated a simple pref change will let us switch to the new url format.
Attachment #316986 - Flags: review?(robert.bugzilla)
Does it matter that these are both 404s:
 const XMLURI_BLOCKLIST                = "http://www.mozilla.org/2006/addons-blocklist";
 const XMLURI_PARSE_ERROR              = "http://www.mozilla.org/newlayout/xml/parsererror.xml"
(In reply to comment #6)
> Does it matter that these are both 404s:
>  const XMLURI_BLOCKLIST                =
> "http://www.mozilla.org/2006/addons-blocklist";
>  const XMLURI_PARSE_ERROR              =
> "http://www.mozilla.org/newlayout/xml/parsererror.xml"

No these are just namespaces used in the xml, they don't have to exist anywhere. Constants should perhaps be XMLNS_... to signify then.
Dave, are these supposed to have var= added to them (only see &):
+  gPrefs.setCharPref(PREF_BLOCKLIST_URL, "http://localhost:4444/2?" + 
+                     "%APP_ID%&%APP_VERSION%&%PRODUCT%&%VERSION%&%BUILD_ID%&" +
+                     "%BUILD_TARGET%&%LOCALE%&%CHANNEL%&" + 
+                     "%OS_VERSION%&%PLATFORM_VERSION%&%DISTRIBUTION%&%DISTRIBUTION_VERSION%");

Are they missing query string var names?
BTW - tracking service changes in bug 430278.
(In reply to comment #8)
> Dave, are these supposed to have var= added to them (only see &):
> +  gPrefs.setCharPref(PREF_BLOCKLIST_URL, "http://localhost:4444/2?" + 
> +                     "%APP_ID%&%APP_VERSION%&%PRODUCT%&%VERSION%&%BUILD_ID%&"
> +
> +                     "%BUILD_TARGET%&%LOCALE%&%CHANNEL%&" + 
> +                    
> "%OS_VERSION%&%PLATFORM_VERSION%&%DISTRIBUTION%&%DISTRIBUTION_VERSION%");
> 
> Are they missing query string var names?

I don't see any need for them for the unit test. So long as it is replacing the parameters the surrounding text isn't all that relevant. I would have done it as part of the path rather then the query string but the js http server makes that harder to catch.
Okay, just looked weird at first for the URL pref.  :)
Comment on attachment 316986 [details] [diff] [review]
patch rev 1

>diff --git a/toolkit/mozapps/extensions/src/nsBlocklistService.js b/toolkit/mozapps/extensions/src/nsBlocklistService.js
>--- a/toolkit/mozapps/extensions/src/nsBlocklistService.js
>+++ b/toolkit/mozapps/extensions/src/nsBlocklistService.js
>...
>@@ -196,16 +203,87 @@ function matchesOSABI(blocklistElement) 
>     if (choices.length > 0 && choices.indexOf(gApp.XPCOMABI) < 0)
>       return false;
>   }
>   
>   return true;
> }
> 
> /**
>+ * Gets the current value of the locale.  It's possible for this preference to
>+ * be localized, so we have to do a little extra work here.  Similar code
>+ * exists in nsHttpHandler.cpp when building the UA string.
>+ */
>+function getLocale() {
>+  try {
>+      // Get the default branch
>+      var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>+          .getService(Components.interfaces.nsIPrefService);
>+      var defaultPrefs = prefs.getDefaultBranch(null);
>+      return defaultPrefs.getCharPref(PREF_GENERAL_USERAGENT_LOCALE);
>+  } catch (e) {}
>+
>+  return gPref.getCharPref(PREF_GENERAL_USERAGENT_LOCALE);
>+}
>+
>+/**
>+ * Read the update channel from defaults only.  We do this to ensure that
>+ * the channel is tightly coupled with the application and does not apply
>+ * to other instances of the application that may use the same profile.
>+ */
nit: s/instances/installations/



>@@ -215,16 +293,54 @@ function Blocklist() {
>   gVersionChecker = Cc["@mozilla.org/xpcom/version-comparator;1"].
>                     getService(Ci.nsIVersionComparator);
>   gConsole = Cc["@mozilla.org/consoleservice;1"].
>              getService(Ci.nsIConsoleService);
> 
>   gOS = Cc["@mozilla.org/observer-service;1"].
>         getService(Ci.nsIObserverService);
>   gOS.addObserver(this, "xpcom-shutdown", false);
>+
>+  // Not all builds have a known ABI
>+  try {
>+    gABI = gApp.XPCOMABI;
>+  }
>+  catch (e) {
Is there a reason not to do what the EM code does here (e.g. UNKNOWN_XPCOM_ABI)?

>+    LOG("UpdateService", "XPCOM ABI unknown: updates are not possible.");
>+  }
Copy / paste from "UpdateService". Also note that the LOG function only takes one param.

>+
>+  var osVersion;
>+  var sysInfo = Components.classes["@mozilla.org/system-info;1"]
>+                          .getService(Components.interfaces.nsIPropertyBag2);
>+  try {
>+    osVersion = sysInfo.getProperty("name") + " " + sysInfo.getProperty("version");
>+  }
>+  catch (e) {
>+    LOG("UpdateService", "OS Version unknown: updates are not possible.");
>+  }
same here
Attachment #316986 - Flags: review?(robert.bugzilla) → review-
would be very good to get this into fx2.0.0.next also so that we can have coherent tracking.
Flags: blocking1.8.1.15?
Whiteboard: [needs new patch]
Attached patch patch rev 2 (obsolete) — Splinter Review
Fixes to my sloppy copy and pasting.

I've included the unknownABI bit but would like confirmation from morgamic or Alex that that is an acceptable thing to pass.

I've also included the config changes for review though those can't land till the server side is done (bug 430278). A couple of the apps seem to have an incorrect detailsURL so I've fixed those up too.
Attachment #316986 - Attachment is obsolete: true
Attachment #317229 - Flags: review?(robert.bugzilla)
Depends on: 430278
Whiteboard: [needs new patch] → [has patch][need review robstrong]
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.15?
Flags: blocking1.8.1.15+
Comment on attachment 317229 [details] [diff] [review]
patch rev 2

iirc my main concern with the previous patch was that if the ABI was unknown it looked like it would bail and not download the blocklist. You could always just omit it if you prefer. Either way r=me
Attachment #317229 - Flags: review?(robert.bugzilla) → review+
Whiteboard: [has patch][need review robstrong] → [has patch]
Attached patch patch rev 3Splinter Review
This is just a minor tweak to bring the blocklist url in line with what it should be.

Seeking approval to land, this changes the url used to request the blocklist but does not affect the actual functionality of the blocklist. There is better testing that the blocklist service is actually attempting to retrieve included.
Attachment #317229 - Attachment is obsolete: true
Attachment #317361 - Flags: review+
Attachment #317361 - Flags: approval1.9?
Comment on attachment 317361 [details] [diff] [review]
patch rev 3

a1.9=beltzner
Attachment #317361 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Whiteboard: [has patch] → [has approval]
Keywords: checkin-needed
Whiteboard: [has approval] → [has approval][needs bug 430278 to land]
Whiteboard: [has approval][needs bug 430278 to land] → [has patch][has approval][needs bug 430278 to land]
Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.326; previous revision: 1.325
done
Checking in calendar/sunbird/app/profile/sunbird.js;
/cvsroot/mozilla/calendar/sunbird/app/profile/sunbird.js,v  <--  sunbird.js
new revision: 1.54; previous revision: 1.53
done
Checking in mail/app/profile/all-thunderbird.js;
/cvsroot/mozilla/mail/app/profile/all-thunderbird.js,v  <--  all-thunderbird.js
new revision: 1.113; previous revision: 1.112
done
Checking in suite/browser/browser-prefs.js;
/cvsroot/mozilla/suite/browser/browser-prefs.js,v  <--  browser-prefs.js
new revision: 1.91; previous revision: 1.90
done
Checking in toolkit/mozapps/extensions/src/nsBlocklistService.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/src/nsBlocklistService.js,v  <--  nsBlocklistService.js
new revision: 1.12; previous revision: 1.11
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/test_bug430120.js,v
done
Checking in toolkit/mozapps/extensions/test/unit/test_bug430120.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/test_bug430120.js,v  <--  test_bug430120.js
initial revision: 1.1
done
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [has patch][has approval][needs bug 430278 to land]
Target Milestone: --- → Firefox 3
Attached patch branch patchSplinter Review
On the branch the blocklist is all still bundled in with the EM. This is still basically the same patch though.
Attachment #322920 - Flags: review?(robert.bugzilla)
Comment on attachment 322920 [details] [diff] [review]
branch patch

r=me Thanks!
Attachment #322920 - Flags: review?(robert.bugzilla) → review+
Comment on attachment 322920 [details] [diff] [review]
branch patch

Seeking approval to land this on the branch.
Attachment #322920 - Flags: approval1.8.1.15?
Comment on attachment 322920 [details] [diff] [review]
branch patch

Approved for 1.8.1.15, a=dveditz for release-drivers
Attachment #322920 - Flags: approval1.8.1.15? → approval1.8.1.15+
Landed on branch

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.71.2.84; previous revision: 1.71.2.83
done
Checking in calendar/sunbird/app/profile/sunbird.js;
/cvsroot/mozilla/calendar/sunbird/app/profile/sunbird.js,v  <--  sunbird.js
new revision: 1.17.2.28; previous revision: 1.17.2.27
done
Checking in mail/app/profile/all-thunderbird.js;
/cvsroot/mozilla/mail/app/profile/all-thunderbird.js,v  <--  all-thunderbird.js
new revision: 1.44.2.47; previous revision: 1.44.2.46
done
Checking in toolkit/mozapps/extensions/src/nsExtensionManager.js.in;
/cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v  <--  nsExtensionManager.js.in
new revision: 1.144.2.66; previous revision: 1.144.2.65
done
Keywords: fixed1.8.1.15
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.15pre) Gecko/20080610 BonEcho/2.0.0.15pre

Branch looks good. Verifying.
Depends on: 434243
Product: Firefox → Toolkit
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.