Windows langpacks are generated with invalid paths

VERIFIED FIXED in Firefox 58

Status

()

defect
P1
blocker
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: marius.santa, Assigned: gandalf)

Tracking

({regression})

58 Branch
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox56 unaffected, firefox57 unaffected, firefox58 verified)

Details

Attachments

(3 attachments)

Reporter

Description

2 years ago
Posted file Temp.zip
[Affected versions]:
Firefox 58.0a1 (20171025230440)

[Affected platforms]:
Windows 10 64-bit
MacOS Sierra 10.13
Ubuntu 16.04 32-bit


[Steps to reproduce]:
1.Launch Firefox with a clean profile.
2.Install https://addons-dev.allizom.org/en-US/firefox/addon/pl-webext-lang-58-0/
3.Go to "about:config".
4.Set "intl.locale.matchOS = False" and "general.useragent.locale = pl".
5.Restart the browser.


[Expected Results]:
The browser restart with the language changed to polish.

[Actual Results]:
An error window is displayed.


[Regression Range]:
Last good revision: 7cc3f9fd01acb701fec4b872634513c8c59f5f1a
First bad revision: 63618add0894f613af2a5b7939a60c2251443ddf
Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=7cc3f9fd01acb701fec4b872634513c8c59f5f1a&tochange=63618add0894f613af2a5b7939a60c2251443ddf

[Additional notes]:
Gif presenting the issue is attached.
Error message text is attached.
When you say "An error window is displayed" do you mean a yellow screen with a red error?

If that's the case, then it's an expected behavior. Basically, nightly code changes every day, and we do not produce nightly langpacks to match those daily changes.

The result is that installing a nightly langpack on a nightly build almost always will give you an outdated localization which in turn triggers the yellow screen.

We're addressing it with a transition to a new localization API that will make us fallback more gracefully.

Please, confirm that that's what you mean, and not what the title states (A crash).
Flags: needinfo?(marius.santa)
Reporter

Comment 2

2 years ago
Yes it is the yellow screen, and the issue you described as you can see from the attached gif. The problem is that from a users point of view the browser crashed with an error, and the only way to use it, is to create a new clean profile.

Considering I am testing Nightly, can you please provide some steps in order for me to be able to generate the appropriate language packs?
Flags: needinfo?(marius.santa)
> Considering I am testing Nightly, can you please provide some steps in order for me to be able to generate the appropriate language packs?

Sure, but please, be warned, that we do not currently support using nightly with langpacks :)

You need to build Firefox, and then do "./mach build langpack-$loc" where $loc is your locale code (de, pl, it etc.) - pick one of those codes we have l10n for : https://hg.mozilla.org/l10n-central/

The result is an xpi in objdir/dist/platform/xpi/langpack-*-58-*-$loc.xpi

Install this XPI and you'll be able to use a localized build until it updates to a newer nightly and your langpack breaks :(

As I said, there's a major effort to rework how our localization works that in the end will result in us dropping from a missing $loc string to fallback en-US string rather than showing yellow screen of death.

Until this point, we support langpacks only in beta and release. Sorry for that!

I'm going to close this bug as wontfix, although we are fixing it. It's just going to take a long time (at least half a year I expect) before we're there.

If you want to track the progress on the new localization API, see bug 1365426.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
Marius, if that langpack on addons-dev is yours, could you please pull it off the site? Even on a dev instance, I wouldn't want people to find this and then fall into the same rabbit hole as you just did. Thanks.
We also apparently do nightly langpack builds these days:

https://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-central-l10n/win64/xpi/
Reporter

Comment 6

2 years ago
@Axel Hecht I removed the language packs from addons-dev.

@Kris Maglione We used one of the xpi created on the 27th with 58.0a1(20171026221945) witch is from the 26th and we got the same error window but with the message:
<window id="main-window"
^
Is this the same issue?

@Zibi Braniecki we can't make our own custom build it takes programs we don't generally use, lots of time and we get lot of errors, it's not feasible, is there any other way to do this?
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(gandalf)
> @Zibi Braniecki we can't make our own custom build it takes programs we don't generally use, lots of time and we get lot of errors, it's not feasible, is there any other way to do this?

I don't know of any other way to get a language pack for nightly than building it.

If you want to test a language pack, you need to:

1) Build Firefox
2) Build language pack for this very version of language pack
3) Install it and test.

Alternatively you can wait for 58 to hit beta when you can test beta releases against beta langpacks without having to build anything manually.
Flags: needinfo?(gandalf)
Flags: needinfo?(kmaglione+bmo)
Sorry, I didn't mean to clear the needinfo before.

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #7)
> Alternatively you can wait for 58 to hit beta when you can test beta
> releases against beta langpacks without having to build anything manually.

We should really aim to get as much QA as possible done before this hits beta...

(In reply to marius.santa from comment #6)
> @Kris Maglione We used one of the xpi created on the 27th with
> 58.0a1(20171026221945) witch is from the 26th and we got the same error
> window but with the message:
> <window id="main-window"
> ^
> Is this the same issue?

No, this is a different issue. It looks like the paths in the manifest.json files for the windows langpacks are mangled, and have backslashes as path separators in some places:

        "onboarding": "browser\\features\\onboarding@mozilla.org\\pl/locale/pl/", 

Zibi, can you look into this? This is probably a release blocker.
Severity: major → blocker
Status: RESOLVED → REOPENED
Component: WebExtensions: General → Internationalization
Flags: needinfo?(gandalf)
Product: Toolkit → Core
Resolution: WONTFIX → ---
Summary: Crash after web extension activating language pack → Windows langpacks are generated with invalid paths
Note that the OS-X langpacks should work on Windows (I tested, and they do work for me on Linux, at least), so testing with those may be a good workaround until the Windows langpacks are fixed:

https://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-central-l10n/mac/xpi/

(Also, AMO actually only signs and distributes Windows langpacks, on the basis that they should be the same for all platforms, and there isn't any install manifest data to distinguish them, so it shouldn't really matter which we test with. But the Windows builds really need to be fixed, if only for the sake of AMO.)
> We should really aim to get as much QA as possible done before this hits beta...

Ok, then I'd say what we need is:

1) Particular release of Firefox Nightly (if possible to the hg revision)
2) A langpack built off of that revision

Then we can test it.

> Zibi, can you look into this? This is probably a release blocker.

Yeah, will look into this!

But I'm wondering - what's the value of producing langpacks per platform, if all platforms share the same langpack? It seems to me that we could just take, say, linux langpacks and release them.
Assignee: nobody → gandalf
Status: REOPENED → ASSIGNED
Flags: needinfo?(gandalf)
Assignee

Updated

2 years ago
Flags: needinfo?(gandalf)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #10)
> But I'm wondering - what's the value of producing langpacks per platform, if
> all platforms share the same langpack? It seems to me that we could just
> take, say, linux langpacks and release them.

We could, but that doesn't justify building broken Windows langpacks ;)

I'm all for not building separate langpacks for each platform, but that's a separate bug, really.

Comment 13

2 years ago
mozreview-review
Comment on attachment 8923145 [details]
Bug 1411951 - Use mozpack.path.normsep when generating chrome entry paths for langpack manifest.json.

https://reviewboard.mozilla.org/r/194334/#review199286

::: python/mozbuild/mozbuild/action/langpack_manifest.py:21
(Diff revision 1)
>  import json
>  import io
>  import datetime
>  import requests
>  import mozversioncontrol
> +import posixpath

I suggest to use mozpack.path instead. Does the same thing, but probably created for the purpose of "paths in our packages", https://firefox-source-docs.mozilla.org/python/mozpack.html#module-mozpack.path.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Thank you Axel! I was looking for such function in the regular os.path module and was surprised not to find it. This is much better!
Flags: needinfo?(gandalf)
Reporter

Comment 17

2 years ago
I can confirm that on Mac OS 10.13 and Ubuntu 16.04 32-bit using Nightly 58.0a1 (20171029220112) language pack activation works fine.

Comment 18

2 years ago
mozreview-review
Comment on attachment 8923145 [details]
Bug 1411951 - Use mozpack.path.normsep when generating chrome entry paths for langpack manifest.json.

https://reviewboard.mozilla.org/r/194334/#review200504

::: python/mozbuild/mozbuild/action/langpack_manifest.py:21
(Diff revision 3)
>  import json
>  import io
>  import datetime
>  import requests
>  import mozversioncontrol
> +import mozpack.path

Everywhere else we do 'import mozpack.path as mozpath'. We have bug 1399430 to make that more sane, but in the meantime it probably makes sense to be consistent with existing scripts.

::: python/mozbuild/mozbuild/action/langpack_manifest.py:271
(Diff revision 3)
>                  os.path.join(os.path.dirname(path), entry.relpath),
>                  base_path,
>                  chrome_entries
>              )
>          elif isinstance(entry, ManifestLocale):
> -            chrome_entries.append({
> +            entry_path = os.path.join(

I think you can just change os.path.join to mozpath.join to get the correct separators, rather than mozpath.normpath(os.path.join). A quick test shows it differs slightly in whether or not there's a trailing '/' character. If that matters in this case, feel free to drop this issue. Otherwise I think it'll be simpler with mozpath.join()
Attachment #8923145 - Flags: review?(mshal)
> I think you can just change os.path.join to mozpath.join to get the correct separators, rather than mozpath.normpath(os.path.join). A quick test shows it differs slightly in whether or not there's a trailing '/' character. If that matters in this case, feel free to drop this issue. Otherwise I think it'll be simpler with mozpath.join()

I tried that, but that seemed to require me to move all os.path.join to mozpath.join including ones the recursive ones that are used to actually load files from the disk.
Otherwise I end up with weird "browser\\locales/browser/en-US/file.dtd"

It may be that our setup allows me to use posix paths to load files in python, but I'm not sure if that's better than just normalizing the path before injecting it into the manifest.

What's your preference?
Flags: needinfo?(mshal)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #19)
> > I think you can just change os.path.join to mozpath.join to get the correct separators, rather than mozpath.normpath(os.path.join). A quick test shows it differs slightly in whether or not there's a trailing '/' character. If that matters in this case, feel free to drop this issue. Otherwise I think it'll be simpler with mozpath.join()
> 
> I tried that, but that seemed to require me to move all os.path.join to
> mozpath.join including ones the recursive ones that are used to actually
> load files from the disk.
> Otherwise I end up with weird "browser\\locales/browser/en-US/file.dtd"
> 
> It may be that our setup allows me to use posix paths to load files in
> python, but I'm not sure if that's better than just normalizing the path
> before injecting it into the manifest.
> 
> What's your preference?

Ok, just use normpath as you had then and switch the import so it's mozpath.normpath()
Flags: needinfo?(mshal)
Priority: -- → P1
Comment hidden (mozreview-request)

Comment 22

2 years ago
mozreview-review
Comment on attachment 8923145 [details]
Bug 1411951 - Use mozpack.path.normsep when generating chrome entry paths for langpack manifest.json.

https://reviewboard.mozilla.org/r/194334/#review201122

r+ with the function name fixed. As it is it would get an AttributeError.

::: python/mozbuild/mozbuild/action/langpack_manifest.py:283
(Diff revision 4)
> +            chrome_entries.append({
> +                'type': 'locale',
> +                'alias': entry.name,
> +                'locale': entry.id,
> +                'platforms': convert_entry_flags_to_platform_codes(entry.flags),
> +                'path': mozpath.path.normsep(entry_path)

This should just be 'mozpath.normsep', not 'mozpath.path.normsep'.
Attachment #8923145 - Flags: review?(mshal) → review+
Comment hidden (mozreview-request)

Comment 24

2 years ago
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a4fb78381ea0
Use mozpack.path.normsep when generating chrome entry paths for langpack manifest.json. r=mshal

Comment 25

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a4fb78381ea0
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Reporter

Comment 26

2 years ago
This issue is verified as fixed on Firefox 58.0a1 (20171106100122) under Wind 10 64-bit.

Post-fix screenshot attached.
Reporter

Updated

2 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.