User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0 Build ID: 20171024165158 Steps to reproduce: Since several months, addon icons disappear randomly in about:addons page when I install new addons. After some search, I discovered thanks to the web dev tool that <xul:image> contains a wrong path in its src attribute due to accented characters present in my Windows user folder. I finally solved the issue by replacing manually the character "é" by "é" in the property "path" of the file extensions.json in Firefox profile folder. All versions of Firefox are concerned. Tested on Firefox from 54 to 59 on Windows 7. Actual results: Addon icons disappear randomly in about:addons page when I install new addons. The property "path" of the file extensions.json in Firefox profile folder contains misinterpreted accented characters coming from Windows user folder. Expected results: The file extensions.json should contain correct accented characters. Otherwise, a possible solution is to use an environment variable such as %APPDATA% in the path.

Related issue : https://bugzilla.mozilla.org/show_bug.cgi?id=1323448

Thanks for the analysis, that's helpful. Perhaps bug 1423897 would help us with this and related issues...

Too late for 58 but we might take a patch for 59.

I found some XPIDatabase.addAddonMetadata callers treat an persistentDescriptor as a path.[1][2] This is wrong. The descriptor should be changed to a path using descriptorToPath. The descriptorToPath comment[3] says: > * Converts the given opaque descriptor string into an ordinary path > * string. In practice, the path string is always exactly equal to the > * descriptor string, but theoretically may not have been on some legacy > * systems. Again, this is wrong. It will break non-ASCII characters to treat descriptors as paths on all (not legacy) Windows systems. You should not depend on implementation detaills of persistentDescriptor in the first place. Not all XPIDatabase.addAddonMetadata callers pass a descriptor. I have no idea how can we repair already-broken data in extensions.json :( [1] https://dxr.mozilla.org/mozilla-central/rev/c38d22170d6f27e94c3c53093215d20255fab27a/toolkit/mozapps/extensions/internal/XPIInstall.jsm#1853 [2] https://dxr.mozilla.org/mozilla-central/rev/c38d22170d6f27e94c3c53093215d20255fab27a/toolkit/mozapps/extensions/internal/XPIProvider.jsm#3629 [3] https://dxr.mozilla.org/mozilla-central/rev/c38d22170d6f27e94c3c53093215d20255fab27a/toolkit/mozapps/extensions/internal/XPIProvider.jsm#410-413

(In reply to Masatoshi Kimura [:emk] from comment #4) > Again, this is wrong. It will break non-ASCII characters to treat > descriptors as paths on all (not legacy) Windows systems. You should not > depend on implementation detaills of persistentDescriptor in the first place. Ugh, thank you for the explanation. Once we have the code fixed to stop doing this, we can actually throw away extensions.json and rebuild it. The simplest thing would just be to bump the database schema, that would force a rebuild for every user when they update to a build that has the fix. Maybe we could limit the rebuilds to profiles in which extensions.json contains non-ASCII characters in a path field. But we'd want to make sure we only do that one time, the rebuild is relatively expensive so we don't want to do it on every startup. I'm thinking the one-time rebuild for everybody is probably the better option.

Maybe a full rebuild is not necessary. You could just replace all full paths by relative paths in extensions.json and use an environment variable/a preference in Firefox which contains the Windows AppData path. This is just an idea, I don't know how difficult it is to implement.

(In reply to Baptiste Thémine from comment #6) > Maybe a full rebuild is not necessary. You could just replace all full paths > by relative paths in extensions.json extensions.json can include things outside the profile so this won't work...

Andy, is there someone else who can take this on for 59 while aswan is out on PTO?

I'm back and can pick this up

Comment on attachment 8945005 [details] Bug 1428234 Part 1: Remove incorrect uses of persistentDescriptor in AddonManager https://reviewboard.mozilla.org/r/215188/#review221118 Looks good to me about .persistenDescriptor/.path usage. I assume that Kris will review the rest. ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1407 (Diff revision 1) > * > * @param {DBAddon} addon > * The DBAddon to add. > */ > addAddon(addon) { > + dump(`${new Error().stack}\n`); Forgot to remove a debug output?

Comment on attachment 8945005 [details] Bug 1428234 Part 1: Remove incorrect uses of persistentDescriptor in AddonManager https://reviewboard.mozilla.org/r/215188/#review221138 ::: toolkit/mozapps/extensions/test/xpcshell/test_db_path.js:15 (Diff revision 1) > +// a past bug with such paths) > +add_task(async function test_non_ascii_path() { > + let env = Components.classes["@mozilla.org/process/environment;1"] > + .getService(Components.interfaces.nsIEnvironment); > + const PROFILE_VAR = "XPCSHELL_TEST_PROFILE_DIR"; > + let profileDir = OS.Path.join(env.get(PROFILE_VAR), "Î åm ñøt åsçii"); It would probably be better to us \u escapes for this. Ideally, our test files should handle UTF-8 just fine. In practice, though, they often get parsed as ISO-8859-1, and don't handle multi-byte characters well.

Comment on attachment 8945006 [details] Bug 1428234 Part 2: Force extensions database rebuild https://reviewboard.mozilla.org/r/215190/#review221140

Pushed by aswan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/993f338d5088 Part 1: Remove incorrect uses of persistentDescriptor in AddonManager r=emk,kmag https://hg.mozilla.org/integration/autoland/rev/35a54155e966 Part 2: Force extensions database rebuild r=kmag

https://hg.mozilla.org/mozilla-central/rev/993f338d5088 https://hg.mozilla.org/mozilla-central/rev/35a54155e966

Baptiste, can you verify that the most recent Nightly build fixes this bug for you?

Of course ! I updated to Nightly 60.0a1 (build 20180126220105). I installed addons from AMO and from about:debugging without any issue for now.

Please request Beta approval on this when you get a chance. It grafts cleanly as-landed.

Comment on attachment 8945005 [details] Bug 1428234 Part 1: Remove incorrect uses of persistentDescriptor in AddonManager Approval Request Comment [Feature/Bug causing the regression]: N/A This is not a (recent) regression [User impact if declined]: Users with non-ASCII characters in their profile path will continue to experience glitches with add-ons (including the icon issue described in the bug, possibly others) [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: yes (comment 20) [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: Just the other patch from this bug [Is the change risky?]: no [Why is the change risky/not risky?]: The actual functional change is small and easy to verify manually [String changes made/needed]: none

Comment on attachment 8945006 [details] Bug 1428234 Part 2: Force extensions database rebuild see comment 22

Comment on attachment 8945005 [details] Bug 1428234 Part 1: Remove incorrect uses of persistentDescriptor in AddonManager Sounds like a useful fix, and good that it includes new tests. Let's uplift this for 59 beta 6.

https://hg.mozilla.org/releases/mozilla-beta/rev/eb62496a89f3 https://hg.mozilla.org/releases/mozilla-beta/rev/e379d4c18dbb

Marking verified for FF 59 because of https://bugzilla.mozilla.org/show_bug.cgi?id=1428234#c20 and 60 is already in production.