1428234 - addon icons missing in about:addons page when profile path includes non-ascii characters

Status: VERIFIED FIXED

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

Description

[Baptiste Thémine]

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.

Comment 1

[Baptiste Thémine]

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

Comment 2

[Andrew Swan [:aswan]]

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

Comment 3

[Sylvestre Ledru [:Sylvestre]]

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

Comment 4

[Masatoshi Kimura [:emk]]

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

Comment 5

[Andrew Swan [:aswan]]

(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.

Comment 6

[Baptiste Thémine]

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.

Comment 7

[Andrew Swan [:aswan]]

(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...

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Comment 9

[Andrew Swan [:aswan]]

I'm back and can pick this up

Comment 10

[Andrew Swan [:aswan]]

Attachment: Bug 1428234 Part 1: Remove incorrect uses of persistentDescriptor in AddonManager

Review commit: https://reviewboard.mozilla.org/r/215188/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215188/

Comment 11

[Andrew Swan [:aswan]]

Attachment: Bug 1428234 Part 2: Force extensions database rebuild

Prior to the first patch for this bug, users with non-ascii characters
in the path to their profile directory got bad data in extensions.json.
With that bug fixed, now force a rebuild to rescue those users.

Review commit: https://reviewboard.mozilla.org/r/215190/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215190/

Comment 12

[Masatoshi Kimura [:emk]]

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 13

[Kris Maglione [:kmag]]

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 14

[Kris Maglione [:kmag]]

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

https://reviewboard.mozilla.org/r/215190/#review221140

Comment 15

[Andrew Swan [:aswan]]

Comment on attachment 8945005 [details]
Bug 1428234 Part 1: Remove incorrect uses of persistentDescriptor in AddonManager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215188/diff/1-2/

Comment 16

[Andrew Swan [:aswan]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215190/diff/1-2/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Andrei Ciure[:aciure]]

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

Comment 19

[Andrew Swan [:aswan]]

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

Comment 20

[Baptiste Thémine]

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Andrew Swan [:aswan]]

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 23

[Andrew Swan [:aswan]]

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

see comment 22

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 25

[Bogdan Tara[:bogdan_tara | bogdant]]

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

Comment 26

[marius.santa]

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