Closed Bug 1428234 Opened 2 years ago Closed 2 years ago

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

Categories

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

All
Windows
defect

Tracking

()

VERIFIED FIXED
mozilla60
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 + verified
firefox60 --- verified

People

(Reporter: baptiste.themine, Assigned: aswan)

References

Details

Attachments

(2 files)

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.
OS: Unspecified → Windows
Hardware: Unspecified → All
Component: Untriaged → Add-ons Manager
Product: Firefox → Toolkit
Thanks for the analysis, that's helpful.  Perhaps bug 1423897 would help us with this and related issues...
Priority: -- → P3
See Also: → 1423897
Summary: addon icons missing in about:addons page → addon icons missing in about:addons page when profile path includes non-ascii characters
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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.
Priority: P3 → P2
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?
Flags: needinfo?(amckay)
I'm back and can pick this up
Assignee: nobody → aswan
Flags: needinfo?(amckay)
Attachment #8945005 - Flags: review?(kmaglione+bmo)
Attachment #8945005 - Flags: review?(VYV03354)
Attachment #8945006 - Flags: review?(kmaglione+bmo)
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?
Attachment #8945005 - Flags: review?(VYV03354) → review+
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.
Attachment #8945005 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8945006 [details]
Bug 1428234 Part 2: Force extensions database rebuild

https://reviewboard.mozilla.org/r/215190/#review221140
Attachment #8945006 - Flags: review?(kmaglione+bmo) → review+
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
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Baptiste, can you verify that the most recent Nightly build fixes this bug for you?
Flags: needinfo?(baptiste.themine)
Of course ! I updated to Nightly 60.0a1 (build 20180126220105).
I installed addons from AMO and from about:debugging without any issue for now.
Flags: needinfo?(baptiste.themine)
Please request Beta approval on this when you get a chance. It grafts cleanly as-landed.
Flags: needinfo?(aswan)
Flags: in-testsuite+
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
Flags: needinfo?(aswan)
Attachment #8945005 - Flags: approval-mozilla-beta?
Comment on attachment 8945006 [details]
Bug 1428234 Part 2: Force extensions database rebuild

see comment 22
Attachment #8945006 - Flags: approval-mozilla-beta?
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.
Attachment #8945005 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8945006 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Marking verified for FF 59 because of https://bugzilla.mozilla.org/show_bug.cgi?id=1428234#c20 and 60 is already in production.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.