566787 - Error getting 'Home' directory in PluginProvider.jsm line 185 in msys shell (e.g. running mochitests)

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P3)

Description

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

Warning: WARN addons.manager: Exception calling callback: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIProperties.get]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///d:/moz/_1_mozilla-central/ff-opt/dist/bin/modules/PluginProvider.jsm :: anonymous :: line 185"  data: no]

This currently breaks app update when it tries to check for incompatible add-ons.

Comment 1

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

Forgot to add the mxr url
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/PluginProvider.jsm#221

Comment 2

[Dave Townsend [:mossop]]

Strange, doesn't every platform implement "Home"?

Comment 3

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

Looks like this only happens when running mochitest on Windows so removing the panic flags. Is there a way to get a list of the types so I can then call getAddonsByTypes and exclude plugins in app update?

Comment 4

[Dave Townsend [:mossop]]

It looks like on Windows getting "Home" should get through to here, are these environment variables missing? http://mxr.mozilla.org/mozilla-central/source/xpcom/io/SpecialSystemDirectory.cpp#717

Comment 5

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

Right band it is set. msys uses /c/ for the drive which likely confuses the code

Comment 6

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

s/band/and/

It is usually incorrect to use env vars to get well known paths on Windows... there are API calls to accomplish this. Below are the associated env vars for a cmd.exe and msys shells

cmd.exe
HOMEDRIVE=C:
HOMEPATH=\Users\username
USERPROFILE=C:\Users\username

msys
HOME=/c/Users/username
HOMEDRIVE=C:
HOMEPATH='\'
USERPROFILE=C:\Users\username

Comment 7

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

From a quick search on mxr it looks like "Home" is only used by the following on Windows (appPicker.js actually uses ProgF on Windows and browser_420786.js returns early except on Linux)
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/filepicker/content/filepicker.js#183

Comment 8

[Dave Townsend [:mossop]]

What I think we want here is the "C:\Documents and Settings\<username>" on XP, "C:\Users\<username>" on Windows 7 and whatever it is on Vista. Home gives us that in some cases but maybe we should make that use an API instead now. I can't see a different directory service definition that covers it.

Comment 9

[Dave Townsend [:mossop]]

In fact it looks like we need to talk to the windows API with CSIDL_PROFILE. That seems to be used in a couple of places for WINCE but not for regular windows.

Comment 11

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Spinning this through the try server now but this seems to pass on my machines. It attempts to use CSIDL_PROFILE to get the home directory, if not attempts the previous methods that were there. The test verifies against USERPROFILE which appears to be correct on my windows box and tinderbox.

Comment 12

[Jim Mathies [:jimm]]

Comment on attachment 466698 [details] [diff] [review]
patch rev 1

Technically, I don't think I can approve xpcom/io patches. So you might want a sr. But this looks far more correct to me than what we're currently doing with GetEnvironmentVariableW.

Comment 13

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/bc60f89fe179

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed based on check-in and passing tests.