Closed Bug 407875 Opened 17 years ago Closed 15 years ago

Unprivileged users are not notified of security updates

Categories

(Toolkit :: Application Update, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- beta4-fixed
status1.9.1 --- .10-fixed

People

(Reporter: bobv, Assigned: robert.strong.bugs)

References

Details

(Keywords: verified1.9.2, Whiteboard: [sg:want P1])

Attachments

(7 files, 7 obsolete files)

19.97 KB, patch
mossop
: review+
robert.strong.bugs
: review+
dietrich
: approval1.9.2+
Details | Diff | Splinter Review
23.10 KB, image/png
Details
21.85 KB, image/png
Details
4.17 KB, patch
mossop
: review+
Details | Diff | Splinter Review
1.06 KB, patch
Dolske
: review+
Details | Diff | Splinter Review
27.84 KB, patch
mossop
: review+
Details | Diff | Splinter Review
1.88 KB, text/x-python-script
Details
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-GB; rv:1.8.1.11) Gecko/20071127 Firefox/2.0.0.11
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-GB; rv:1.8.1.11) Gecko/20071127 Firefox/2.0.0.11

This major security bug was sort of reported in bug 318855 two years ago but has still not been fixed. I believe the reason for this is that too much was asked for. It is major because people following best security practice will have unpatched browsers because nobody tells them that they need to apply updates.

The bug is that if a user has no write access to the installation directory then:
 (1) there is no automatic notification that updates are available
 (2) The "Check for updates" option in the Help menu is greyed-out.

Possibly there should be a configuration option so that the administrator can disable the warning. But the default should be that users are told when they are running an unsafe browser.

I have reported it against Linux, but I believe it applies to all systems. 

Bug 318855 suggests that the user be prompted for the root password so that the update can be installed. That is (a) less urgent, (b) harder to implement and (c) more controversial. I suggest that the notification problem be fixed before  that bug is addressed. 

Reproducible: Always

Steps to Reproduce:
1. Wait till you hear via the grapevine that a new Firefox is available
2. Start Firefox and notice the complete lack of warnings
3. Notice that Help/Check for Updates is greyed out
Reporting the same bug you know about will not get it fixed any faster. May I suggest https://lists.mozilla.org/listinfo/announce in the interim.
Status: UNCONFIRMED → RESOLVED
Closed: 17 years ago
Resolution: --- → DUPLICATE
This is a legitimate subset of bug 318855, not a duplicate. We should fix at least this one, if not both, for Firefox 3. (note: I think we made changes so this isn't  an issue on Vista, but it remains one for WinXP, Linux and Mac).
Blocks: 318855
Status: RESOLVED → UNCONFIRMED
Flags: blocking-firefox3?
Keywords: uiwanted
Resolution: DUPLICATE → ---
Whiteboard: [sg:want p1]
Could just punt a message out to nsIAlertService, maybe?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: wanted-firefox3+
Flags: blocking-firefox3?
Flags: blocking-firefox3-
Sure that'd be great. All we need is a string and the go-ahead. We really need something here -- too many users get stuck in down-rev versions and probably have no idea that they are.
Assignee: nobody → johnath
Flags: blocking-firefox3.1?
Not a hard, will-not-ship blocker, but a P2 and very much wanted.
Flags: wanted-firefox3.1+
Flags: blocking-firefox3.1?
Flags: blocking-firefox3.1-
Priority: -- → P2
Target Milestone: --- → Firefox 3.1
note: not sure if this is wanted but we could also provide the ability for a non admin user that knows an administrator account / password to opt-in (opt-in since there is no way of reasonably knowing) the ability to update when notified. Bug 437349 lays the ground work for providing this.
re comment #2
This was working in FF2 but is not working in FF3 on Vista x64
re comment #7
btw: the test we perform that could make it work on Vista x64 is to see if we have write access to the install directory... otherwise it shouldn't have worked.
Robert - did you not check in a patch to make this work in vista!??
Bug# 351949
I did and it specifically wasn't supposed to duplicate the same behavior as Software Update on all versions of Windows.
Robert: please excuse me - I'm now extremely confused :-/

You stated on another bug that if it worked in vista it was a bug.
Yet there was another bug (351949) that was fixed so the update worked in vista - so this must be a valid scenario (or the previous bug and fix should be invalid)

That fix (from bug 351949) no longer works in FF3 - hence my comment#7

So your comment re all versions of windows - I don't personally care about windows 98/Me/2000/XP/2003 - I have Vista (32bit and 64bit). I have now updated FF3 on my 32bit Vista (from 2.0.0.15) and the check for updates is also disabled there after the update.
  
32bit is vista premium + sp1
64bit is vista ultimate + sp1

So are you saying I should re-open bug# 351949 and tag it as a regression as this doesn't seem to care if it is a 64bit or 32bit version of vista?
(In reply to comment #11)
> Robert: please excuse me - I'm now extremely confused :-/
> 
> You stated on another bug that if it worked in vista it was a bug.
If a non-privileged user was notified of updates on any system including Vista 64 when they don't have write access to the installation directory then it is a bug. If the user is running Vista and is in the administrators group with UAC turned on and the installation directory under the system's "Program Files"  then they would be notified of updates.

> Yet there was another bug (351949) that was fixed so the update worked in vista
That was specifically for handling the Vista admin user with UAC turned on with the installation directory under the system's "Program Files".

> - so this must be a valid scenario (or the previous bug and fix should be
> invalid)
Not sure what you mean by this.

> That fix (from bug 351949) no longer works in FF3 - hence my comment#7
That fix is specifically for an admin user with UAC turned on with the installation directory under the system's "Program Files"

>..snip
> So are you saying I should re-open bug# 351949 and tag it as a regression as
> this doesn't seem to care if it is a 64bit or 32bit version of vista?
Are you saying that a user that is a member of the administrators group with UAC turned on and the installation directory is under the system's "Program Files" directory is not being informed of updates?
Firefox 2 had the "bug", then. On a non-admin Vista account, Firefox 2 would check for, download and apply (with UAC prompt) updates exactly as one would expect.

A quick check with Process Monitor shows that Firefox 2 checks:
 %LocalAppData%\Mozilla\Firefox\Mozilla Firefox\update.test
 %LocalAppData%\Mozilla\Firefox\Mozilla Firefox\updates\0\update.test

Firefox 3 checks:
 %LocalAppData%\Mozilla\Firefox\Mozilla Firefox\update.test
 %LocalAppData%\Mozilla\Firefox\Mozilla Firefox\updates\0\update.test
 %ProgramFiles%\Mozilla Firefox\update.test

The Local AppData file tests succeed, the Program Files one fails (all as expected). Neither version was virtualised by Vista. I used 2.0.0.16 and 3.0, with elevated installation both times to a directory only administrators can write to.

There is certainly a regression in functionality for non-admin Vista users - how you wish to address it is up to you. :)
(In reply to comment #13)
> Firefox 2 had the "bug", then...
Bug 390433 never landed on the branch. :(
Ok, stop the bus. In what world is it in the interest of security that ANYONE is not informed of a security UPDATE?? You've got the cart in front of the horse brother, 'Check for Updates' should NEVER be grayed out. Let all users check for updates, auto-check for updates, download updates.. even attempt to INSTALL updates. THEN have a popup inform them that they need Administrator access to install, if necessary.

Hackers are laughing at you. Fix it. Fix it now!

You're leaving end users with their butts hanging in the wind because they have no idea that they're not up to date. This should be a priority one bug, unless your core is already busted. Which it's not.

The bug is that 'users are not notified'. Fix THAT, for everyone. Stop argueing about situational programming, and fix it across the board so EVERYONE is notified. Let the OS worry about the password prompt if the install needs it. It's your (the Firefox team) job to make sure everyone knows they need a security update. Once that's done, then you can get cute with notifications and what not.

Peace out.

p.s. I LOVE firefox, so take this with the compassion that inspired the post, ok?
This bug has a security group p1 wanted, a wanted firefox 3, a wanted firefox 3.1, and it is being worked on. Further "you should do this" comments aren't going to help.
My main concern was that this bug and it's brothers are in the news. Just techie stuff now, but you know MS will try to take this mainstream the first chance they get. I just don't want to see the reputation you guys have built get ruined or even marred in the publics eye.

Reading through the posts here, it sounded like the train was starting to head down the wrong track. That's mainly what I was trying to point out. Reading back through it, I guess my post does read kind of 'pushy', so sorry about that. I wouldn't have posted at all on a bug report if I didn't feel it was relevant.

I'm sure there are hundreds of thousands of others like me, out there, 'keeping the faith' as it were. Anyway, This post isn't adding any new info to the bug report, so I'll sign off and just keep my fingers crossed. :)
Perhaps the bug assignee could accept the bug then, to ensure people realise it is being actively worked on? The platform probably can be moved to All/All too, presuming the fix will apply cross-platform. I'm sure we're all looking forward to news of this being fixed. :)
(In reply to comment #12)
> (In reply to comment #11)
> > So are you saying I should re-open bug# 351949 and tag it as a regression as
> > this doesn't seem to care if it is a 64bit or 32bit version of vista?
> Are you saying that a user that is a member of the administrators group with
> UAC turned on and the installation directory is under the system's "Program
> Files" directory is not being informed of updates?

No the user is not an admin and does not belong to the Administrators Group.
However the user does have the administrator password. So they have limited account privileges[1] - which is what was in the title of 351949.
They are not limited by UAC but by the actual user rights assigned.

A quick fix for vista and UAC could be to remove the check on %ProgramFiles%\Mozilla Firefox\update.test.  Enterprises that don't want the check can turn off the check using prefs.js
OS: Linux → All
limited account privilege in the summary of Bug 351949 is a badly worded reference to an admin account that has limited privileges due to running with UAC (it can be taken both ways but reading the bug itself clarifies this especially the comments where disabing UAC fixed it) which can cause the behavior as cited in Bug 351949 comment #0.

The solution you suggest breaks for a user that doesn't know an administrator user's password. We have to take both into account and not everyone in this scenario will be in an enterprise where the admin can disable updates. 
> The solution you suggest breaks for a user that doesn't know an administrator
> user's password. We have to take both into account and not everyone in this
> scenario will be in an enterprise where the admin can disable updates. 
> 

then its a simple app.update.vistaUserWithUACHasAdminPassword that defaults do false but users in the know can set to true.

This would be a simple quick win for vista with UAC that is backwards compatable whilst the full cross platform issue is worked out.
Yes, this is already understood for the Vista case. I'm discussing other possible ways of solving this with the user experience team with hope to improve the user experience and solving more than just the Vista case.
(In reply to comment #22)
> Yes, this is already understood for the Vista case. I'm discussing other
> possible ways of solving this with the user experience team with hope to
> improve the user experience and solving more than just the Vista case.

Rob - update/install is much more your bailiwick than mine - I understand why Dan would have put my name on it, but would you agree this makes more sense assigned to you?
Johnathan, that works
Assignee: johnath → robert.bugzilla
Status: NEW → ASSIGNED
Product: Firefox → Toolkit
as said here https://bugzilla.mozilla.org/show_bug.cgi?id=318855#c66, problem still applies and as said above, on mac (test on 10.5)
mconnor: why is this not blocking? we have literally millions of users stranded on old versions, suspiciously tending to be the versions that got a lot of press. We need some notification UI for such stranded users.

If we can't think of something in the UI itself maybe it could be like the "firstrun" page after an update: next time they start they get an extra tab saying "A security update is available, please go install it".
Flags: blocking1.9.1- → blocking1.9.1?
That's a good question. Apparently it got voted out to a Priority 2? How does that work? Stop releasing until this and anything else that SHOULD be fixed before a new release IS fixed. It makes it feel like somebody is dropping the ball. -I- know you're all terribly busy with who knows how many projects, but the general public won't understand it.
(In reply to comment #26)
>...
> If we can't think of something in the UI itself maybe it could be like the
> "firstrun" page after an update: next time they start they get an extra tab
> saying "A security update is available, please go install it".
Dan, I've done some preliminary work on this already and plan on having this fixed for Beta 2. This will be the primary bug I will be working on as soon as I finish up Bug 324121.

The issue and options available are all very well understood for the trunk though your suggestion about opening a page for notification might be a good way to go for 1.8 and 1.9. Thanks
Is this work try to target beta 2?
Won't block release on this, though interest remains high.
Flags: blocking1.9.1? → blocking1.9.1-
(In reply to comment #32)
> Won't block release on this, though interest remains high.

This bugs birthday was last month. Stop blowing smoke up our *** and admit that it's not going to get fixed.

Did I motivate you into proving me wrong? Good!! I'm disgusted by how long this bug has been floating around. I had the highest esteem for Mozilla software until this happened.

Don't worry, you've opened the door, some other company will step in and pick up the reliability ball. Opera perhaps?

Motivated yet? Good!!
(In reply to comment #34)
> Motivated yet? Good!!

Historically, acting like a dick has tended to demotivate bugs, rather than the reverse.  Not out of some childish game of spite, but because it makes the bug a less pleasant thing to work on.

If Rob has a patch that solves the problem and is well contained and tested, it will get in.  If you have one, that's great news, please attach it and request reviews.  In the meantime, take a look at https://bugzilla.mozilla.org/page.cgi?id=etiquette.html and try to exhibit a modicum of respect for the human beings who are trying, despite the feces-flinging, to build a browser and make the internet a better place.

(In reply to comment #34)
> Don't worry, you've opened the door, some other company will step in and pick
> up the reliability ball. Opera perhaps?

Super, you are welcome to use whichever browser you want, and I think it's great that you have so many browser options. In the meantime, if you'll keep your disgust to a minimum, we'll continue trying to fix this, along with our other bugs.  Help is appreciated!
(In reply to comment #35)

> Super, you are welcome to use whichever browser you want, and I think it's
> great that you have so many browser options. In the meantime, if you'll keep
> your disgust to a minimum, we'll continue trying to fix this, along with our
> other bugs.  Help is appreciated!

Hey I'll put my money where my mouth is, assign me something to do and I'd be glad to help out. Last time I tried to help out was when they tried to sort out the add-in search engine list. The guy in charge disappeared off the face of the planet for a year, showed back up and disappeared again.

I don't want to try to re-invent the wheel, tell me what's keeping things from moving forward, or assign me a task to solve, and I'll do it.
Resource / time constraints is what is keeping things from moving forward.

The strings for this bug were checked in attachment #354990 [details] [diff] [review] in bug 318855

In general what needs to be done is the code needs to change to allow checking and the page as shown in the screenshot in bug 318855 needs to be displayed when the user can't update... this will need to take into account existing functionality naturally.
Ok.. That wasn't terribly helpful. From my perspective I mean, I'm sure you're giving the background of what you are trying to fix. :) I've analyzed the two bugs reports, and the data set / code you pointed me to. Taken together, I see multiple problems, and the 'gist' of what you're trying to tell me is that THIS bug's only goal is to repair a small subset of the problem. That confuses me as there are ..a dozen? other bugs marked as duplicates of this bug with varying issues.

So.. I've outlined the way the logic should (pardon the pun) logically flow. I think it's safe to assume that this is NOT what currently exists, but please do bear with me. If someone could notate what is currently going on, or point out flaws in my logic outline, that would be great, but probably extraneous and confusing. As I noted, this bug IS over a year old. Best scenario: everything else I've noted as a seperate problem below has already been fixed. I can't imagine the odds on that, but you never know.

Bottom line, given the large number of bugs pointing here, I think we need to review to ensure that everything has been dealt with. There in, ignore my logic flow for the moment, if you will. I think what we need is for someone to go through and notate where the problem areas DO have their own seperate bugs (if there are any) then, we can determine the repair status on these seperate problems, then 'back the train down the tracks' and determine IF this small subset of the problem is all that is left to fix. This way we don't have to 're-invent the wheel'. We SHOULD re-invent the wheel, if the wheel has as many dings as it seems to. However, if we can repair ALL of the problems on the various platforms, thereby pounding the wheel back into a round shape, it wouldn't be necessary. Given the knot of problems we have here, I don't really see that working, but it is possible I suppose. Now then, into the meat of it:

A large part of the debate is on what the default settings should look like. I think part of the problem is that somewhere someone put 'the cart in front of the horse' As I said, I see a knot of multiple problems.

Problem # 1 The actual update logic. Here's my take on what SHOULD be happening.

~ (NOTE: user clicked on 'Check for updates' OR auto-update initialized.) ~

1) NEVER gray out 'Check for updates'. This would be the first thing to re-code.

<Check vendor>

2) Then respond with:
	a. 'This version of $brandname was not distributed by Mozilla. Please contact $vendor to obtain current version information.'
	b. Proceed to check for update

<Check for Update>

3) Depending on the availability of an update:
	a. 'We could not contact the update server.' blah blah blah..
	b. No update from Mozilla:

	<Check if automatic update>

		I. If auto-update flag is false: 'Congratulations, you have the most current version of Mozilla $brandname.'
		II. If auto-update flag is true: Gracefully exit.
	c. There is an update from Mozilla: proceed to check for write access

<Check for write access>

4) This bit may be tricky, and will likely involve 2-3 different checks in each OS at a minimum, to acheive a 'yes or no' answer.

<Check for write access in all necessary locations on all os's>

	a. 'We have found a newer version of Mozilla $brandname, however, you do not have permission to update Mozilla $brandname. Please log in with an Administrative account to update.'
	b. If we have write access:

	<Check if auto-update>

		I. If auto-update flag is false: 'We have found a newer version of Mozilla $brandname, please click 'Ok' to install it, or 'Cancel' to abort.'
		II. If auto-update flag is true:

		<Check auto-update 'notify' option>

			A. If notify is true: re-use above notification 'We have found a newer version of Mozilla $brandname, please click 'Ok' to install it, or 'Cancel' to abort.'
			B. If notify is false: Silently proceed to update

~ (Congatulations! You are so cool you get to try to update!) ~

4a is where people are going to get annoyed and turn off auto-update eventually. ..or bug their sysadmin till the sysadmin switches it off. ^^ This is where we could consider throwing in a 'Check if the OS allows me to ask for the admin / root password' option. Bob Vickers had an idea to add a 'Enter password and proceed' type button. Perhaps have the button appear on OS'es that allow it?

This default should satisfy most everybody. At any rate, with enough flags in the hidden options, corporate / enterprise 'vendors' and people re-distributing Mozilla products should be able to set up the options to disable the auto-update, change the reason message, point to their own server, or whatever they want to do. If the vendor can't figure that part out, perhaps they should leave Mozilla auto-update turned on! ;) At any rate, a given enterprise could feasably 'push' an initial install with auto-update and etc. set up just the way they want it, then allow or disallow them as they see fit. Pushing their own update, or allowing a Mozilla update should, of course, leave the settings intact.

Now then problem #2, the auto-update panel option. If vendor is not Mozilla, gray out automatic update in the options panel. Add a grayed out message stating 'This version of $brandname was not distributed by Mozilla. Mozilla automatic update had been disabled.' (As stated above, we should allow the vendor to change the message if they desire to do so)

Problem # 3 People having issues with the 'Check what add-ins will be disabled' option. This will add a whole new layer of checks, if it is not thrown in at the end of the update routine. If it's wrapped up in this code, I recommend ditching the option en-tire-ly. Who wouldn't want to be notified if one of their add-ons get disabled?? Whatever, it's fluff, throw it back in once everything else is working correctly. ..or simply bolt it onto the end of the update process, where it makes more sense.

Problem # 4:

Copied from bug # 318855
	The admin installs the updates successfully. The
	user logs in again, starts Firefox but Firefox does not recognize that updates
	have been installed already and tries to download / install again.

Um.. Multi-user install over top of a single user install? Mozilla product was installed by user with admin priveledges, then user was demoted? I don't know if either of these is even possible or already fixed. If they're not:

1) Multi / Single user cross-breeding: Add a sanity check in the update routine to prevent the admin from screwing it up.
2) Demoted user: Check and notification at about 4a <Is this a singler-user mode install? Where'd our write access go??> Notification: 'The current installation requires THIS user to have administrative access to upgrade Mozilla $brandname.'

# 5 is also from bug # 318855
		I was originally watching this bug as I had FF installed on my OSX systems in a
	shared applications folder and only the administrator who installed was able to
	check or get update notifications.

	Even other admins on the same computer could not check for updates or receive
	update notifications.

Um.. I assume we're not correctly checking for write access in OSX, and what I've outlined would correct the notification issues? Is this correct by design?? SHOULD the OSX installation block other admins from updating? I'm not aware of how or if OSX manages single / multi user installs.

Moving right along.. #6 would involve debates between non-critical and critical updates or partial vs. full version updates being pushed. Forget about it. You want to screw things up? Allow someone to skip 3 partial updates then install a patch update over top of it. In this respect, there are no non-critical updates. If an enterprise wishes, they can certainly throw security to wind and only push full version updates. It would be BUTT stupid, and I would fire any sysadmin who suggested it, but they could do it if they so wished. It's a non-issue, scratch it off your list.

Another non-issue.. using 'software such as MakeMeAdmin or RunAsAdmin to temporarily elevate their privileges'. Since you're hacking your OSes security system, hack a work around to force your software to update anyway. You can't seriously expect a world-class distribution to cater to your system hacks. This belongs in a user help request forum, not a bug report. Next!

Ok!! I think that's everything wrapped up in the smallest nut shell possible! If I were running the show, and had to stick with the current structure, I would make it a hard goal to at least solve all the seperate problems / bugs before the next full version. If I had full latitude, I would examine the core structure and ensure that the checks are being performed in the correct logical order to prevent excessive looping and cut down on extraneous code (not to mention possible exploit holes). Then, once all the patches for the next full version are in place, patch all previous versions to the extent possible... or should I say feasible? Finally it would be irresponsible not to hold an official 'Period of mourning' or 'Moment of Silence' for those users who have been abandoned. Be certain that they are out there. Those chinese hackers pushing patched exploits are counting on it.

Hmm, that's all I can think of at the moment. I'm a bit burnt out now, so I'll take a break, then spend another hour reviewing it, then post it and check on any feed-back in the morning when I can get back to it. Oh! Please do post back ANY feedback, even if it's only partial. I want to try to ensure no one gets 'left behind' from now on. :)

Footnote: Should I perhaps submit this as a 'master' bug report for all of the potential sub-bugs?
(In reply to comment #38)
> Moving right along.. #6 would involve debates between non-critical and critical
> updates or partial vs. full version updates being pushed. Forget about it. You
> want to screw things up? Allow someone to skip 3 partial updates then install a
> patch update over top of it. In this respect, there are no non-critical
> updates. If an enterprise wishes, they can certainly throw security to wind and
> only push full version updates. It would be BUTT stupid, and I would fire any
> sysadmin who suggested it, but they could do it if they so wished. It's a
> non-issue, scratch it off your list.
This was more about security/stability maintenance releases vs feature releases. Do you really want to force a user to upgrade to version 4 if they prefer version 3, and it is still receiving security patches?

(In reply to comment #38)
> Another non-issue.. using 'software such as MakeMeAdmin or RunAsAdmin to
> temporarily elevate their privileges'. Since you're hacking your OSes security
> system, hack a work around to force your software to update anyway. You can't
> seriously expect a world-class distribution to cater to your system hacks. This
> belongs in a user help request forum, not a bug report. Next!
Firstly, that was rude – I did not post a comment 'expecting a world-class distribution to cater to [my] system hacks'. I spent time detailing a complete solution, and MMA and RRA were simply mentioned for (a) A reason for a download-only option [there are other reasons! Like delaying the update, or sharing the update file], and (b) A possible technique for privilege elevation on Windows. I would also not refer to these security utilties as 'system hacks' – they serve a purpose, and fulfil a requirement XP is otherwise unable to provide.
(In reply to comment #39)

> This was more about security/stability maintenance releases vs feature
> releases. Do you really want to force a user to upgrade to version 4 if they
> prefer version 3, and it is still receiving security patches?

Of course not. Isn't there an opt out during ALL full version upgrades (automatic or otherwise)?

> (In reply to comment #38)
> Firstly, that was rude – I did not post a comment 'expecting a world-class
> distribution to cater to [my] system hacks'.

My apologies, I didn't intend to single you out, I just used that example for ALL the people I saw asking about 'such and such a hack' I totally understand their validity, and where they may be required. But unless they get bought out by M$ written into a Windows update, the VAST minority of users will be using any given hack. That's why I used the snippet I did, to stem that way of thinking, and point out that there is a better and faster way of achieving a solution. :)

Ok.. seriously now, I'm going to bed! ;)
(In reply to comment #38)
> Ok.. That wasn't terribly helpful. From my perspective I mean, I'm sure you're
> giving the background of what you are trying to fix. :) I've analyzed the two
> bugs reports, and the data set / code you pointed me to. Taken together, I see
> multiple problems, and the 'gist' of what you're trying to tell me is that THIS
> bug's only goal is to repair a small subset of the problem. That confuses me as
> there are ..a dozen? other bugs marked as duplicates of this bug with varying
> issues.
> 
> So.. I've outlined the way the logic should (pardon the pun) logically flow. I
> think it's safe to assume that this is NOT what currently exists, but please do
>...
It is typical for the developer working on the bug to figure out the best way to implement the fix for a bug and analyze the bugs that are marked duplicate. If the dupe bugs are the same time as the original bug then nothing else needs to be done about them. If the dupe bugs have additional requests that can / should be fixed as part of the bug then they are done at the same time or they are unduped and fixed separately.

I haven't gone through all of the comments you referenced but at least one of them refers to applying updates which is bug # 318855 and is separate from this bug.

As far as this bug goes what needs to be done is stated in comment #0 and I reiterated in comment #37. The logic flow you have stated could to say the least be stated much more simply... if you want the logic flow of what is to be done to be reviewed just post a comment like:
---------
Can I get comments on how I think the logic flow should be:

1) Check for updates (btw: vendor is irrelevant... it is just a string that is available across all apps)
1a) if user has privileges to update continue as it works today
1b) if user does not have privileges show new ui
etc.
---------

(In reply to comment #38)
>...
> Problem # 1 The actual update logic. Here's my take on what SHOULD be
> happening.
> 
> ~ (NOTE: user clicked on 'Check for updates' OR auto-update initialized.) ~
> 
> 1) NEVER gray out 'Check for updates'. This would be the first thing to
> re-code.
That's a given...

> <Check vendor>
The string for vendor is just an available UI string across all apps and doesn't need to be stated in the logic flow.

> 2) Then respond with:
>     a. 'This version of $brandname was not distributed by Mozilla. Please
> contact $vendor to obtain current version information.'
>     b. Proceed to check for update
Why limit it to only Mozilla? The same logic will work with all distributions that provide toolkit's app update

> <Check for Update>
> 
> 3) Depending on the availability of an update:
>     a. 'We could not contact the update server.' blah blah blah..
>     b. No update from Mozilla:
> 
>     <Check if automatic update>
> 
>         I. If auto-update flag is false: 'Congratulations, you have the most
> current version of Mozilla $brandname.'
>         II. If auto-update flag is true: Gracefully exit.
This seems as if you are checking automatic updates when there is no update available... that makes no sense and the logic currently in use shouldn't need to change anyways so don't bother calling it out or just state use current logic.

>     c. There is an update from Mozilla: proceed to check for write access
> 
> <Check for write access>
> 
> 4) This bit may be tricky, and will likely involve 2-3 different checks in each
> OS at a minimum, to acheive a 'yes or no' answer.
For most OS's we only need to check one location and we already have checks for all locations.

> 
> <Check for write access in all necessary locations on all os's>
> 
>     a. 'We have found a newer version of Mozilla $brandname, however, you do
> not have permission to update Mozilla $brandname. Please log in with an
> Administrative account to update.'
The UI and text for the ui was already created / approved in bug 318855

>     b. If we have write access:
> 
>     <Check if auto-update>
> 
>         I. If auto-update flag is false: 'We have found a newer version of
> Mozilla $brandname, please click 'Ok' to install it, or 'Cancel' to abort.'
>         II. If auto-update flag is true:
> 
>         <Check auto-update 'notify' option>
> 
>             A. If notify is true: re-use above notification 'We have found a
> newer version of Mozilla $brandname, please click 'Ok' to install it, or
> 'Cancel' to abort.'
>             B. If notify is false: Silently proceed to update
> 
> ~ (Congatulations! You are so cool you get to try to update!) ~
Please just state use current logic to shorten this mess

> 4a is where people are going to get annoyed and turn off auto-update
> eventually. ..or bug their sysadmin till the sysadmin switches it off. ^^ This
> is where we could consider throwing in a 'Check if the OS allows me to ask for
> the admin / root password' option. Bob Vickers had an idea to add a 'Enter
> password and proceed' type button. Perhaps have the button appear on OS'es that
> allow it?
Please JUST state the logic to be used.
>snip... there is just way too much to comment on

> Um.. I assume we're not correctly checking for write access in OSX, and what
> I've outlined would correct the notification issues? Is this correct by
> design?? SHOULD the OSX installation block other admins from updating? I'm not
> aware of how or if OSX manages single / multi user installs.
We are correctly checking for write access in OS X. The problem is how OS X sets permissions on install.

>...
> Ok!! I think that's everything wrapped up in the smallest nut shell possible!
I strongly believe it could have been a much smaller nutshell... if you doubt this or find it insulting I apologixe. Take a look at other bugs and the comments made to get an idea how other bugs are handled. After doing that try to use that as a guideline for commenting.

> If I were running the show, and had to stick with the current structure, I
> would make it a hard goal to at least solve all the seperate problems / bugs
> before the next full version. If I had full latitude, I would examine the core
> structure and ensure that the checks are being performed in the correct logical
> order to prevent excessive looping and cut down on extraneous code (not to
> mention possible exploit holes).
This is an opensource project and you do have full latitude to do this. As is it appears to me that you haven't looked at the code which should be the next thing you do before going any further.

> Then, once all the patches for the next full
> version are in place, patch all previous versions to the extent possible... or
> should I say feasible? Finally it would be irresponsible not to hold an
> official 'Period of mourning' or 'Moment of Silence' for those users who have
> been abandoned. Be certain that they are out there. Those chinese hackers
> pushing patched exploits are counting on it.
> 
> Hmm, that's all I can think of at the moment. I'm a bit burnt out now, so I'll
> take a break, then spend another hour reviewing it, then post it and check on
> any feed-back in the morning when I can get back to it. Oh! Please do post back
> ANY feedback, even if it's only partial. I want to try to ensure no one gets
> 'left behind' from now on. :)
Instead of burning yourself out with extremely long comments you could always work on trying to fix this instead. :)

> Footnote: Should I perhaps submit this as a 'master' bug report for all of the
> potential sub-bugs?
Why? Bug should be clear and concise in regards to what actually needs to be fixed.
LOTS of good stuff Robert, thanks so much!! Please hold off on commenting till I reply to both of yours :)

(In reply to comment #41)
> If the dupe bugs have additional requests that can /
> should be fixed as part of the bug then they are done at the same time or they
> are unduped and fixed separately.

Well that mostly makes sense. I suppose if you are working on a certain chunk of code, and the other problems are in or near the same chunk of code. For something like this though, it could get quite confusing quite quick. For administrative purposes, I would think it would be important to split out the separate bugs as soon as possible. In this example, I think some sort of reference point to track all the issues would be beneficial, to say the least.

> I haven't gone through all of the comments you referenced but at least one of
> them refers to applying updates which is bug # 318855 and is separate from this
> bug.

Excellent! This is exactly the sort of thing which will help me see the full picture of what has been fixed and what hasn't!

> As far as this bug goes what needs to be done is stated in comment #0 and I
> reiterated in comment #37. The logic flow you have stated could to say the
> least be stated much more simply... if you want the logic flow of what is to be
> done to be reviewed just post a comment like:
> ---------
> Can I get comments on how I think the logic flow should be

> 1) Check for updates (btw: vendor is irrelevant... it is just a string that is
> available across all apps)
> 1a) if user has privileges to update continue as it works today
> 1b) if user does not have privileges show new ui
> etc.
> ---------

This bit CAN be discussed later, right now we (well, I) need to track down all the seperate problems and ensure they have valid bugs which are fixed. I know this is how the current model would solve the problem, but I think this is where the program logic has 'jumped off the tracks'. As one bug patch gets piled on top of another, some of which are / were probably better definde as feature enhancements, the checks are willy-nilly in an order that makes no sense and is breeding the need for more patches. Like I said earlier though, it is POSSIBLE we could keep the current model and bang it back into shape. :)

> (In reply to comment #38)
> >...
> > Problem # 1 The actual update logic. Here's my take on what SHOULD be
> > happening.
> > 
> > ~ (NOTE: user clicked on 'Check for updates' OR auto-update initialized.) ~
> > 
> > 1) NEVER gray out 'Check for updates'. This would be the first thing to
> > re-code.
> That's a given...

Here's one place I need some guidance. Has this been repaired in another bug? It doesn't seem to be in the scope of this bug.

> 
> > <Check vendor>
> The string for vendor is just an available UI string across all apps and
> doesn't need to be stated in the logic flow.

re: $vendor and notification text.

As you can tell (I'm sure) I haven't even LOOKED at the code yet. ..at least not for a few years, anyway. First I want to sort out the various problems I see, figure out what is fixed, then proceed in hunting down the rest of the problems (ie - checking the actual code) Perhaps we have a communication gap, though. I'll use OpenSUSE as an example.. no, SUSE, SUSE re-distributes Mozilla products in their online repositories. what string would SUSE set to indicate THEY distributed it and NOT Mozilla? That's the string we need to check. That needs to be set in Mozilla products, as it will NOT always be the OS manufacturer who is distributing it. Perhaps I should have used the example $distributor instead :)

As far as the notification text goes, I don't much care WHAT the wording is. I started putting that together after I referenced your list of strings. Then it just got bigger and bigger, but I didn't fall back to your strings. Whatever they or the other current ones say is fine by me.. so long as they're getting the same point across.

> > 2) Then respond with:
> >     a. 'This version of $brandname was not distributed by Mozilla. Please
> > contact $vendor to obtain current version information.'
> >     b. Proceed to check for update
> Why limit it to only Mozilla? The same logic will work with all distributions
> that provide toolkit's app update

Sorry, apparently that's where we need to say $vendor or something to reference 'Mozilla'?

> > <Check for Update>
> > 
> > 3) Depending on the availability of an update:
> >     a. 'We could not contact the update server.' blah blah blah..
> >     b. No update from Mozilla:
> > 
> >     <Check if automatic update>
> > 
> >         I. If auto-update flag is false: 'Congratulations, you have the most
> > current version of Mozilla $brandname.'
> >         II. If auto-update flag is true: Gracefully exit.
> This seems as if you are checking automatic updates when there is no update
> available... that makes no sense and the logic currently in use shouldn't need
> to change anyways so don't bother calling it out or just state use current
> logic.

I see the confusion. We're checking if we're doing an automatic update in order to differentiate from a silent (graceful) exit, or notifying the user who clicked 'Check for Updates' that he's good to go. As I said, most of this logic stuff is probably crammed together in the code in a non-linear fashion. No biggie. 

> >     c. There is an update from Mozilla: proceed to check for write access
> > 
> > <Check for write access>
> > 
> > 4) This bit may be tricky, and will likely involve 2-3 different checks in each
> > OS at a minimum, to acheive a 'yes or no' answer.
> For most OS's we only need to check one location and we already have checks for
> all locations.

Well that's good to know. The OSX permissions on install was the only thing that would have led me to think otherwise. So far.. I assume this is a reported bug somewhere, which has been written off as an OSX issue? Was there a similar issue in one of the Vistas? Without referencing I seem to recall you mentioning somewhere that some repair fubarred some other repair, but you fixed it. Perhaps this is where it would still be a good idea to break out ALL the problems in one place, and referrence all the bugs from a master list.

> > <Check for write access in all necessary locations on all os's>
> > 
> >     a. 'We have found a newer version of Mozilla $brandname, however, you do
> > not have permission to update Mozilla $brandname. Please log in with an
> > Administrative account to update.'
> The UI and text for the ui was already created / approved in bug 318855
> 
> >     b. If we have write access:
> > 
> >     <Check if auto-update>
> > 
> >         I. If auto-update flag is false: 'We have found a newer version of
> > Mozilla $brandname, please click 'Ok' to install it, or 'Cancel' to abort.'
> >         II. If auto-update flag is true:
> > 
> >         <Check auto-update 'notify' option>
> > 
> >             A. If notify is true: re-use above notification 'We have found a
> > newer version of Mozilla $brandname, please click 'Ok' to install it, or
> > 'Cancel' to abort.'
> >             B. If notify is false: Silently proceed to update
> > 
> > ~ (Congatulations! You are so cool you get to try to update!) ~
> Please just state use current logic to shorten this mess

Um.. I myself don't know WHAT the current logic actually does. My outline is strictly as an example to try to track down problem areas. If the functionality is as outlined above, or rather it all exists somewhere, then we're good to go! :)

> > 4a is where people are going to get annoyed and turn off auto-update
> > eventually. ..or bug their sysadmin till the sysadmin switches it off. ^^ This
> > is where we could consider throwing in a 'Check if the OS allows me to ask for
> > the admin / root password' option. Bob Vickers had an idea to add a 'Enter
> > password and proceed' type button. Perhaps have the button appear on OS'es that
> > allow it?
> Please JUST state the logic to be used.

Ok.. this was meant as more of an example set of logic, but I'll give it a shot...

replace 4a with:


     a. We don't have full write access, why?

     <Check if multi - single user install>

          I. It's a single user install without permission to install

          <Check if OS allows 'install as Admin'>

               A. 'We have found a newer version of Mozilla $brandname, however, you do not have permission to update Mozilla $brandname. Please click the button to continue to install using Administrative permissions.'  -->

               <Call the subroutine for the 'Install as Admin' button.>

               B. 'We have found a newer version of Mozilla $brandname, however, you do not have permission to update Mozilla $brandname. You will need to grant the current user Administrative permission before continuing the installation.'

          II. Multi-user install. 'We have found a newer version of Mozilla $brandname, however, you do not have permission to update Mozilla $brandname. You should log in to the Administrative account from which Mozilla $brandname was installed in order to continue the installation.

How's that look? Note we will only be in this branch if we DON'T have write access. Also note that I left the 'Mozilla' wording intact because I don't want to further confuse things by using an improper '$vendor string as an example. :)
(In reply to comment #42)

> > Ok!! I think that's everything wrapped up in the smallest nut shell possible!
> I strongly believe it could have been a much smaller nutshell... if you doubt
> this or find it insulting I apologixe. Take a look at other bugs and the
> comments made to get an idea how other bugs are handled. After doing that try
> to use that as a guideline for commenting.

Certainly I agree!! You have a much broader knowledge of what has been repaired and what hasn't!! Further, you could probably create an accurate ouline of the current logic flow of the various update sub-routines, then make something similar to mine and say 'this would make more sense'. I have the ability to do neither, because I haven't dug into the code yet.. and I'm not sure ANYONE has tracked down the status of all these seperate little update / install issues as of yet. :)

> > If I were running the show, and had to stick with the current structure, I
> > would make it a hard goal to at least solve all the seperate problems / bugs
> > before the next full version. If I had full latitude, I would examine the core
> > structure and ensure that the checks are being performed in the correct logical
> > order to prevent excessive looping and cut down on extraneous code (not to
> > mention possible exploit holes).
> This is an opensource project and you do have full latitude to do this. As is
> it appears to me that you haven't looked at the code which should be the next
> thing you do before going any further.

Excellent!! Your help is invaluable if I'm going to do this, let me know if you would prefer I move my inquiry to a seperate 'bug' for sanity sake. :)

> > Then, once all the patches for the next full
> > version are in place, patch all previous versions to the extent possible... or
> > should I say feasible? Finally it would be irresponsible not to hold an
> > official 'Period of mourning' or 'Moment of Silence' for those users who have
> > been abandoned. Be certain that they are out there. Those chinese hackers
> > pushing patched exploits are counting on it.
> > 
> > Hmm, that's all I can think of at the moment. I'm a bit burnt out now, so I'll
> > take a break, then spend another hour reviewing it, then post it and check on
> > any feed-back in the morning when I can get back to it. Oh! Please do post back
> > ANY feedback, even if it's only partial. I want to try to ensure no one gets
> > 'left behind' from now on. :)
> Instead of burning yourself out with extremely long comments you could always
> work on trying to fix this instead. :)

Don't worry, I'm going to go full circle, then come back around and help with this one once I've got a clear picture of all the seperate issues, and once I've got my hooks into the code to see what is actually going on. :)

> > Footnote: Should I perhaps submit this as a 'master' bug report for all of the
> > potential sub-bugs?
> Why? Bug should be clear and concise in regards to what actually needs to be
> fixed.

Mainly so I wouldn't annoy you with extraneous stuff that doesn't 'specifically pertain' to this particular bug report. If you're fine with it, it's fine with me.

Thanks so much!! I'll start looking into those other bugs you mentioned.. I'll need to put together a master list somehow, can't imagine trying to keep it here and posting an update every time there's new info to add. ..is there a toolkit dev site where it would be proper to post a sort of worksheet? If not I suppose I could post one on my domain somewhere, but it wouldn't be the best solution for keeping track of this on a permanent basis, I'm sure.

..it occurs to me to ask about the people who originally developed the install and update subroutines. Is this 15 year old code, or fresher code that someone may have an interest in having input concerning anything getting re-worked?
Robert, I admire your patience. Let's hope this guy Crogon is just a newbie developer and not a plain old troll.

Crogon: it might help if you could "build your own" Firefox (though I think it is not 100% necessary in order to fix the code). However, be warned that Gecko 1.9.1 / Firefox 3.1 /Thunderbird 3.1 / SeaMonkey 2.0 / Sunbird 1.0 (and later, if any, in all cases) use Mercurial (not CVS anymore) as their version control system. (For Tb/Sm/Sb there is even a second repository, and both must be used to build these apps, so I guess starting with Firefox would be easier.) Maybe you already know how to "build your own" Firefox using Mercurial, but if you don't, I guess https://developer.mozilla.org/En/Mercurial would be a good place to start reading.

Once you think you have a code fix, or maybe a first try at one, you can post it here as attachment, in the form of a unified diff, and ask for review on it (I'm not sure how to go about fiding the "right" reviewer but I suppose Robert would help). I suppose that if you reach that point (or maybe even before that when --and if-- you seriously decide to tackle the problem at the code level rather than only at the concept level), you will be assigned the bug...

For discussing concepts rather than code fixes, one of the mozilla.dev.apps.* newsgroups (on the news.mozilla.org server; in this case probably m.d.a.firefox) might be a better place for the discussion. These NGs are also available as mailing lists and Google Groups but I don't know the details.
btw: the reviewers list for toolkit is available here:
http://www.mozilla.org/projects/toolkit/review.html

The toolkit Sub-Module for Application Update on that list is Software Update... I've sent an email to get the list updated
Tony, actually, I'm an OLD school developer.. well, we used to call them programmers. ;) Anyway, I'm willing to get my hands dirty again to help solve this problem. :)

Never even heard of Mercurial, I'll have to dig into it. I am familiar with building my own apps, in Linux anyway, been a few years since I needed to build a Windows app.

I've been trying to wait for T-bird 3 to join some newsgroups. I still haven't started using an email client on this laptop. maybe I'll find some stand-alone newsgroup manager to use in the meantime.

I'll start digging into this now, since I know I'll be touching the code later. At this point though, I think I still need to try to track down all the bugs involved with these particular issues. That's going to be a process in and of itself. I'm currently going through the entire list of bug-reports for Toolkit. I figure if nothing else, it will get me familiar with some of the technical details, background and etc.

Thanks for the link to the reviewer list Robert, I'll be sure to check that out as well. :)

I think the last thing I could use is a bit of advise on what to use to code and compile with. I've got WinXP and Linux machines available, with a grip of other machines sitting on the shelf, so I could install Solaris or something if that would be best. What platform would you recommend for development? Any must have apps? I used to use Borland, but I'm sure my engine is thoroughly out of date. I've been using TextPad to hand code everything it can handle.. which lately has been everything. :) I know there's been a big boom of dev apps in the last year, so I imagine there's a sweet spot, I just don't know what it is. :)

Thanks again for all the help, you guys rock!!
Bah, I had two more quick questions I forgot to add in before I clicked commit. :P

1) In the bugzilla advanced search there's a component named 'Application Update'. Is that Identical to the 'Software Update' on the Review List, or should I expand my search?

2)Full version upgrades for Toolkit.. is there a certain date when the Software Update component was totally replaced? If there was, I could ignore everything I find before that time period, couldn't I?

Thanks again!
Hey Crogon,

Please use irc or the news groups / mail lists for future questions.

answers:
1) answered in comment #46

2) please familiarize yourself with bugzilla... this isn't applicable

regarding compilers... google is your friend... it is well documented.
Hi in case of higher than ordinary activity about this bug in few last days, I get interested in this issue. It seems that this is the main bug in all similar, but in description there is no "Expected Results:" Some people as I am too claim that mozilla should prompted for root password and flag like this: app.update.vistaUserWithUACHasAdminPassword should be the true in default. I don't want to discuss but show my arguments for this behavour:
1. Almost everyone use firefox on PERSONAL computer and people are said to use guest's account, so most of them know root password. On windows there is mechanism for prompt users implemented, on linux there is sudo. I gues that users wchich know root password on Solaris or others as rare systems are in great minority.
2. Users who don't know admin password mainly use FF in Job, School or some other institution where exists secure policy and where admin is responsible for updates so he can set flag off and even turn off auto-updates.
Jan, that is bug 318855, it is understood that is desired, it won't be fixed in this bug, I doubt very highly that it will be fixed in bug 318855 for Firefox 3.1 with the time we have left, and we might be able to fix this bug for Firefox 3.1 as long as someone can find the time to do so.

Further comments in this bug just take up the time I have available to try to fix this bug for Firefox 3.1. If someone else wants to fix it by all means please do so... I am no longer going to respond but I will read comments and the more comments made the less likely I will find time to fix this for Firefox 3.1.
(In reply to comment #15)
> Ok, stop the bus. In what world is it in the interest of security that ANYONE
> is not informed of a security UPDATE?? You've got the cart in front of the
> horse brother, 'Check for Updates' should NEVER be grayed out. Let all users
> check for updates, auto-check for updates, download updates.. even attempt to
> INSTALL updates. THEN have a popup inform them that they need Administrator
> access to install, if necessary.

From Tom:

Yes, I agree that users should be notified if there is an update. That said, concerns have been raised about a possible flurry of trouble tickets generated by users in a public library, etc. The problem is easily worked around. I will leave the specifics to those who know what they are.

Win XP; day-to-day surfing done under an unprivileged account for security reasons, but I have access to privileges if I know I need them.

> 
> Hackers are laughing at you. Fix it. Fix it now!
> 
> You're leaving end users with their butts hanging in the wind because they have
> no idea that they're not up to date. This should be a priority one bug, unless
> your core is already busted. Which it's not.
> 
> The bug is that 'users are not notified'. Fix THAT, for everyone. Stop argueing
> about situational programming, and fix it across the board so EVERYONE is
> notified. Let the OS worry about the password prompt if the install needs it.
> It's your (the Firefox team) job to make sure everyone knows they need a
> security update. Once that's done, then you can get cute with notifications and
> what not.
> 
> Peace out.
> 
> p.s. I LOVE firefox, so take this with the compassion that inspired the post,
> ok?
Quoted from comment #7

----------------------------------------------------------
btw: the test we perform that could make it work on Vista x64 is to see if we
have write access to the install directory... otherwise it shouldn't have
worked.
-----------------------------------------------------------------------------
(end quote)
Such users (and I include myself) will have the proper write access AFTER, but not before, the manual step of switching to the privileged account.

Trying to eliminate the manual step just won't work.
Win XP
FF 3.0.8 with the aid of the grapevine.
Hope this helps.
Tom
blocking2.0: --- → ?
Flags: blocking1.9.2?
Dan, this is marked sg:want P1 but without context. As with bug 318855 I can't really declare it to be a Firefox 3.6 blocker this late in the game, but if you could add some motivation we could discuss the timing with Rob and see how he'd prefer to approach this and related problems.
Flags: wanted1.9.2+
Flags: blocking1.9.2?
Flags: blocking1.9.2-
[x] Vote For a message! 

It is out of the question that firefox don`t show that a new version is available!
this IS a SECURITY bug and should be fixed asap! It`s a shame it hasn`t for such a long time!
Target Milestone: mozilla1.9.1 → ---
Attached patch patch rev1 (obsolete) — Splinter Review
Uses the strings I added in bug 318855 (I really meant to add them in this bug).
Attachment #413201 - Flags: review?(dtownsend)
Comment on attachment 413201 [details] [diff] [review]
patch rev1

btw: this isn't 100% as of yet... still need to tweak a couple of things but it is close
Comment on attachment 413201 [details] [diff] [review]
patch rev1

>-pref("app.update.url.manual", "http://%LOCALE%.www.mozilla.com/%LOCALE%/%APP%/");
>+pref("app.update.url.manual", "http://www.getfirefox.com");

My understanding is that marketing has actually been trying to move away from getfirefox.com lately and just use firefox.com instead.
Attached patch patch rev2 (obsolete) — Splinter Review
Thanks reed!
Attachment #413201 - Attachment is obsolete: true
Attachment #413205 - Flags: review?(dtownsend)
Attachment #413201 - Flags: review?(dtownsend)
Whiteboard: [sg:want p1] → [sg:want P1]
Comment on attachment 413205 [details] [diff] [review]
patch rev2

Might be a good thing to get a second pair of eyes on this for review.
Attachment #413205 - Flags: review?(vladimir)
Comment on attachment 413205 [details] [diff] [review]
patch rev2

Looks fine to me, pretty straightforward.
Attachment #413205 - Flags: review?(vladimir) → review+
I updated the idl as follows

/**
 * A temporary interface to allow adding new methods without changing existing
 * interfaces for Gecko 1.9.2. After the 1.9.2 release this interface will be
 * removed.
 */
[scriptable, uuid(e22e4bf1-b18c-40cd-a2be-2d565723d056)]
interface nsIApplicationUpdateService2 : nsIApplicationUpdateService
{
  /**
   * Whether or not the Update Service can check for updates. This is a function
   * of whether or not the current user is not prohibited from checking and the
   * application has met all of the prerequisites to check for updates.
   */
  readonly attribute boolean canCheckForUpdates;

  /**
   * Whether or not the Update Service can download and install updates.
   * This is a function of whether or not the current user has access
   * privileges to the install directory.
   */
  readonly attribute boolean canApplyUpdates;
};
Carrying forward vlad's r+
Attachment #413205 - Attachment is obsolete: true
Attachment #413227 - Flags: review+
Attachment #413205 - Flags: review?(dtownsend)
Attachment #413227 - Flags: review?(dtownsend)
Comment on attachment 413227 [details] [diff] [review]
patch rev3 - fixed some comments and logging

Looks fine barring a couple of minor nits and issues. I think the IDL comment needs rewording a bit though.

>diff --git a/browser/base/content/utilityOverlay.js b/browser/base/content/utilityOverlay.js
>--- a/browser/base/content/utilityOverlay.js
>+++ b/browser/base/content/utilityOverlay.js
>@@ -470,27 +470,28 @@ function buildHelpMenu()
>   // Enable/disable the "Report Web Forgery" menu item.  safebrowsing object
>   // may not exist in OSX
>   if (typeof safebrowsing != "undefined")
>     safebrowsing.setReportPhishingMenu();
> 
> #ifdef MOZ_UPDATER
>   var updates = 
>       Components.classes["@mozilla.org/updates/update-service;1"].
>-      getService(Components.interfaces.nsIApplicationUpdateService);
>+      getService(Components.interfaces.nsIApplicationUpdateService).
>+      QueryInterface(Components.interfaces.nsIApplicationUpdateService2);

Since nsIApplicationUpdateService2 extends nsIApplicationUpdateService you should just be able to put the former in the getService call and do without the additional QueryInterface. The same is true elsewhere in the patch.

>diff --git a/toolkit/mozapps/update/content/updates.js b/toolkit/mozapps/update/content/updates.js
>--- a/toolkit/mozapps/update/content/updates.js
>+++ b/toolkit/mozapps/update/content/updates.js
>@@ -620,16 +620,23 @@ var gIncompatibleCheckPage = {
>    * The progress bar for this page
>    */
>   _pBar: null,
> 
>   /**
>    * Initialize
>    */
>   onPageShow: function() {
>+    var aus = CoC["@mozilla.org/updates/update-service;1"].
>+              getService(CoI.nsIApplicationUpdateService).
>+              QueryInterface(CoI.nsIApplicationUpdateService2);
>+    // Display the manual update page if the user is unable to apply the update
>+    if (!aus.canApplyUpdates) {
>+        gUpdates.wiz.currentPage.setAttribute("next", "manualUpdate");
>+    }

Nit: No braces around the single statement.

>diff --git a/toolkit/mozapps/update/public/nsIUpdateService.idl b/toolkit/mozapps/update/public/nsIUpdateService.idl
>--- a/toolkit/mozapps/update/public/nsIUpdateService.idl
>+++ b/toolkit/mozapps/update/public/nsIUpdateService.idl
>@@ -399,16 +399,39 @@ interface nsIApplicationUpdateService : 
>    * Whether or not the Update Service can download and install updates.
>    * This is a function of whether or not the current user has access
>    * privileges to the install directory.
>    */
>   readonly attribute boolean canUpdate;
> };
> 
> /**
>+ * A temporary interface to allow adding new methods without changing existing
>+ * interfaces for Gecko 1.9.2. After the 1.9.2 release this interface will be
>+ * removed.
>+ */
>+[scriptable, uuid(e22e4bf1-b18c-40cd-a2be-2d565723d056)]
>+interface nsIApplicationUpdateService2 : nsIApplicationUpdateService
>+{
>+  /**
>+   * Whether or not the Update Service can check for updates. This is a function
>+   * of whether or not the current user is not prohibited from checking and the
>+   * application has met all of the prerequisites to check for updates.
>+   */
>+  readonly attribute boolean canCheckForUpdates;

This comment makes very little sense to me. How about something like "This is a function of whether or not application update is disabled by the application and the platform the application is running on."?

>diff --git a/toolkit/mozapps/update/src/nsUpdateService.js.in b/toolkit/mozapps/update/src/nsUpdateService.js.in

>   catch (e) {
>-     LOG("gCanUpdate - unable to update. Exception: " + e);
>+     LOG("gCanApplyUpdates - unable to apply updates. Exception: " + e);
>     // No write privileges to install directory
>     return false;
>   }
> 
>+  LOG("gCanCheckForUpdates - able to apply updates");

This should be gCanApplyUpdates.
Attachment #413227 - Flags: review?(dtownsend) → review-
Attached patch patch rev4 - addresses comments (obsolete) — Splinter Review
Attachment #413227 - Attachment is obsolete: true
Attachment #413253 - Flags: review?(dtownsend)
Fixed all comments and incorrect indentation in the timer manager
Attachment #413253 - Attachment is obsolete: true
Attachment #413253 - Flags: review?(dtownsend)
Comment on attachment 413253 [details] [diff] [review]
patch rev4 - addresses comments

Holding off on review... looks like something is now broken
Attached patch patch rev5 - addresses comments (obsolete) — Splinter Review
I forgot to invalidate fastload. I also changed it to immediately advance since it won't display the incompatible add-ons page currently.
Attachment #413257 - Flags: review?(dtownsend)
The comments in the IDL seem to be the wrong way around now?
bah... I was rushing way too much.
Attachment #413257 - Attachment is obsolete: true
Attachment #413259 - Flags: review?(dtownsend)
Attachment #413257 - Flags: review?(dtownsend)
Comment on attachment 413259 [details] [diff] [review]
fixed comments (checked in)

Looks good now
Attachment #413259 - Flags: review?(dtownsend) → review+
Comment on attachment 413260 [details]
screenshot approved by beltzner in bug 318855

Note: this uses the unofficial branding pref due to it being my build and it will be http://www.firefox.com on official builds
Comment on attachment 413259 [details] [diff] [review]
fixed comments (checked in)

Carrying forward Vlad's r+ and requesting 1.9.2
Attachment #413259 - Flags: review+
Attachment #413259 - Flags: approval1.9.2?
Comment on attachment 413259 [details] [diff] [review]
fixed comments (checked in)

[sg:want p1] + wanted1.9.2 = a+
Attachment #413259 - Flags: approval1.9.2? → approval1.9.2+
Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/d8552c8f795f

I'm going to let this bake until tomorrow morning
Status: ASSIGNED → RESOLVED
Closed: 17 years ago15 years ago
Resolution: --- → FIXED
Hardware: x86 → All
Just chatting with rs, and realized that this would pop the notification every 24 hours until the update gets applied. Since many users encountering this dialog may not be on machines that they control, or may not have extremely responsive or co-operative IT departments, I sense that we may end up annoying people with a daily prompt that they can't really do anything about.

Instead, since we issue these updates every month or so anyway, I think that the "OK" button should dismiss the dialog and set the appropriate preference so that it's not offered again until the next update is available. Of course, it will also be seen if a user manually checks for updates.

Rob said he'd be able to modify the patch; it has a192=beltzner, implicitly.
Attachment #413346 - Flags: review?(dtownsend)
I am someone who is responsible for keeping Firefox up-to-date for several hundred users and my view is that if I am not doing my job properly then it is a good thing that users are told about it because then they will badger me to apply the update so that they can continue browsing safely.

So I would rather the pop-up continued to come up every day.
The move of the check just makes the code a tad easier to understand.
Attachment #413346 - Attachment is obsolete: true
Attachment #413366 - Flags: review?(dtownsend)
Attachment #413346 - Flags: review?(dtownsend)
Attachment #413366 - Attachment is obsolete: true
Attachment #413387 - Flags: review?(dtownsend)
Attachment #413366 - Flags: review?(dtownsend)
Attachment #413387 - Flags: review?(dtownsend) → review+
Comment on attachment 413387 [details] [diff] [review]
slightly cleaner (checked in)

Pushed the followup to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/904fd580c972
Just caught the pref panel needs a change to allow configuration... hopefully in time.
Attachment #413449 - Flags: review?(dtownsend)
Attachment #413449 - Flags: review+
Attachment #413449 - Flags: review?(dtownsend)
Attachment #413259 - Attachment description: fixed comments → fixed comments (checked in)
Attachment #413387 - Attachment description: slightly cleaner → slightly cleaner (checked in)
Attachment #413449 - Attachment description: pref panel followup → pref panel followup (checked in)
(In reply to comment #84)
> I am someone who is responsible for keeping Firefox up-to-date for several
> hundred users and my view is that if I am not doing my job properly then it is
> a good thing that users are told about it because then they will badger me to
> apply the update so that they can continue browsing safely.
> 
> So I would rather the pop-up continued to come up every day.

Does that mean you have somehow found a way to update that many Firefoxes at once?  I'm in the same situation, but I materially don't _have_ the time to update my 160 clients every time.  I simply can't.  I tried, I failed.  It's not possible.

This bug being fixed means that every update, _every_ _update_, every single user I have will phone me saying that Firefox broke.  And I'll say to every single user that it's not broke, and they have to click on "OK" (because despite being the only thing they _can_ do, they won't do it without my say-so, for fear of breaking something).  This will, of course, happen all at the same time, because every Firefox will be notified at the same time.  Basically, every time I'll be finished with a user, the phone will ring with the next one.

Now what I'd need would be a anti-virus-like update agent, with a central console to check which client needs to be updated, and the ability to update it remotely if the agent doesn't do it automatically.  I don't and can't have fancy things like domain controllers and Active Directory and those kind of things, so a Firefox-specific solution is the only way to go for me.
(In reply to comment #84) <snip>
and
(In reply to comment #91) <snip>
Bod and Nicolas, I am gathering info on how Firefox is deployed in business environments along with what abilities admins would like to have for managing their deployments inn the hope of being to provide these things in the future. If either of you (as well as any other admins reading this) could write out their wish lists and send them to me at rstrong at mozilla dot com I'd appreciate it. Thanks
blocking2.0: ? → ---
Comment on attachment 415508 [details] [diff] [review]
1.9.1 branch patch rev1

Dave, can I get you to do a once over on this? Thanks
Attachment #415508 - Attachment description: 1.9.1 branch patch in progress → 1.9.1 branch patch rev1
Attachment #415508 - Flags: review?(dtownsend)
Attachment #415508 - Flags: review?(dtownsend) → review+
Comment on attachment 415508 [details] [diff] [review]
1.9.1 branch patch rev1

note: all toolkit apps will also need patches for 1.9.1.7 in the dependent bugs.
Attachment #415508 - Flags: approval1.9.1.7?
Axel: Heads up that we're landing this 1.9.1. These strings already exist in 1.9.1, but were unused and will now be used.
Comment on attachment 415508 [details] [diff] [review]
1.9.1 branch patch rev1

Better now than later!

Approved for 1.9.1.7. a=ss
Attachment #415508 - Flags: approval1.9.1.7? → approval1.9.1.7+
Refresh my memory, is this the same bug that broke updates for the french 3.6 b4 or 5?

If so, we should verify that the 1.9.1 strings are OK and l10n-changesets have the required changes, too, before landing.
(In reply to comment #99)
> Refresh my memory, is this the same bug that broke updates for the french 3.6
> b4 or 5?
Yes it is.
Not sure when I can get to verify l10n for this, which I think we should be doing before landing the en-US patch.
Can I or someone else help with the verification? If so, what's the process for verification?
Axel, would diffing the tips of 1.9.1 / 1.9.2 updates.dtd to see if the three strings are identical be enough?
We need to check both the head of 1.9.1 as well as the release changesets. I do have a host of opt-ins that got lagged during work week, and the 1.9.2 firefox and fennec opt-ins go first.

Without the updates, french will be broken, I know that much.
For the three strings in 1.9.1 would it be adequate to check head of 1.9.1 to the head of 1.9.2 for both existence and them being identical? Seems that if they are the same then it should be good to go for 1.9.1 and I can knock this out in about an hour.

French was fixed on 1.9.1
http://hg.mozilla.org/releases/l10n-mozilla-1.9.1/fr/rev/b42de38cbe10
Hey Axel, I'm willing to do the work to verify these strings in all locales... I've verified through the c's that the strings in 3.5 are the same as the strings in 3.6 so far. I would like confirmation from you as to what would be acceptable to you as verification before going further with verification of these strings. Thanks
Whiteboard: [sg:want P1] → [sg:want P1][needs 1.9.1 landing]
Whiteboard: [sg:want P1][needs 1.9.1 landing] → [sg:want P1][needs 1.9.1 landing][needs input from Axel]
Comment on attachment 415508 [details] [diff] [review]
1.9.1 branch patch rev1

removing 1.9.1.8 approval since we're out of time; try again next time.
Attachment #415508 - Flags: approval1.9.1.9?
Attachment #415508 - Flags: approval1.9.1.8-
Attachment #415508 - Flags: approval1.9.1.8+
Depends on: 546922
Comment on attachment 415508 [details] [diff] [review]
1.9.1 branch patch rev1

Clearing 1.9.1.9 approval request because it looks like we have a regression (bug 546922) to deal with first. Re-request approval when you're ready to land (if you still want to land on the older branches)
Attachment #415508 - Flags: approval1.9.1.9?
So, it looks like some sysadmins may have created incorrectly formatted locked pref files (see bug 546922 comment #4 and 5). I leave it to drivers to decide whether or not this case is worth holding back the patch in this bug from landing on a stable branch.
If we can't confirm bug 546922 we should probably resolve it as WFM and get this patch renominated for approval, yes.
No longer depends on: 546922
Bug 546922 was invalid due to an improperly formatted locked pref file so this bug is waiting on input from Axel.
Comment on attachment 415508 [details] [diff] [review]
1.9.1 branch patch rev1

a1919=beltzner
Attachment #415508 - Flags: approval1.9.1.9+
Whiteboard: [sg:want P1][needs 1.9.1 landing][needs input from Axel] → [sg:want P1][needs answer to c106 from Axel before 1.9.1 landing]
Sorry for the lag here.

I've created a test script and I think we should be fine for 1.9.1. I ran this against the l10n-changesets of 3.5.8.

Rob, if you have runtime tests, that'd be even better, probably to be run against the langpacks for 3.5.8.
Axel, not sure exactly what you mean by runtime tests? I just recently added mochitest chrome tests on trunk and will likely backport them so if mochitest chrome tests were run on l10n then it would catch failures such as the one with french due to the &nbsp: being in the string.
Nice. Only caveat is, we're not running mochitest on localized builds so someone has to do it. We could ask QA to do it on the candidate builds, though.
Whiteboard: [sg:want P1][needs answer to c106 from Axel before 1.9.1 landing] → [sg:want P1]
It will likely e much faster to just run against the language packs and I'll try to do so over the weekend.
I just talked to juan on our l10n drivers call, feel free to ask QA for help on the testing here.
Comment on attachment 415508 [details] [diff] [review]
1.9.1 branch patch rev1

This missed Firefox 3.5.9, renominating for 3.5.10
Attachment #415508 - Flags: approval1.9.1.9-
Attachment #415508 - Flags: approval1.9.1.9+
Attachment #415508 - Flags: approval1.9.1.10?
works here
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; fr; rv:1.9.2) Gecko/20100115 Firefox/3.6

detects 3.6.2 and notify it is impossible to update.

Thanks a lot for this fix.
(In reply to comment #119)
> detects 3.6.2 and notify it is impossible to update.

Thanks for your feedback. I will mark it as verified fixed on 1.9.2.
Keywords: verified1.9.2
Target Milestone: --- → mozilla1.9.3a1
Al, do we have to update some of the Litmus tests?
Flags: in-testsuite?
Flags: in-litmus?
minusing in-testsuite. There is currently no way for the testsuite to run unprivileged afaik but I have been trying to come up with a way to add a test for this. The ui already has a mochitest chrome test.
Flags: in-testsuite? → in-testsuite-
Comment on attachment 415508 [details] [diff] [review]
1.9.1 branch patch rev1

a=beltzner for 1.9.1.10
Attachment #415508 - Flags: approval1.9.1.10? → approval1.9.1.10+
Sadly, the 1.9.1 backport has the canUpdate getter backwards of what is in 1.9.2:
1.9.1:
get canUpdate() {
  return gCanApplyUpdates && gCanCheckForUpdates;
}

1.9.2:
get canUpdate() {
  return gCanCheckForUpdates && gCanApplyUpdates;
}

The sad result is that while bug 538933 is fixed by the 1.9.2 version above, it isn't by the 1.9.1 backport.
:rs - can you respond to comment 125 and set the status1.9.1 flag back to .10-fixed if you disagree. If not, we'll need another backport here.
beltzner, sure... what Mike Hommey brought up is the creation of the updates directory with a distribution of Firefox that is built with --disable-updater. Optimally, the directory wouldn't be created when built with --disable-updater and it isn't as of Firefox 3.6 due to other work I did for Firefox 3.6. We could land the patch Mike submitted in bug 538933 to make this nicer for Firefox 3.5.x distributions if you think this is important to do.
p.s. - forgot to mention that was the case since the app update component was first created and I made it so it no longer occurred as of Firefox 3.6... in other words it isn't a regression but it would be a nice to have for those Firefox 3.5.x users running custom distributions of Firefox.
(In reply to comment #127)
> beltzner, sure... what Mike Hommey brought up is the creation of the updates
> directory with a distribution of Firefox that is built with --disable-updater.
> Optimally, the directory wouldn't be created when built with --disable-updater

Not only with --disable-updater, but with --enable-updater and app.update.enabled = false.

> and it isn't as of Firefox 3.6 due to other work I did for Firefox 3.6. We
> could land the patch Mike submitted in bug 538933 to make this nicer for
> Firefox 3.5.x distributions if you think this is important to do.

Note the patch is now only what is in comment 125. I agree that this is not a regression per se, but while the 3.6 work has just basically landed on 3.5, it's sad that the backport doidn't keep the original fix for that (especially considering it's only about gCanApplyUpdates && gCanCheckForUpdates vs gCanCheckForUpdates && gCanApplyUpdates). I don't really care if you fix that bug on the branch or not.
Agreed that it is sad... I didn't consider that this patch could fix bug 538933 and in relation to the creation of the updates directory I was more concerned with the ability for distros to disable app update in its entirety which was done in bug 471219. I don't have a problem with bug 538933 being fixed / landed for a Firefox 3.5.x but I don't really care much one way or the other either since it likely affects a small number of users and the affect is IMO very minor.
Marking this as fixed in .10. We won't take the additional change on the branch. As far as I know the linux distros are generally updating to the 1.9.2 branch with the next release.
I believe there's a small bug in the update process still, at least for me. When I run the update in the latest nightly it finds the update downloads it reboots prompts for elevation and updates fine. But when I go into about firefox it still has a button saying apply update. I click this button and firefox restarts again but going back into the about firefox again shows the same apply now button.

The only way to fix this I have found is to go into C:\Users\<User>\AppData\Local\Mozilla\Firefox\Minefield and delete the actual update files.

Any ideas?
(In reply to comment #132)
Please file a new bug
You need to log in before you can comment on or make changes to this bug.