1389160 - All addons disabled after browser restart if path to firefox profile contains non-ascii characters.

Status: VERIFIED FIXED

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

Description

[Nao Nakashima]

Attachment: Before restart.png

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170803103124

Steps to reproduce:

1. Create new Firefox profile
2. Install any addon that adds icon to toolbar. Examples:
https://addons.mozilla.org/ru/firefox/addon/privacy-badger17/
https://addons.mozilla.org/ru/firefox/addon/screenshot-capture-annotate/
https://addons.mozilla.org/ru/firefox/addon/ublock-origin/
3. Addon icon appears in toolbar or in "customize" window (for restartless addons)
4. Restart Firefox


Actual results:

All addon icons disappears from toolbar, menu and "customize" window.

Addon is listed as Enabled on about:addons page.
Icon can be re-enabled if you disable and enable addon (for restartless addons).
Icons for addons that requires restart can't be seen in any way.

Comment 1

[Nao Nakashima]

Attachment: After restart.png

Comment 2

[Nao Nakashima]

Attachment: After restart addons list.png

Comment 3

[Nao Nakashima]

Also this bug looks like bug 528299, but it was added 8 years ago, so I created a new one.

Comment 4

[:Gijs (he/him)]

I mean, clearly this works for most people or we wouldn't have shipped a release. We need to figure out why it's not working for you.

- Did this use to work with previous versions of Firefox? I assume you're having this issue with 55?
- This is going to sound dumb, but are you actually creating a new profile when testing this? Where is the profile stored? How do you start Firefox again with that profile? Is this affecting all your profiles or just some?
- are there errors in the browser console when you start up the second time?
- if you go to about:config after you start up the second time, and copy the contents of browser.uiCustomization.state , do the ids of the buttons those add-ons add occur in there?
- if after the restart you execute the following in the browser console (you need to enable chrome/remote debugging in the regular devtools first): CustomizableUI.getWidget(""), putting between the "" the id of the button of one of these add-ons, what does it return?

Comment 5

[Nao Nakashima]

(In reply to :Gijs from comment #4)
> - Did this use to work with previous versions of Firefox? I assume you're
> having this issue with 55?
Sure, I had no problems with versions < 55.

> - This is going to sound dumb, but are you actually creating a new profile
> when testing this? 
To be 100% sure, I moved my G:\Users\Маша\AppData\Roaming\Mozilla\Firefox folder
to other place and started Firefox.

> Where is the profile stored?
G:\Users\Маша\AppData\Roaming\Mozilla\Firefox\Profiles\ew4wrb4l.default

> How do you start Firefox again with that profile?
From desktop link, that points to "G:\Program Files\Mozilla Firefox\firefox.exe"

> Is this affecting all your profiles or just some?
All profiles.

> - are there errors in the browser console when you start up the second time?
Not sure they are related:

While creating services from category 'profile-after-change', service for entry 'Notification Telemetry Service', contract ID '@mozilla.org/notificationTelemetryService;1' does not implement nsIObserver.
Use of nsIFile in content process is deprecated. NetUtil.jsm:304:12
Use of nsIFile in content process is deprecated.  filesystem.js:40:9
Key event not available on some keyboard layouts: key=“r” modifiers=“accel,alt” id=“key_toggleReaderMode”  browser.xul
Key event not available on some keyboard layouts: key=“i” modifiers=“accel,alt,shift” id=“key_browserToolbox”  browser.xul

> - if you go to about:config after you start up the second time, and copy the
> contents of browser.uiCustomization.state , do the ids of the buttons those
> add-ons add occur in there?

Yes. It has "jid1-mnnxcxisbpnsxq_jetpack-browser-action" (from privacy badger) and "ublock0-button" (from ublock origin) in sections named "nav-bar" and "seen".

{"placements":{"PanelUI-contents":["edit-controls","zoom-controls","new-window-button","privatebrowsing-button","save-page-button","print-button","history-panelmenu","fullscreen-button","find-button","preferences-button","add-ons-button","developer-button","sync-button"],"addon-bar":["addonbar-closebutton","status-bar"],"PersonalToolbar":["personal-bookmarks"],"nav-bar":["urlbar-container","search-container","bookmarks-menu-button","downloads-button","home-button","pocket-button","jid1-mnnxcxisbpnsxq_jetpack-browser-action","ublock0-button"],"TabsToolbar":["tabbrowser-tabs","new-tab-button","alltabs-button"],"toolbar-menubar":["menubar-items"]},"seen":["pocket-button","developer-button","webide-button","jid1-mnnxcxisbpnsxq_jetpack-browser-action","ublock0-button"],"dirtyAreaCache":["PersonalToolbar","nav-bar","TabsToolbar","toolbar-menubar","PanelUI-contents","addon-bar"],"currentVersion":6,"newElementCount":0}

> - if after the restart you execute the following in the browser console ...

CustomizableUI.getWidget("jid1-mnnxcxisbpnsxq_jetpack-browser-action")
null
CustomizableUI.getWidget("ublock0-button")
null
CustomizableUI.getWidget("home-button")
Object { isGroup: true, id: "home-button", type: "custom", provider: "xul", forWindow: function (), areaType: Getter, instances: Getter }

If I re-enable this two addons I got buttons back and getWidget() shows info about them.
But they are gone after restart again...

Comment 6

[:Gijs (he/him)]

OK. To me the fact that the button is present in the nav-bar placements (so the pref hasn't been blown away or the button explicitly removed by anything), and the fact that getWidget returns null (which means CUI *knows* that the button used to be an API-style widget that got created once upon a time, but isn't present right now) suggest that the toolbar customization code knows that the buttons used to exist, but nothing is recreating them (or maybe something recreated them and then destroyed them again immediately). So it seems somehow the relevant bits of the add-on (or maybe all of it?) isn't running, despite being listed as enabled in the add-ons manager.

Do other side-effects of the add-on being enabled happen? For instance, can you check whether the ublock origin or privacy badger functionality is still working, even if the button isn't in the toolbar?

Mass-CC'ing some other folks who might have insights. Did we make changes to how we store the add-on data that could explain the add-on being listed but the code (as far as I can tell) not running?

Comment 7

[:Gijs (he/him)]

(In reply to Nao Nakashima from comment #5)
> (In reply to :Gijs from comment #4)
> > - Did this use to work with previous versions of Firefox? I assume you're
> > having this issue with 55?
> Sure, I had no problems with versions < 55.
> 
> > - This is going to sound dumb, but are you actually creating a new profile
> > when testing this? 
> To be 100% sure, I moved my G:\Users\Маша\AppData\Roaming\Mozilla\Firefox
> folder
> to other place and started Firefox.
> 
> > Where is the profile stored?
> G:\Users\Маша\AppData\Roaming\Mozilla\Firefox\Profiles\ew4wrb4l.default

Is this a network drive?

Comment 8

[Nao Nakashima]

(In reply to :Gijs from comment #6)
> Do other side-effects of the add-on being enabled happen? For instance, can
> you check whether the ublock origin or privacy badger functionality is still
> working, even if the button isn't in the toolbar?
No, they are not working. I tried some others add-ons and they all not working
at all after browser restart. They still enabled on about:addons page.


Also, sometimes after installing add-ons (using browser, on addons.mozilla.org) and restarting I see
that all add-ons are disabled on about:addons and breadcrumb menu informs me that some add-ons 
was installed not from firefox.
Like this (not my screenshot):
 https://ffp4g1ylyit3jdyti1hqcvtb-wpengine.netdna-ssl.com/addons/files/2017/06/image7.png
This is not reproduced always, I can't find a way to trigger it.

Comment 9

[Nao Nakashima]

> > G:\Users\Маша\AppData\Roaming\Mozilla\Firefox\Profiles\ew4wrb4l.default
> Is this a network drive?

No, it's local drive. Windows and all stuff installed on G:
There is no C: in my system.

Comment 10

[Nao Nakashima]

I found the root of the problem.
I created two new OS users and rechecked the problem:
1) User named "Маша2" - same problem
2) User named "test2" - problem doesn't occurs!

So looks like this bug is reproduced only if path to firefox profile containts non-ascii characters.

Not working:
G:\Users\Маша\AppData\Roaming\Mozilla\Firefox\Profiles\gflzddob.test

Good:
G:\Users\test\AppData\Roaming\Mozilla\Firefox\Profiles\57g381sw.default


I moved all my profiles to "G:\Firefox Profiles", changed paths in profiles.ini and now everything is working!

Comment 11

[Robert Helmer [:rhelmer]]

(In reply to Nao Nakashima from comment #10)
> I found the root of the problem.
> I created two new OS users and rechecked the problem:
> 1) User named "Маша2" - same problem
> 2) User named "test2" - problem doesn't occurs!
> 
> So looks like this bug is reproduced only if path to firefox profile
> containts non-ascii characters.
> 
> Not working:
> G:\Users\Маша\AppData\Roaming\Mozilla\Firefox\Profiles\gflzddob.test
> 
> Good:
> G:\Users\test\AppData\Roaming\Mozilla\Firefox\Profiles\57g381sw.default
> 
> 
> I moved all my profiles to "G:\Firefox Profiles", changed paths in
> profiles.ini and now everything is working!

Thanks for tracking that down - would be helpful to track this down to a specific bug, I'll try to do so today.

Comment 12

[Andy McKay]

[Tracking Requested - why for this release]:
I wonder if this is related to bug 1388584, which is getting a point release. It would be nice to test with that patch.

Comment 13

[Kohei Yoshino [:kohei]]

I have seen some Japanese people complaining about this on Twitter, probably because they're using Japanese characters for their Windows user name.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Yeah, very likely to be the same issue. Should be fixed in 55.0.2 (ETA Wednesday)

Comment 15

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #14)
> Yeah, very likely to be the same issue. Should be fixed in 55.0.2 (ETA
> Wednesday)

I think it's a separate issue in the addons code.  If I create a profile with \u0448 in the name on Windows 10.1 using Firefox 55.0.1:
- Places does not break.  This is expected because the code looked Unicode-safe, it just wasn't single-quote safe.
- The Privacy Badger extension I installed gets disabled.  The browser console says "Disabling foreign installed add-on..."[1] immediately after a message like "WARN Loading extensions 'null': Reading Manifest:..." that implies something went very wrong trying to parse the manifest which led to the add-on not being recognized.

This looks to be at least a little Windows specific because I was unable to reproduce on Linux.

If reproducing on Windows, I wasn't very experienced at inputting Unicode strings.  I found using wordpad's richedit control most useful.  I held ALT down and typed the decimal code "1096" into the pane and then released ALT to get the "ш" and then copied and pasted that into firefox -P's create profile option.  (Correction: I think I actually did "1064" which is the capital version, \u0428 "Ш".)

1: The code here seems to be: https://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProviderUtils.js#1277

Comment 16

[Andrew Sutherland [:asuth] (he/him)]

Attachment: extensions.json

Here is the extensions.json from the profile in question, as requested by :aswan.  
Note that the two extensions I personally installed were (legacy) Tree Style Tab and Privacy Badger.  There's also a (legacy) Firefox Hotfix that shows up in about:addons.  *Only* Privacy Badger is disabled.  (So maybe the extension parsing failure is less than fatal given that they show up in about:addons.)

Comment 17

[Andrew Sutherland [:asuth] (he/him)]

Attachment: addonStartup.json.lz4

Comment 18

[Nao Nakashima]

(In reply to Sylvestre Ledru [:sylvestre] from comment #14)
> Yeah, very likely to be the same issue. Should be fixed in 55.0.2 (ETA
> Wednesday)

The problem is not fixed in 55.0.2 btw.

Comment 19

[Kohei Yoshino [:kohei]]

Auto update to 55.0.2 is now enabled and Japanese people are complaining about this on Twitter.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Andrew, are you working on it?
We disabled updates because of this bug.

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Andy, fyi, we disabled updates because of this bug.

Comment 22

[Ted Campbell [:tcampbell]]

Any chance it is related to 8.3 filenames sneaking in somewhere? In Bug 1385928, I found problems when my username had more than 8chars (fully ascii though) and that was due to confusion about 8.3 filenames. (My problem could be worked around by disabling sandbox as well)

Comment 23

[Andrew Swan [:aswan]]

Kris said on IRC that he found the source of the problem, though I didn't actually see his explanation and he didn't respond to my request that he comment here with the explanation.  In any case, rhelmer also said he had a working patch but he was struggling with try.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Can we get a test for this too?

Comment 25

[Robert Helmer [:rhelmer]]

The problem is this fopen:
http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/AddonManagerStartup.cpp#145

I have a patch to move this over to NSPR instead, I'll post it momentarily.

Comment 26

[Robert Helmer [:rhelmer]]

So I did a few try runs overnight, and tried first simply changing to a non-ascii path to the profile for all add-ons manager tests, and this is not the only test that fails:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab19f8fe26900996c3afed7a72b07686a9f1492b&selectedJob=123798707

I suspect that we'd find other failures if we made this the default for all tests in the tree, but I will file a followup bug on that.

Comment 27

[Sylvestre Ledru [:Sylvestre]]

(In reply to Kohei Yoshino [:kohei] from comment #19)
> Auto update to 55.0.2 is now enabled and Japanese people are complaining
> about this on Twitter.
Kohei, do you see a lot of volume for this or just a few?
We are trying to understand the impact of this and see if it is a dot release driver or not.

Comment 28

[Robert Helmer [:rhelmer]]

Attachment: Bug 1389160 - use NSPR IO instead of raw fopen

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

Comment 29

[Robert Helmer [:rhelmer]]

Attachment: Bug 1389160 - use non-ascii in path to extensions dir by default for specified tests

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

Comment 30

[Kohei Yoshino [:kohei]]

Not many but some are tweeting clearly about this. And IIUC not only Japanese people are affected, right? Chinese, Korean, whoever using non-ascii chars in their Windows user name. I believe it's a dot release driver.

Comment 31

[Robert Helmer [:rhelmer]]

Comment on attachment 8898378 [details]
Bug 1389160 - use NSPR IO instead of raw fopen

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

Comment 32

[Robert Helmer [:rhelmer]]

Comment on attachment 8898379 [details]
Bug 1389160 - use non-ascii in path to extensions dir by default for specified tests

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

Comment 33

[Kris Maglione [:kmag]]

Comment on attachment 8898378 [details]
Bug 1389160 - use NSPR IO instead of raw fopen

https://reviewboard.mozilla.org/r/169752/#review175002

::: toolkit/mozapps/extensions/AddonManagerStartup.cpp:140
(Diff revision 1)
>    bool result;
>    return NS_SUCCEEDED(file->IsFile(&result)) && result;
>  }
>  
>  static nsCString
>  ReadFile(const char* path)

It would be better to pass in a nsIFile here and use `file->OpenNSPRFileDesc` to open it.

We probably also may as well make this return `Result<nsCString, nsresult>` so we can use NS_TRY in it.

Comment 34

[Kris Maglione [:kmag]]

Comment on attachment 8898379 [details]
Bug 1389160 - use non-ascii in path to extensions dir by default for specified tests

https://reviewboard.mozilla.org/r/169754/#review175004

::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:232
(Diff revision 1)
>  
>      // Get the profile directory for tests to use.
> +
> +    if (this.testNonAsciiProfile) {
> +      let nonAsciiDir = testScope.do_get_tempdir();
> +      nonAsciiDir.append("★ ☂");

Nit: Can you use a `\u` escape sequence for this? Sometimes our encoding for test files winds up screwed up, and I want to be sure we're testing the right thing.

::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:239
(Diff revision 1)
> +      this.profileDir = nonAsciiDir;
> +    } else {
> -    this.profileDir = testScope.do_get_profile();
> +      this.profileDir = testScope.do_get_profile();
> +    }
> +
> +    dump(`rhelmer debug profile dir: ${this.profileDir.path}\n`);

Please remove :)

Comment 35

[Robert Helmer [:rhelmer]]

OK so the test situation seems more complicated here... even with AddonTestUtils.jsm overriding gProfD, I still see some tests using the profile dir that the xpcshell harness is providing.

The good news is that the reason for the test failures in comment 26 isn't as dire as I had first assumed, but the bad news is that it makes this difficult to test in an automated way.

We (clearly) didn't have tests for this before though and the patch passes existing automated tests so going to proceed with that for now.

Comment 36

[Robert Helmer [:rhelmer]]

Comment on attachment 8898378 [details]
Bug 1389160 - use NSPR IO instead of raw fopen

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

Comment 37

[Kris Maglione [:kmag]]

Comment on attachment 8898378 [details]
Bug 1389160 - use NSPR IO instead of raw fopen

https://reviewboard.mozilla.org/r/169752/#review175024

::: toolkit/mozapps/extensions/AddonManagerStartup.cpp:145
(Diff revision 3)
> -  FILE* fd = fopen(path, READ_BINARYMODE);
> +  PRFileDesc* fd;
> +  NS_TRY(file->OpenNSPRFileDesc(PR_RDONLY, 0, &fd));

Nit:

    AutoCloseFile fd;
    NS_TRY(file->OpenNSPRFileDesc(PR_RDONLY, 0, &fd.rwget()));

Then remove the MakeScopeExit.

::: toolkit/mozapps/extensions/AddonManagerStartup.cpp:163
(Diff revision 3)
> -  size_t len = ftell(fd);
> -  if (len <= 0 || fseek(fd, 0, SEEK_SET) != 0) {
> -    return result;
> +  result.SetLength(size);
> +
> +  auto len = PR_Read(fd, result.BeginWriting(), size);
> -  }
>  
> -  result.SetLength(len);
> +  if (size != len) {

We should return failure here rather than truncating now that we have failure results.

::: toolkit/mozapps/extensions/AddonManagerStartup.cpp:248
(Diff revision 3)
>  
> -  nsCString lz4 = ReadFile(path);
> +  nsCString lz4;
> +  MOZ_TRY_VAR(lz4, ReadFile(file));
> +
>    if (lz4.IsEmpty()) {
>      return result;

Hm. I know you didn't change this, but we should get rid of `nsCString result` and just return `lz4`.

Comment 38

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Robert Helmer [:rhelmer] from comment #26)
> I suspect that we'd find other failures if we made this the default for all
> tests in the tree, but I will file a followup bug on that.

Yeah, given what happened in bug 1388584, I'm thinking we want all test profiles with auto-generated names to include the following in their generated paths:
- single-quote, to catch Bug 1388584 and any similar SQL escaping issue.
- Unicode code points from the Basic Multilingual Plane \u0000-\uffff that can be represented by a single code unit in JS.  The patch up for review includes points from this plane: \u2602 "☂", and \u2605 "★" which both fit in a single JS code unit.  This on its own should catch out problems like this bug where fopen only supports ASCII (windows), or general locale/LC_CTYPE problems.
- Unicode code points from the Astral Planes that must be represented as multiple code units in JS and thus represent a new edge case.  It just so happens that "FOX FACE", \u{1F98A} "

Comment 39

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Robert Helmer [:rhelmer] from comment #26)
> I suspect that we'd find other failures if we made this the default for all
> tests in the tree, but I will file a followup bug on that.

Yeah, given what happened in bug 1388584, I'm thinking we want all test profiles with auto-generated names to include the following in their generated paths:
- single-quote, to catch Bug 1388584 and any similar SQL escaping issue.
- Unicode code points from the Basic Multilingual Plane \u0000-\uffff that can be represented by a single code unit in JS.  The patch up for review includes points from this plane: \u2602 "☂", and \u2605 "★" which both fit in a single JS code unit.  This on its own should catch out problems like this bug where fopen only supports ASCII (windows), or general locale/LC_CTYPE problems.
- Unicode code points from the Astral Planes that must be represented as multiple code units in JS and thus represent a new edge case.  It just so happens that "FOX FACE", \u{1F98A} (omitted because it truncated my previous comment) is in this plane, and seems like a good candidate.
- Unicode Regional Indicator Symbol pairs that either render as the ISO 3166 letter pairs or a cool flag. \u{1f1fa}\u{1f1f3}is the letter pair "UN" which renders as (omitted because of truncation) in whatever font you're looking at this.  I'm not actually sure this gets us anything beyond the fox face, but I think soccer/football fans like to use flags around World Cup time?  The UN flag choice seems least contentious of an arbitrary choice.

Things it looks like we don't need to worry about:
- Double-quote is illegal for Windows, so although that's also an obvious choice to worry about, it's not allowed to happen, so, hooray for some good news.

Comment 40

[Robert Helmer [:rhelmer]]

Comment on attachment 8898378 [details]
Bug 1389160 - use NSPR IO instead of raw fopen

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169752/diff/3-4/

Comment 41

[Robert Helmer [:rhelmer]]

Comment on attachment 8898378 [details]
Bug 1389160 - use NSPR IO instead of raw fopen

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169752/diff/4-5/

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5c55ebeb214b
use NSPR IO instead of raw fopen r=kmag

Comment 43

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5c55ebeb214b

Comment 44

[Marco Bonardo [:mak]]

(In reply to Andrew Sutherland [:asuth] from comment #39)
> Yeah, given what happened in bug 1388584, I'm thinking we want all test
> profiles with auto-generated names to include the following in their
> generated paths:

Did anyone file a bug to investigate this very good testing suggestion by Andrew yet?
I can't see one, it for sure would help in the future with avoiding these surprising issues.

Comment 45

[Sylvestre Ledru [:Sylvestre]]

Robert, please fill the uplift request to release. It impacts French users too. So, I guess many locales are impacted too. This is a driver for a dot release I think.

Comment 46

[Robert Helmer [:rhelmer]]

(In reply to Marco Bonardo [::mak] from comment #44)
> (In reply to Andrew Sutherland [:asuth] from comment #39)
> > Yeah, given what happened in bug 1388584, I'm thinking we want all test
> > profiles with auto-generated names to include the following in their
> > generated paths:
> 
> Did anyone file a bug to investigate this very good testing suggestion by
> Andrew yet?
> I can't see one, it for sure would help in the future with avoiding these
> surprising issues.

Thanks for the reminder, I just filed bug 1392308 for this.

Comment 47

[Robert Helmer [:rhelmer]]

Comment on attachment 8898378 [details]
Bug 1389160 - use NSPR IO instead of raw fopen

Approval Request Comment
[Feature/Bug causing the regression]: bug 1358846
[User impact if declined]: users with non-ascii in path (such as unicode in username) will start up with add-ons disabled
[Is this code covered by automated tests?]: yes by existing coverage - the exact case here needs more work to test, bug 1392308
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: yes, create profile with unicode in profile path and install add-on(s), then restart and verify that add-on(s) are still active
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: minor
[Why is the change risky/not risky?]: moving from raw fopen to NSPR IO in simple read-file-to-string function, covered by existing tests
[String changes made/needed]: none

Comment 48

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

Comment on attachment 8898378 [details]
Bug 1389160 - use NSPR IO instead of raw fopen

OK for beta uplift, fix for data loss issue.

Comment 49

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8898378 [details]
Bug 1389160 - use NSPR IO instead of raw fopen

let's take this on 55 as well.

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cef728b1b47e

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/86e3368b6058

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/e61f84dd3432

Comment 53

[CosminB]

This issue is verified as fixed on Firefox 55.0.3 (20170821144629), Firefox 56.0b5 (20170821193225) and Firefox 57.0a1 (2017-08-21) under Wind 7 64-bit, Wind 8.1 64-bit and Wind 10 32-bit.

Firefox profiles with a path that contains non-ASCII characters will not disable the extensions/legacy addons after the restart of the browser.

Results: https://public.etherpad-mozilla.org/p/Bug1389160 

While running the tests I observed that some icons with the country flags were not displayed in the profile name box.

Is this behavior expected or I should file another bug for this issue Robert?

Comment 54

[Andrew Sutherland [:asuth] (he/him)]

(In reply to CosminB from comment #53)
> While running the tests I observed that some icons with the country flags
> were not displayed in the profile name box.
> 
> Is this behavior expected or I should file another bug for this issue Robert?

This is expected.  Country flags are a function of the font, just like how Mozilla's new font has a custom ligature (or whatever magic fonts use) that makes it look like "Moz://a" when you type "Mozilla".  If the font doesn't have a given flag, then the font should display the possibly-stylized two-letter country-code if it has those characters (or the system is able to failover to another font with those characters).  It's also possible unknown character glyphs could be displayed in those cases.  The main thing to look for is that there isn't something that looks like https://en.wikipedia.org/wiki/Mojibake and that the profile is using the expected directory.

Comment 55

[Emil Ghitta, QA [:emilghitta]]

Thanks CosminB for verifying this issue!

Comment 56

[Sylvestre Ledru [:Sylvestre]]

It is possible that we caused some regressions:
https://www.reddit.com/r/firefox/comments/6w48lz/firefox_updated_to_5503_everything_appears_to/

Comment 57

[Robert Helmer [:rhelmer]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #56)
> It is possible that we caused some regressions:
> https://www.reddit.com/r/firefox/comments/6w48lz/
> firefox_updated_to_5503_everything_appears_to/

This code is only for loading add-on startup data, it doesn't have anything to do with bookmarks.

Comment 58

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #57)
> (In reply to Sylvestre Ledru [:sylvestre] from comment #56)
> > It is possible that we caused some regressions:
> > https://www.reddit.com/r/firefox/comments/6w48lz/
> > firefox_updated_to_5503_everything_appears_to/
> 
> This code is only for loading add-on startup data, it doesn't have anything
> to do with bookmarks.

Bug 1388584 sounds more likely (which is a similar issue but different implementation)