Firefox 2 software update should send information about the specific windows OS version on the client. This will allow us to update Win2k/XP users to Firefox 3, but not Win98/ME users (since Firefox3 won't work on win98/ME).

Gah, yeah, definitely need this. -> Seth, this can happen late, after the tabbrowser stuff is done.

According to Live HTTP Headers, Firefox is already sending this information to the update server (as I'd expect) - since it's part of the UA string... https://aus2.mozilla.org/update/1/Firefox/3.0a1/2006061205/WINNT_x86-msvc/en-US/nightly/update.xml GET /update/1/Firefox/3.0a1/2006061205/WINNT_x86-msvc/en-US/nightly/update.xml HTTP/1.1 Host: aus2.mozilla.org User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060612 Minefield/3.0a1 Accept: text/xml,application/xml,application/xhtml+xml,text/html;q=0.9,text/plain;q=0.8,image/png,*/*;q=0.5 Accept-Language: en-gb,en;q=0.7,en-us;q=0.3 Accept-Encoding: gzip,deflate Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.7 Keep-Alive: 300 Connection: keep-alive Cache-Control: no-cache Cookie: aus=81.178.99.216.1132243438847790 HTTP/1.x 200 OK Date: Mon, 12 Jun 2006 20:56:18 GMT Server: Apache/2.0.52 (Red Hat) X-Powered-By: PHP/4.3.9 Content-Length: 42 Connection: close Content-Type: text/xml;

We cannot rely on the UA string directly, for various performance and correctness reasons (a not-insignificant number of users spoof their UA string). We should put the same information in the URL.

updating swag and accepting

here's what I was thinking: change the app.update.url pref in mozilla/browser/app/profile/firefox.js to something like: pref("app.update.url", "https://aus2.mozilla.org/update/1/%PRODUCT%/%VERSION%/%B UILD_ID%/%BUILD_TARGET%/%OS_VERSION%/%LOCALE%/%CHANNEL%/update.xml"); (I added the %OS_VERSION% after %BUILD_TARGET%) In mozilla/toolkit/mozapps/update/src/nsUpdateService.js.in, in the UpdateService() constructor (where we currently set gABI, for example) I'll use the oscpu attribute of the nsIHttpProtocolHandler interface to get the os version, which will be something like "Windows NT 5.1". I'll follow the code in mozilla/xpfe/components/updates/src/nsUpdateNotifier.js and do somethingl ike: var httpHandler = Components. classes["@mozilla.org/network/protocol;1?name=http"]. getService(Components.interfaces.nsIHttpProtocolHandler); gOscpu = httpHandler.oscpu; I need to escape the spaces in to %20 since the string will be part of the URL. There is code already in nsUpdateServer.js.in to replace all the + to %2B, so I'll add something like: url = url.replace(/\+/g, "%2B"); +url = url.replace(/ /g, "%20"); in nsUpdateServer.js.in, I'll then add: url = url.replace(/%OS_VERSION%/g, gOscpu); questions: 1) the bug is about windows only, but I think we should add this for all platforms. 2) for thunberbird, which uses the same upgrade code, is the http protocol handler available? (I think the answer is yes, but i'll check.) 3) I'll talk to the aus2.mozilla.org folks to let them know what is coming. does this seem like the right approach?

simply escaping the spaces may not be enough, as I'm not sure if oscon value can contain other characters that could mess up the url. it might be necessary to url escape the gOscon value.

Seth: it might make sense to add something to nsIXULRuntime for the real OS version. i worry that someday the HTTP handler might be "fixed" to return an OS version field that corresponds to the UA string being spoofed. also, you might need to bump the AUS URL's version number (i.e., the leading "1" might need to be a "2"). Keep in mind that other people (mkaply at least) are relying on the format of the URL string as well.

Seth and I talked about changes that would have to happen to the server-side component of Software Update (aka AUS). So, we agreed on a few points: * the updateVersion (hard-coded) would be bumped to 2 to reflect the change in the URI * the sever-side code would have an additional case for this new version, which would ultimately change the flow/behavior of how the XML output is generated Questions: * How would we determine whether or not a given platform is a member of the "blacklisted" or "unsupported" list? One possible solution would be adding an array to the AUS config that defines these platforms. * We are assuming that if a given platform is no longer supported the default output for all updates would be "no updates" -- which is the well-formed blank XML document. Is this correct? We assumed this because we felt that telling people their OS sucks is outside the scope of what our browser should be doing. :) To see the config document for AUS: http://lxr.mozilla.org/mozilla/source/webtools/aus/xml/inc/config-dist.php

We're not depending on it yet, but we would need to update the docs here: http://developer.mozilla.org/en/docs/Setting_up_an_update_server I think songbird and flock are depending on stuff though...

thanks to darin, morgamic and mkaply for the feedback. darin, adding a readonly oscpu attribute to nsIXULRuntime (and then doing gApp.oscpu in nsUpdateService.js.in) sounds better to me. for the implementation of nsXULAppInfo::GetOscpu(), nsHttpHandler::InitUserAgentComponents() currently contains the code generating the value we using in the user agent. see http://lxr.mozilla.org/mozilla1.8/source/netwerk/protocol/http/src/nsHttpHandler.cpp#614 Any suggestions about where to move that code, or if netwerk should depend on toolkit, or vice versa, or if should be moved some other place in the tree and have both netwerk and toolkit depend on it.

It might makes sense to factor that bit out of HTTP into some file in xpcom/base -- maybe a new file. In general, netwerk should not depend on toolkit :-)

interesting, there is http://lxr.mozilla.org/mozilla1.8/source/xpcom/base/nsISystemInfo.idl already, which was intended to be a "thin wrapper around PR_GetSystemInfo()" I could implement nsXULAppInfo::GetOscpu() on top of PR_GetSystemInfo(), using the PR_SI_RELEASE for the PRSysInfo cmd argument to PR_GetSystemInfo(). actually, it looks like shaver did implement it at one point (in 2000) and then bryner removed it (since it was unused, or never built?) in 2004. see http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/xpcom/base/nsSystemInfo.cpp&root=/cvsroot (But I'm not sure why the interface was never removed.) darin, how do you feel about me implementing nsXULAppInfo::GetOscpu() on top of PR_GetSystemInfo()?

My foresight is legendary. You may applaud.

just chatted with Mike "Nostradamus" Shaver over irc, and instead of using the existing nsISystemInfo interface, which has four string values (hostname, OSName, OSVersion, and architecture), we discussed this: hijack the interface and use a nsIProperty bag instead. then, in the implementation, fill the property bag with the values for hostname, OSName, OSVersion, and architecture (from PR_GetSystemInfo()), and fix nsUpdateService.js.in to use the nsISystemInfo interface (and *not* add oscpu to nsIXULRuntime). shaver points out two things: 1) extensions may need the system info at install / upgrade time, too (so that's one reason not to put into nsIXULRuntime) 2) if we use a property bag, later we can expose things like "how much memory does this system have" without changing the interface. darin, how does this sound?

sounds fine. fwiw, nsIXULRuntime is always available.

from my windows xp machine, here's the old URL my build would ping: https://aus2.mozilla.org/update/1/Firefox/3.0a1/0000000000/WINNT_x86-msvc/en-US/default/update.xml After the patch, here's new new URL: https://aus2.mozilla.org/update/2/Firefox/3.0a1/0000000000/WINNT_x86-msvc/Windows_NT_5.1/en-US/default/update.xml notice the bumped version (1 -> 2) and the additional OS version ("Windows_NT_5.1") One concern I have is if: PR_SI_SYSNAME + "_" PR_SI_RELEASE contains any characters that could affect the URL and break update.

(In reply to comment #17) > One concern I have is if: > > PR_SI_SYSNAME + "_" PR_SI_RELEASE > > contains any characters that could affect the URL and break update. I am extremely concerned about that. URL-escape them before doing the substitution, to be sure? We also don't want / in there, I expect.

A different separator for spaces and between PR_SI_SYSNAME and PR_SI_RELEASE would be a good thing I would think.

new patch coming that does: + gOSVersion = encodeURIComponent(sysInfo.properties.getProperty("name") + + " " + + sysInfo.properties.getProperty("version")); resulting in: https://aus2.mozilla.org/update/2/Firefox/3.0a1/0000000000/WINNT_x86-msvc/Windows_NT%205.1/en-US/default/update.xml I'll check with morgamic to see if Windows_NT%205.1 is acceptable.

Dumb question - do we have a clearly enumerated set of platform/version numbers to be sure that the server-side AUS will not accidentally not provide updates (I'm esp worried about the many Linux variants for example). Also quick note that we'd like to get any AUS impacting changes (like this) in beta1 so we can test at scale during the b1->b2 update cycle.

Comment on attachment 228163 [details] [diff] [review] updated patch, change separator to space, use encodeURIComponent() to escape >Index: xpcom/base/nsISystemInfo.idl >+[scriptable,uuid(27e3850f-3ff5-4cd9-b2a7-b2c7e276e133)] > interface nsISystemInfo : nsISupports > { >+ readonly attribute nsIPropertyBag2 properties; > }; I don't think we need this interface. The ContractID for the SystemInfo class can just QI to nsIPropertyBag2. The easiest way to do that is to just have your class extend from nsHashPropertyBag. Then you can define an Init method that calls nsHashPropertyBag's Init method and populates the nsHashPropertyBag. Doing this will reduce the amount of code needed for consumers since they can just access the property bag interface directly. It'll also simply your implementation a bit since you won't need to CI the hash property bag. Since you're adding code to XPCOM there's no need to use XPCOM to get at a neighboring class! :-)

There certainly won't be enumerations of possible values... the server should be smart enough to provide updates for all specific version except those in some kind of blacklist.

Comment on attachment 228181 [details] [diff] [review] better patch, per darin's review comments. note, nsISystemInfo is removed > "@mozilla.org/systeminfo;1" I recomend "system-info" for readability and it's a tad more consistent with other ContractIDs in the system. Should nsSystemInfo::Init really fail if it is unable to populate some fields? It might be nice to code nsSystemInfo::Init as a for loop. const struct { PRSysInfo cmd; const char *name; } items[] = { { PR_SI_SYSNAME, "name" }, { PR_SI_HOSTNAME, "host" }, ... }; Then iterate over that list and call PR_GetSystemInfo + SetPropertyAsACString. The result should be less generated code. r=darin

Per http://people.redhat.com/drepper/dsohowto.pdf I believe making "name" an array instead of a pointer would be even less code.

Please note we shouldn't land this patch anywhere until Bug 343689 is resolved or else we will break nightly updates.

Also - for futures should we consider using query params as a place to stash additional information as a way to be more backwards-compat?

new patch coming, with a slightly different pref value. morgamic asked for the OS_VERSION to be at the end of the patch, right before update.xml here's what we'll get now: https://aus2.mozilla.org/update/2/Firefox/3.0a1/0000000000/WINNT_x86-msvc/en-US/default/Windows_NT%205.1/update.xml

> Please note we shouldn't land this patch anywhere until Bug 343689 is resolved > or else we will break nightly updates. I won't land until morgamic (and bug #343689) give me the green light.

> Also - for futures should we consider using query params as a place to stash > additional information as a way to be more backwards-compat? This seems like a good idea to me since this change could then be made in a backward compatible way. Back in the day, I think we discussed using query params, but the feeling was that it wouldn't matter since we'd be making server-side changes in lock-step. Maybe we are learning otherwise? Also, the file system paths were nice because you could build a simple AUS from files in a corresponding folder structure.

Comment on attachment 228219 [details] [diff] [review] moved %OS_VERSION% in the pref, per morgamic >Index: xpcom/base/nsSystemInfo.cpp >+ static const struct { >+ PRSysInfo cmd; >+ const PRUnichar *name; >+ } items[] = { >+ { PR_SI_SYSNAME, NS_LITERAL_STRING("name").get() }, >+ { PR_SI_HOSTNAME, NS_LITERAL_STRING("host").get() }, >+ { PR_SI_ARCHITECTURE, NS_LITERAL_STRING("arch").get() }, >+ { PR_SI_RELEASE, NS_LITERAL_STRING("version").get() } >+ }; Sadly, this use of NS_LITERAL_STRING does not work on all platforms :-( You see, on some platforms, there is no L"foo" syntax that can be used to generate a UTF-16 literal, so instead we are forced to impl NS_LITERAL_STRING in terms of NS_ConvertASCIItoUTF16. That means that the memory returned by .get() is a temporary that is destroyed when the NS_ConvertASCIItoUTF16 object is destroyed. In this case, those objects are destroyed after the "items" array is initialized. As a result, accessing items[i].name may result in addressing invalid memory. Perhaps you could solve that by using NS_NAMED_LITERAL_STRING or by using NS_ConvertASCIItoUTF16 in your loop body. Doesn't it suck having to support non-tier1 platforms? Windows, Linux and Mac all have workable L"foo" support. r=darin w/ that fixed

darin, thanks for the info about the way I used NS_LITERAL_STRING and certain platforms. I'll fix that and attach a new patch. in parallel, I'll seek a= for the branch. morganic has a reviewed patch for bug #343689, and once he pushes that, I'd like to check in this change to trunk and branch for 2.0b1.

Comment on attachment 228219 [details] [diff] [review] moved %OS_VERSION% in the pref, per morgamic seeking a=. but note, checkin to branch and trunk is conditional on bug #343689 being checked in and pushed on the server side.

>> Also - for futures should we consider using query params as a place to stash >> additional information as a way to be more backwards-compat? > >This seems like a good idea to me since this change could then be made in a >backward compatible way. I chatted with morgamic over irc, and he had this to say: <morgamic> there is a top level rewrite that just forwards the path <morgamic> so /foo/foo/foo/foo/foo/update.xml ---> aus2.mozilla.org/update/index.php?path=/foo/foo/foo/foo/foo/update.xml perhaps this is why he prefers having the values in the path, and not as the query string? I'll let morgamic weigh in here, but if we decide to the query string approach, for the client side, this should just be a matter of changing the pref: +pref("app.update.url", "https://aus2.mozilla.org/update/2/%PRODUCT%/%VERSION%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/update.xml?osversion=%OS_VERSION%");

fixed on the trunk.

Wanted to note -- the server-side component will be pushed when bug 343715 is resolved, which blocks bug 343689. Until then, there will be no updates (would be the case anyway, as b2 isn't out???). Within 24 hours the server-side update should be completed and we can then look at testing b1->b2. :P We would still need an eventual list of unsupported platforms so the server-side component would know which OS_VERSION strings to deny updates for. I'm still not very clear on where to get these -- any thoughts on that?

fixed on branch.

it's a shame that I didn't think ahead and try to get this into 1.5.0.x before the major update. We could have used it to, for example, not offer major updates to certain users on certain OS. maybe we should make this part of 1.5.0.10, so that we have this option in the future?

We could of course use the ua string for that, though it's a lot harder on AUS.

Let's get something in place in 1.5.0.10 so we're better prepared for 1.5.0.11. Seth: Can you please ask for approval for 1.8.0.10 with the appropriate patch?

morgamic / preed: on the aus side, will switching 1.5.0.10+ to use the version 2 style update url cause any problems or work for AUS? (I don't think so, but I thought I'd double check.) I agree that we'd want this on the MOZILLA_1_8_0_BRANCH, because, for example: a) 1.5.0.x -> 3.0 might be offered at some point, but only for certain versions of windows b) we might only offer partial updates for a given os version, due to issues like #341190

Actually I think it will, given the patch we made to "fix" bug 360127. We'd be breaking the rule we set in place that assumes that version 2 url's only come in from Firefox 2. We would have to remove the hack we put in that ignores v2 url's coming from 1.5.* clients.

morgamic, thanks for the quick response. I forgot about that fix for bug #360127. what if on the MOZILLA_1_8_0_BRANCH, I ported this fix, but used a different version, other than 2, like 3? (same format, though.) Or is that asking for trouble?

We have the option of doing that, or undoing the "fix" for bug 360127. I don't mind the URL version 3 at all -- given that if we simply undid the fix it'd potentially send out some incorrect updates to people suffering from the related profile/installer bug. If we can verify that there aren't that many people though, undoing that patch is a way to do it that doesn't require changes to the client. Which method is best for everyone?

> If we can verify that there aren't that many people though, undoing that patch > is a way to do it that doesn't require changes to the client. we still need a change to the client. specifically, I need to backport in this bug to the MOZILLA_1_8_0_BRANCH. I think using version 3 makes more sense than backing out the "fix" for bug 360127, and risk sending people incorrect updates. mike writes: > Shouldn't this be done today? Does it block the next release? > Which way do you want to go? I agree with jay that if we had this in 1.5.0.10, it gives us options (past 1.5.0.10) for future major releases. mike, if I did version 3, how much work would it be on the AUS side to keep the existing check for version 2, but make version 3 not have the 1.5.0.x version check?

I meant to mark this wanted, not blocking... since I don't think this is a true blocker. Based on the discussions I've had with Seth and the risks that we might introduce with AUS changes this late in the game, I think we will need to thoroughly test both server and client changes...which might not be doable given the schedule for the upcoming release (and the pending major update also makes it a bit risky for changes going in now).

Adding support for a URL version 3 would be an easy patch. Right now AUS doesn't have a case for URL version 3, and we'd just be adding that case in and allowing it to proceed with the update. It's not a major change, as it's over the top of everything else and only covers incoming URLs that don't yet exist. I'll leave it up to you guys to decide if/when we need to do this... but we should probably file a bug for it somewhere else. I created an AUS bug for it in bug 368117.

> we should probably file a bug for it somewhere else. agreed. see bug #368200 jay (or someone who has privs) can you clear the wanted 1.8.0.x flag?

clearing wanted1.8.x flag, we're done with this bug.

robert, this bug, along with bug #343689 on the aus side is what we'll use to prevent Firefox 3 major updates from being offered to Win98 users.