513958 - Firefox silently fails to start if %APPDATA% variable is missing

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect, P2)

Description

[Matthew Middleton (:zzxc)]

On Windows, if the %APPDATA% environment variable is missing or set to an invalid path, Firefox fails to start with no error message.  According to users, both IE and Chrome work properly in this condition, but a number of other applications fail.

Firefox silently failing to start is one of the most commonly reported issues on support.mozilla.com.  Several reports from Live Chat, all with Windows Vista, confirmed that %appdata% on the affected account was empty.  The best workaround we've found has been to create a new Windows account and move the data to the new account.  (Individual chat logs can be viewed by people with access to the sumotools server at http://dm-sumotools01.mozilla.org/cgi-bin/zzxc/chatquery.php?tags=wontstart&transcript=appdata  - ask in #sumodev on IRC if you need a copy of a chat log)

We should consider using another mechanism of finding the AppData folder if %APPDATA% is missing, or at least warn users of the problem instead of silently exiting.

Comment 1

[Dave Garrett]

What's the cause of the missing/invalid %appdata% var in these instances?

Comment 2

[Matthew Middleton (:zzxc)]

Attachment: Environment variable dump from an affected user

We weren't able to determine the cause of the missing %appdata%.  Possibilities include poorly behaved software or spyware.  We got a dump of all environment variables (set > file.txt) in one case, which I am attaching.

Comment 3

[Tanner Filip [:tanner]]

It seems to happen in only admin accounts... I've had it fail twice on only admin accounts, on my computer

Comment 4

[Matthew Middleton (:zzxc)]

Users have reported that other browsers are not affected by %APPDATA% missing, including Google Chrome and IE, so this should be fixed in Mozilla.  (Instead of using this environment variable, it should be using a shell API.)

Comment 5

[[:Cww]]

This is the most common cause of Firefox doesn't start (and they don't get crash reporter) for Vista.  I have no idea WHY %APPDATA% is broken for users but as far as the user is concerned, it's only Firefox/Thunderbird that is non-functioning.  The only workaround is to make a new vista user profile and move ALL of your user data and settings.  It's not a good workaround by any means as it can break stuff and ruin permissions.

Comment 6

[Dave Garrett]

(In reply to comment #5)
> The only workaround is to make a new vista user profile and
> move ALL of your user data and settings.  It's not a good workaround by any
> means as it can break stuff and ruin permissions.

Why can't they just set %APPDATA% correctly again?

Personally, I think that if support is getting people with a computers this borked we should be pointing them to instructions on how to backup their data and do a clean reinstall of their OS.

Comment 7

[Tanner Filip [:tanner]]

(In reply to comment #4)
> Users have reported that other browsers are not affected by %APPDATA% missing,
> including Google Chrome and IE, so this should be fixed in Mozilla.  (Instead
> of using this environment variable, it should be using a shell API.)

I agree. A clean install of an OS can be costly, For the past 3-4 months I had to reinstall Fx each time it was updated, or make a new admin account, which the %APPDATA% breaks in, again. May a sys. restore solve this? fwiw, iTunes won't update with %APPDATA% broken, but safari, chrome, etc. do.

Comment 8

[Noah (NoahSUMO/SolidSnake) [:Noah]]

> (In reply to comment #5)
> Personally, I think that if support is getting people with a computers this
> borked we should be pointing them to instructions on how to backup their data
> and do a clean reinstall of their OS.
Dave, I totally disagree with you. First, a lot of users make sacrifices just to have Firefox installed. Say dumping a popular plugin, AV or firewall because it causes conflicts with Firefox.

Now to go to the farfetched extreme of telling a user to reinstall a OS just to use a browser (Firefox) is simply ridiculous. Even if you are helping them fix other problems that may occur in the future. It's not our place to be telling users to reinstall OS'es. Which I believe is more complicated than many of our average users know how to do, or feel comfortable doing in the first place.

Many users don't even know how to backup up data or have the means to do so (no external media, no software to mass backup, no external HD). Who wants to lose all their installed software and personal data just to get one browser up and going again? Not to mention the fact that other browsers are working properly in spite of this.

Has Firefox stopped being the user friendly choice, and became geared towards power users?

Comment 9

[Daniel Veditz [:dveditz]]

Not going to make 1.9.1.4 -- a fix should be landed in 1.9.2 ASAP though so we get testing in time to make the next 1.9.1.x release. Assigning to Johnathan so this doesn't get lost in the "General" component wasteland -- please find the appropriate owner.

Comment 10

[Tanner Filip [:tanner]]

(In reply to comment #8)
> > (In reply to comment #5)
> > Personally, I think that if support is getting people with a computers this
> > borked we should be pointing them to instructions on how to backup their data
> > and do a clean reinstall of their OS.
> Dave, I totally disagree with you. First, a lot of users make sacrifices just
> to have Firefox installed. Say dumping a popular plugin, AV or firewall because
> it causes conflicts with Firefox.
> 
> Now to go to the farfetched extreme of telling a user to reinstall a OS just to
> use a browser (Firefox) is simply ridiculous. Even if you are helping them fix
> other problems that may occur in the future. It's not our place to be telling
> users to reinstall OS'es. Which I believe is more complicated than many of our
> average users know how to do, or feel comfortable doing in the first place.
> 
> Many users don't even know how to backup up data or have the means to do so (no
> external media, no software to mass backup, no external HD). Who wants to lose
> all their installed software and personal data just to get one browser up and
> going again? Not to mention the fact that other browsers are working properly
> in spite of this.
> 
> Has Firefox stopped being the user friendly choice, and became geared towards
> power users?


Yeah; I agree. Installing the OS again is just ridiculous. I can see if its linux, or an OS disk comes with the computer, but as i said earlier, it can be over $100. If i were in this situation, i would go with another browser, and i am in that postion, and i am using another browser. I wouldn't pay a cent to use a browser, much less a browser that is 100% free and open source.

Comment 11

[Tanner Filip [:tanner]]

Daniel,
Ugh, i think i may have accidentally overridden your changes... Sorry.

Comment 12

[Tanner Filip [:tanner]]

(In reply to comment #11)
> Daniel,
> Ugh, i think i may have accidentally overridden your changes... Sorry.

Alright, i got it reassigned, i cant get the 1.9.1 status wanted flag back...

Sorry for all the comments by me ;)

Comment 13

[Dave Garrett]

(In reply to comment #8)
> > (In reply to comment #5)
> > Personally, I think that if support is getting people with a computers this
> > borked we should be pointing them to instructions on how to backup their data
> > and do a clean reinstall of their OS.
> Dave, I totally disagree with you. First, a lot of users make sacrifices just
> to have Firefox installed. Say dumping a popular plugin, AV or firewall because
> it causes conflicts with Firefox.
> 
> Now to go to the farfetched extreme of telling a user to reinstall a OS just to
> use a browser (Firefox) is simply ridiculous. Even if you are helping them fix
> other problems that may occur in the future. It's not our place to be telling
> users to reinstall OS'es. Which I believe is more complicated than many of our
> average users know how to do, or feel comfortable doing in the first place.
> 
> Many users don't even know how to backup up data or have the means to do so (no
> external media, no software to mass backup, no external HD). Who wants to lose
> all their installed software and personal data just to get one browser up and
> going again? Not to mention the fact that other browsers are working properly
> in spite of this. 
> 
> Has Firefox stopped being the user friendly choice, and became geared towards
> power users?

Don't overreact. You act like I said I don't think this bug should be fixed.

All I said is that in such a situation where the person's computer is clearly very broken, then we should be pointing people to the necessary support to fix their computer fully. If people ask for help from us for something this major we shouldn't just be working around the part that affects us, because clearly other things on their system are going to be affected too (i.e. iTunes in comment 7, but undoubtedly more). Yes, it may be complicated and require learning on their part, but if it's the only way to guarantee that their unknown host of problems is fixed then that's just how it is. And no, I'm not suggesting we walk them through it ourselves; just that we point them in the right direction, possibly to their computer vendor's support.

Ideally, someone would investigate here and figure out specifically what is actually wrong with their system and learn how to fix it without more drastic measures. It's probably caused by malware that needs to be identified and excised. In the meantime, though, it sounds to me like current work arounds don't seem to be sufficient.

(In reply to comment #10)
> Yeah; I agree. Installing the OS again is just ridiculous. I can see if its
> linux, or an OS disk comes with the computer, but as i said earlier, it can be
> over $100. If i were in this situation, i would go with another browser, and i
> am in that postion, and i am using another browser. I wouldn't pay a cent to
> use a browser, much less a browser that is 100% free and open source.

Loosing your disk or buying from a bad vendor that doesn't give you one is not what should be expected. (though, it may be more common than it should be) Whatever OS you have, you should have a way to reinstall it in some form, and if you don't that's a problem you really should find a way to fix.

This is all about the support process for when people get this problem and not the bug itself, which is restricted to a clearly needed work around on Mozilla's end. My apologies for somehow sparking a somewhat off-topic discussion here. :/

Comment 14

[Johnathan Nightingale [:johnath]]

Rob - environment variables going haywire or otherwise behaving oddly feels like a problem you've tackled before - do you have thoughts on the best way to approach this?

Comment 15

[Robert Strong (they/them - no direct email)]

Johnathan, I'll take a look.

Comment 16

[Robert Strong (they/them - no direct email)]

I haven't been able to reproduce as of yet. Can I get someone to export the following registry keys and attach them to this bug.

HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Explorer\Shell Folders
and
HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Explorer\User Shell Folders

btw: afaict we don't rely on the env var and do use the appropriate shell api calls and I highly suspect that the reason the shell api call is failing is due to the same reason the env var isn't set.

Comment 17

[[not reading bugmail]]

In Vista, AppData is both a Key and a Folder.

AppData RegKey in Shell Folders

F:\Users\cuz\AppData\Roaming

AppData RegKey in User Shell Folders

%USERPROFILE%\AppData\Roaming

Vista RegKeys coming up next...

Comment 18

[[not reading bugmail]]

Attachment: Vista Shell Folders Reg Keys

Comment 19

[[not reading bugmail]]

Attachment: Vista User Shell Folders Reg Keys

Comment 20

[Robert Strong (they/them - no direct email)]

Just to verify, these registry keys are from a system that has this bug?

Comment 21

[Robert Strong (they/them - no direct email)]

If it is a system that experiences this bug could you also export / attach the following reg key?
HKEY_CURRENT_USER\Volatile Environment

Comment 22

[[not reading bugmail]]

Attachment: Vista Volitile Enviroment RegKeys - POR

Unfortunately I just have a working Vista system for a standard point of reference, so I don't experience this issue.  I'm assuming these keys would be mangled or missing on a system with the bug.

Comment 23

[Robert Strong (they/them - no direct email)]

Comment on attachment 403419 [details]
Vista Volitile Enviroment RegKeys - POR

Thanks but I need the keys from a system that is experiencing this problem.

Comment 24

[Robert Strong (they/them - no direct email)]

In summary, we are coming up on Firefox 3.6 soon so it would be very helpful if anyone that is experiencing this could

Check the value of LOCALAPPDATA in the environment and if present post the value to this bug

Export the following registry keys and attach them to this bug.

HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Explorer\Shell Folders

and

HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Explorer\User Shell Folders

and

HKEY_CURRENT_USER\Volatile Environment

Thanks

Comment 25

[Robert Strong (they/them - no direct email)]

I'd still very much like to get the info / files asked for in comment #24 even though I have been able to simulate this in case my simulation causes a different bug.

What appears to be happening is if the App Data dir is unavailable the api returns the default user's app data directory. Since the user doesn't have write access to this dir we bail. Since this location is readable by other users this isn't a good location to store this data anyway. I'll try to see what chrome, safari, etc. do in this situation.

Comment 26

[Robert Strong (they/them - no direct email)]

The only way I have been able to simulate the absence of APPDATA so far also causes Safari to crash and IE to hang so it isn't good enough for this. I might be able to come up with a reasonable fix for this but it would be much safer if I were able to get the info asked for in comment #24.

Comment 27

[Robert Strong (they/them - no direct email)]

Found another way to simulate this and I have been able to run Firefox without problem on Vista x86 and 64 bit without the APPDATA environment var. I've checked both trunk and 1.9.1 to see if any of the codebase tries to get the appdata directory from the environment instead of calling appropriate shell api's and haven't found any. At this point I need the info requested in comment #24 or some other clue to go any further.

Tanner, would you be able to provide the info asked for in comment #24?

Comment 28

[Tanner Filip [:tanner]]

Attachment: HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Explorer\Shell

Comment 29

[Tanner Filip [:tanner]]

Attachment: HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Explorer\User Shell

Comment 30

[Tanner Filip [:tanner]]

Attachment: HKEY_CURRENT_USER\Volatile Environment

Comment 31

[Tanner Filip [:tanner]]

fwiw, Safari doesn't work when appdata is missing...

Comment 32

[Robert Strong (they/them - no direct email)]

From the reg keys I believe we could try to fallback to reading the APPDATA / LOCALAPPDATA values when this fails. Need to think on this / investigate some more.

Comment 33

[Robert Strong (they/them - no direct email)]

Tanner, could I get you to export the following registry key on this system and attach it to this bug? Thanks
HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows NT\CurrentVersion\WinLogon

Comment 34

[Tanner Filip [:tanner]]

Attachment: HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows NT\CurrentVersion\WinLogon

Comment 35

[Tanner Filip [:tanner]]

Command prompt stuff that rs told me to do...

http://pastebin.mozilla.org/673575

Comment 36

[Robert Strong (they/them - no direct email)]

taking

Comment 37

[Robert Strong (they/them - no direct email)]

Comment on attachment 403628 [details]
HKEY_CURRENT_USER\Volatile Environment

note: there is an HKEY_CURRENT_USER\Volatile Environment\2 registry key which leads me to think that two user sessions are initiated with the second one being the one in use along with it not having APPDATA set properly. Doesn't change the approach I think I'll use to workaround this but it is unusual.

Comment 38

[Robert Strong (they/them - no direct email)]

Attachment: possible workaround patch

Comment 39

[Robert Strong (they/them - no direct email)]

I've submitted a patch to the try server that when that build runs on an affected system we should be able to get a little more info as well as check if the possible workaround patch works. I'll post a link to the build after it completes.

Comment 40

[Robert Strong (they/them - no direct email)]

Tanner, could you run the following build on the affected system.
https://build.mozilla.org/tryserver-builds/[e-mail]-Bug513958/Bug513958-win32.zip

Besides working, it should also display a messagebox with the path to the appdata directory as it is returned by the shell api's. Could you either type that path into this bug or take a screenshot of the messagebox and attach it to this bug? Thanks!

Comment 41

[Robert Strong (they/them - no direct email)]

Attachment: possible workaround patch rev2

The first patch didn't fix it for Tanner. I'm going to send this to the try server and hopefully this patch will.

Comment 42

[Robert Strong (they/them - no direct email)]

Anyone else besides Tanner that experiences this should export the following registry key using regedit.exe
HKEY_CURRENT_USER\Volatile Environment

Of particular interest is if there isn't a HKEY_CURRENT_USER\Volatile Environment\1\ registry key and there is a HKEY_CURRENT_USER\Volatile Environment\2\ registry key. I have found no information as to what would cause this as of yet.

Comment 43

[Robert Strong (they/them - no direct email)]

Tanner or anyone else experiencing this bug, the link below is for a try server build with a possible fix. I'd appreciate it if you could try it out on an affected system with the affected account. Thanks!

https://build.mozilla.org/tryserver-builds/[e-mail]-Bug513958rev2/Bug513958rev2-win32.zip

Comment 44

[Robert Strong (they/them - no direct email)]

btw: the try build might not fix this entirely since I just patched the startup code and the following might also need to be patched.
http://mxr.mozilla.org/mozilla-central/source/xpcom/io/SpecialSystemDirectory.cpp#830

Comment 45

[Robert Strong (they/them - no direct email)]

Tanner, when you try this build if it doesn't start please try running firefox.exe -P from a command line.

Comment 46

[Tanner Filip [:tanner]]

(In reply to comment #43)
> Tanner or anyone else experiencing this bug, the link below is for a try server
> build with a possible fix. I'd appreciate it if you could try it out on an
> affected system with the affected account. Thanks!
> 
> https://build.mozilla.org/tryserver-builds/[e-mail]-Bug513958rev2/Bug513958rev2-win32.zip

Works under admin account, but not the regular build (3.5.2)

Comment 47

[[not reading bugmail]]

Here are some notes on the AppData folder just from an internet search on the subject.

Review MS doc: Managing Roaming User Data Deployment Guide
http://technet.microsoft.com/zh-tw/library/cc766489%28WS.10%29.aspx
The document also has notes on the Vista vs XP folder structure.

The AppData folder is about managing roaming profiles and enabling folder redirection from a server or your own system.  Doing a quick internet search turned up some users reported having changed permissions to see the AppData folder, modifying ownership or turning off/on sync/offline files also affecting roaming profiles and caused profile corruption.  

So users can inadvertently cause problems with the actual system folders, or maybe as suggested its because of malware.

Typing Shell:AppData in Search opens Explorer to the shell roaming profile folder:  
..\AppData\Roaming 

Typing just AppData in Search opens Explorer to 
..\AppData\Local 
..\AppData\Locallow
..\AppData\Roaming

Typing in %AppData% in Search opens Explorer to 
..\AppData\Roaming

There should be a Mozilla folder in AppData\Local and AppData\Roaming 
Though the roaming folder doesn't appear to be a carbon copy of the Local folder.

Comment 48

[Robert Strong (they/them - no direct email)]

Thanks cuz84d... I've spent numerous hours researching this during the week and have read the same or similar info. In this instance the shell api call is actually failing which I haven't seen much info about. btw: typing %LOCALAPPDATA% will open to ..\AppData\Local and there are several others.

Also, the Mozilla directory under AppData\Local contains profile data that a user would typically not want to sync (e.g. roam) such as the cache whereas the Mozilla directory AppData\Roaming contains roaming data that a user would typically want to sync such as bookmarks (e.g. places), etc.

For this bug we can workaround the problem with the OS as has been seen by Tanner and I am working on a patch to provide this fallback behavior for the two other areas (xpcom/io/SpecialSystemDirectory.cpp and crashreporter) that I didn't fix with the patch that Tanner tested.

Comment 49

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Hey Jim, this is a fairly strange bug as bugs go. The APPDATA environment var is only a secondary symptom in that we work fine without that environment var set but whatever is causing us not to startup for some users is either preventing or unsetting APPDATA in the internal structure for storing APPDATA on Windows. I know that prior to Vista the gina was responsible for adding this but it isn't clear to me what is on Vista. I have my suspicions as to the root cause but nothing concrete and whatever the case I think we should have a fallback for when the call to SHGetSpecialFolderPathW and friends fail.

I started going down the path of adding fallbacks for as many as possible in SpecialSystemDirectory.cpp but since they reside all over the place I chose not to at least for now.

Comment 50

[Robert Strong (they/them - no direct email)]

Attachment: patch rev2 - little cleaner

Comment 51

[Robert Strong (they/them - no direct email)]

Comment on attachment 404780 [details] [diff] [review]
patch rev2 - little cleaner

bah I'm tired... there is a mistake in this patch

Comment 52

[Jim Mathies [:jimm]]

Comment on attachment 404780 [details] [diff] [review]
patch rev2 - little cleaner

Hmm, so why are we adding all this code bloat to cover a situation where the user's machine is obviously configured incorrectly? Or rather, how wide spread is the problem, is it bad enough that it dictates us jumping through whoops in our code to address? FWIW, msdn docs explicitly state we should not be doing this type of registry lookup, we should rely on SHGetFolderPath or SHGetKnownFolderPath.

+  if (!SUCCEEDED(res)) {

Lets make this -
if (FAILED(res)) {

+  HRESULT res = SHGetFolderPath(NULL,
+                                CSIDL_APPDATA,
+                                NULL,
+                                0,
+                                path);

Can we go ahead and update these to use SHGFP_TYPE_CURRENT?


+  HRESULT res = SHGetFolderPath(NULL,
+                                CSIDL_APPDATA,
+                                NULL,
+                                0,
+                                path);
+  if (!SUCCEEDED(res)) {
+    HKEY key;
+    res = ::RegOpenKeyExW(HKEY_CURRENT_USER,
+                          L"Software\\Microsoft\\Windows\\Cu...
+                          0,
+                          KEY_READ,
+                          &key);
+    if (!SUCCEEDED(res))
+      return false;
+
+    DWORD type, size;
+    res = RegQueryValueExW(key,
+                           L"AppData",
+                           NULL,
+                           &type,
+                           (LPBYTE)&path,
+                           &size);
+    ::RegCloseKey(key);
+    // The type must be REG_SZ, the buffer size must not equal 0 and the buffer
+    // size be a multiple of 2.
+    if (!SUCCEEDED(res) || type != REG_SZ || size == 0 || size % 2 != 0)
+        return false;
+  }
+
+  if (SUCCEEDED(res)) {

Your mixing up your return value error checks here. For SHGetFolderPath use HRESULT, SUCCEEDED/FAILED. For the reg calls you need a DWORD result with checks like if (dwRes != ERROR_SUCCESS). There are a few of these in the patch.

+    // The type must be REG_SZ, the buffer size must not equal 0 and the buffer
+    // size be a multiple of 2.
+    if (!SUCCEEDED(res) || type != REG_SZ || size == 0 || size % 2 != 0)
+        return false;

My personal entries do not appear to use REG_EXPAND_SZ, but there may be cases where they do. I guess we are targeting a corner case here so ignoring that type would just be a corner of the corner case? :)

Comment 53

[Robert Strong (they/them - no direct email)]

(In reply to comment #52)
> (From update of attachment 404780 [details] [diff] [review])
> Hmm, so why are we adding all this code bloat to cover a situation where the
> user's machine is obviously configured incorrectly? Or rather, how wide spread
> is the problem, is it bad enough that it dictates us jumping through whoops in
> our code to address?
This appears to only affect a single account and not all accounts on a system. It is prolific enough that it is considered a common issue. I think this small amount of bloat as it stands atm is worth it but I am concerned about not being able to reproduce and the possible other areas that might break once this is fixed. At least we should get crashreporter reports after this.

> FWIW, msdn docs explicitly state we should not be doing
> this type of registry lookup, we should rely on SHGetFolderPath or
> SHGetKnownFolderPath.
Agreed wholeheartedly. According to the reports it appears that IE succeeds when in this misconfigured state.

>...
> +    // The type must be REG_SZ, the buffer size must not equal 0 and the
> buffer
> +    // size be a multiple of 2.
> +    if (!SUCCEEDED(res) || type != REG_SZ || size == 0 || size % 2 != 0)
> +        return false;
> 
> My personal entries do not appear to use REG_EXPAND_SZ, but there may be cases
> where they do. I guess we are targeting a corner case here so ignoring that
> type would just be a corner of the corner case? :)
The Shell Folders registry values are created on logon and should always be REG_SZ. Take a look at an account that hasn't logged in after an OS startup.

Comment 54

[Robert Strong (they/them - no direct email)]

Attachment: patch rev3 comments addressed

I'm going to run this through a couple of tests before requesting review.

I'd like to get these calls using SHGFP_TYPE_CURRENT but I don't want to in this bug since there might be migration issues for users that are using Firefox already and have redirected folders... not sure how we are going to fix that and it shouldn't be fixed in this bug.

Comment 55

[Robert Strong (they/them - no direct email)]

Attachment: patch rev4

Comment 56

[Jim Mathies [:jimm]]

(In reply to comment #54)
> Created an attachment (id=404924) [details]
> patch rev3 comments addressed
> 
> I'm going to run this through a couple of tests before requesting review.
> 
> I'd like to get these calls using SHGFP_TYPE_CURRENT but I don't want to in
> this bug since there might be migration issues for users that are using Firefox
> already and have redirected folders... not sure how we are going to fix that
> and it shouldn't be fixed in this bug.

FYI, SHGFP_TYPE_CURRENT = 0, it's what we are passing.

Comment 57

[Robert Strong (they/them - no direct email)]

Comment on attachment 404972 [details] [diff] [review]
patch rev4

Benjamin, I wanted to get your r+ for this workaround for an OS issue. It turns out that IE and Chrome work when in this state and Firefox does not. I suspect the underlying cause is that a user account installed software that is logging on as the user and is causing this bug hence why this only affects one account on the system. This is only a guess based on the behavior and registry entries on an affected system. Also, one person that was experiencing this bug tested out one of the earlier patches and was able to launch Firefox.

Comment 58

[Robert Strong (they/them - no direct email)]

(In reply to comment #56)
>...
> FYI, SHGFP_TYPE_CURRENT = 0, it's what we are passing.
Right your are. I also just checked WinCE and it has it as well. Sorry about that :(

Comment 59

[Benjamin Smedberg]

Comment on attachment 404972 [details] [diff] [review]
patch rev4

>diff --git a/toolkit/xre/nsXREDirProvider.cpp b/toolkit/xre/nsXREDirProvider.cpp

>+#ifndef WINCE
>+static nsresult
>+GetRegWindowsAppDataFolder(PRBool aLocal, nsAString& _retval)
>+{
>+  // This provides a fallback for getting the path to APPDATA or LOCALAPPDATA
>+  // by querying the registry when the call to SHGetSpecialFolderLocation or
>+  // SHGetPathFromIDListW is unable to provide these paths (Bug 513958).

Here and in the other location, please make this comment into a doc comment before the function.

Comment 60

[Robert Strong (they/them - no direct email)]

Attachment: patch updated to comments for checkin

Comment 61

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-central (woohoo greentree!)
http://hg.mozilla.org/mozilla-central/rev/691e2386ee3e

No way I can reproduce or test this that I know of so minusing in-testsuite

Comment 62

[martin.feger]

You're using the RegQueryValueExW API incorrectly, you have to set 'size' to the size of the buffer before calling it. The code currently works if you're lucky and the stack junk that ends up in size is larger than the size of the registry data. You could also overrun the buffer in the unlikely case of extremely long values.

While I have your attention may I also recommend that you should use SHGetValue? It's far more convenient when you just need a single value out of the registry.

Comment 63

[Robert Strong (they/them - no direct email)]

Martin, please correct me if I am wrong
http://msdn.microsoft.com/en-us/library/ms724911%28VS.85%29.aspx

>diff --git a/toolkit/crashreporter/client/crashreporter_win.cpp b/toolkit/crashreporter/client/crashreporter_win.cpp
>--- a/toolkit/crashreporter/client/crashreporter_win.cpp
>+++ b/toolkit/crashreporter/client/crashreporter_win.cpp
>@@ -1338,20 +1338,45 @@ bool UIGetSettingsPath(const string& ven
>                        string& settings_path)
> {
>   wchar_t path[MAX_PATH];
>...
>+    HKEY key;
>+    DWORD type, size, dwRes;
>+    dwRes = ::RegOpenKeyExW(HKEY_CURRENT_USER,
>+                            L"Software\\Microsoft\\Windows\\CurrentVersion\\Explorer\\Shell Folders",
>+                            0,
>+                            KEY_READ,
>+                            &key);
>+    if (dwRes != ERROR_SUCCESS)
>+      return false;
>+
>+    dwRes = RegQueryValueExW(key,
>+                             L"AppData",
>+                             NULL,
>+                             &type,
>+                             (LPBYTE)&path,
>+                             &size);
>+    ::RegCloseKey(key);
>+    // The call to RegQueryValueExW must succeed, the type must be REG_SZ, the
>+    // buffer size must not equal 0, and the buffer size be a multiple of 2.
>+    if (dwRes != ERROR_SUCCESS || type != REG_SZ || size == 0 || size % 2 != 0)
>+        return false;
For this case size will return the size of path in case the value is an empty string. If path is too small dwRes will return ERROR_MORE_DATA.

> bool UIEnsurePathExists(const string& path)
>diff --git a/toolkit/xre/nsXREDirProvider.cpp b/toolkit/xre/nsXREDirProvider.cpp
>--- a/toolkit/xre/nsXREDirProvider.cpp
>+++ b/toolkit/xre/nsXREDirProvider.cpp
>@@ -927,6 +927,60 @@ GetShellFolderPath(int folder, nsAString
>...
>+  DWORD type, size;
>+  res = RegQueryValueExW(key, (aLocal ? L"Local AppData" : L"AppData"), NULL,
>+                         &type, NULL, &size);
>+  // The call to RegQueryValueExW must succeed, the type must be REG_SZ, the
>+  // buffer size must not equal 0, and the buffer size be a multiple of 2.
>+  if (res != ERROR_SUCCESS || type != REG_SZ || size == 0 || size % 2 != 0) {
>+    ::RegCloseKey(key);
>+    _retval.SetLength(0);
>+    return NS_ERROR_NOT_AVAILABLE;
>+  }
For this case we use size to allocate the buffer

>+  // |size| includes room for the terminating null character
>+  DWORD resultLen = size / 2 - 1;
>+
>+  _retval.SetLength(resultLen);
>+  nsAString::iterator begin;
>+  _retval.BeginWriting(begin);
>+  if (begin.size_forward() != resultLen) {
>+    ::RegCloseKey(key);
>+    _retval.SetLength(0);
>+    return NS_ERROR_NOT_AVAILABLE;
>+  }
>+
>+  res = RegQueryValueExW(key, (aLocal ? L"Local AppData" : L"AppData"), NULL,
>+                         NULL, (LPBYTE) begin.get(), &size);
>+  ::RegCloseKey(key);
>+  if (res != ERROR_SUCCESS) {
>+    _retval.SetLength(0);
>+    return NS_ERROR_NOT_AVAILABLE;
>+  }

>diff --git a/xpcom/io/SpecialSystemDirectory.cpp b/xpcom/io/SpecialSystemDirectory.cpp
>--- a/xpcom/io/SpecialSystemDirectory.cpp
>+++ b/xpcom/io/SpecialSystemDirectory.cpp
>@@ -210,6 +210,44 @@ static nsresult GetWindowsFolder(int fol
>...
>+    WCHAR path[MAX_PATH + 2];
>+    DWORD type, size;
>+    res = RegQueryValueExW(key, (aLocal ? L"Local AppData" : L"AppData"), NULL,
>+                           &type, (LPBYTE)&path, &size);
>+    ::RegCloseKey(key);
>+    // The call to RegQueryValueExW must succeed, the type must be REG_SZ, the
>+    // buffer size must not equal 0, and the buffer size be a multiple of 2.
>+    if (res != ERROR_SUCCESS || type != REG_SZ || size == 0 || size % 2 != 0)
>+        return NS_ERROR_FAILURE;
For this case as the first one size will return the size of path in case the value is an empty string. If path is too small dwRes will return ERROR_MORE_DATA.

Regarding SHGetValue... I prefer using the non-helper versions of api's since they typically take the same or longer time as the helper versions and we consistently use the non-helper versions in Mozilla code.

Comment 64

[Robert Strong (they/them - no direct email)]

(In reply to comment #63)
> Regarding SHGetValue... I prefer using the non-helper versions of api's since
> they typically take the same or longer time as the helper versions and we
> consistently use the non-helper versions in Mozilla code.
should read:
since the helper version typically take the same or longer time as the non-helper versions

Comment 65

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-1.9.2
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7347a2572f1d

Comment 66

[martin.feger]

(In reply to comment #63)
> >diff --git a/toolkit/crashreporter/client/crashreporter_win.cpp b/toolkit/crashreporter/client/crashreporter_win.cpp
> >--- a/toolkit/crashreporter/client/crashreporter_win.cpp
(...)
> >   wchar_t path[MAX_PATH];
> >...
> >+    HKEY key;
> >+    DWORD type, size, dwRes;

size is declared here.

> >+    dwRes = ::RegOpenKeyExW(HKEY_CURRENT_USER,
> >+                            L"Software\\Microsoft\\Windows\\CurrentVersion\\Explorer\\Shell Folders",
> >+                            0,
> >+                            KEY_READ,
> >+                            &key);
> >+    if (dwRes != ERROR_SUCCESS)
> >+      return false;
> >+
> >+    dwRes = RegQueryValueExW(key,
> >+                             L"AppData",
> >+                             NULL,
> >+                             &type,
> >+                             (LPBYTE)&path,
> >+                             &size);

And here it is used the first time, it was never initialized.

> For this case size will return the size of path in case the value is an empty
> string. If path is too small dwRes will return ERROR_MORE_DATA.

But how is RegQueryValueExW supposed to know if "path is too small" when you never tell it path's size? The MSDN link is quite clear IMHO:

lpcbData
A pointer to a variable that specifies the size of the buffer pointed to by the lpData parameter, in bytes. When the function returns, this variable contains the size of the data copied to lpData. 

The lpcbData parameter can be NULL only if lpData is NULL.

> >+++ b/toolkit/xre/nsXREDirProvider.cpp
> >@@ -927,6 +927,60 @@ GetShellFolderPath(int folder, nsAString

The use of RegQueryValueEx in this function is correct.

> >+++ b/xpcom/io/SpecialSystemDirectory.cpp
> >@@ -210,6 +210,44 @@ static nsresult GetWindowsFolder(int fol
> >...
> >+    WCHAR path[MAX_PATH + 2];
> >+    DWORD type, size;
> >+    res = RegQueryValueExW(key, (aLocal ? L"Local AppData" : L"AppData"), NULL,
> >+                           &type, (LPBYTE)&path, &size);

Again, RegQueryValueEx cannot determine on its own that path points to a buffer of MAX_PATH+2 WCHARs here, so the result is basically random.

Comment 67

[Jim Mathies [:jimm]]

Martin is right, 'size' should be initialized to max_path when requesting the actual data. It only works because size is likely some huge random number. Thankfully this code is executing on machines that are already totally borked!

Comment 68

[Tetsuro Kato (tete)]

It seems that the return value type of SHGetSpecialFolderPathW is not HRESULT but BOOL.

Comment 69

[Daniel Veditz [:dveditz]]

Are we ready to land this on 3.5.x now, or just leave it as a bugfix going forward? Seems like we don't really want to "block" on it.

Comment 70

[Robert Strong (they/them - no direct email)]

I personally don't think this needs to be backported since anyone in this state would most likely need to donwload a build with the fix and the current download has this fix.