Closed Bug 1425662 Opened 2 years ago Closed 2 years ago

Problems reading _locales directories on Linux

Categories

(WebExtensions :: General, defect)

57 Branch
defect
Not set

Tracking

(firefox59 fixed)

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: javier--Lx3u0NnX4Eug3hq7uRufuMOjDIwrH7, Assigned: javier--Lx3u0NnX4Eug3hq7uRufuMOjDIwrH7)

References

Details

Attachments

(1 file, 3 obsolete files)

I am following the guidelines at
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Internationalization
for my extension, but it will not load. I have tried the notify-link-clicks-i18n example and the result is the same. web-ext run fails with:

[program.js][error] 
WebExtError: installTemporaryAddon: Error: unknownError: Could not install add-on at '/home/test/webextensions-examples/notify-link-clicks-i18n': Error: Extension is invalid
    at /home/test/node_modules/web-ext/dist/webpack:/src/firefox/remote.js:116:27
    at EventEmitter.handleMessage (/home/test/node_modules/firefox-client/lib/client.js:161:7)
    at EventEmitter.readMessage (/home/test/node_modules/firefox-client/lib/client.js:220:10)
    at EventEmitter.onData (/home/test/node_modules/firefox-client/lib/client.js:186:16)
    at emitOne (events.js:96:13)
    at Socket.emit (events.js:188:7)
    at readableAddChunk (_stream_readable.js:176:18)
    at Socket.Readable.push (_stream_readable.js:134:10)
    at TCP.onread (net.js:547:20)

Verbose output also shows:

[firefox/index.js][debug] Firefox stdout: 1513425650621	addons.webextension.<unknown>	ERROR	Loading extension 'undefined': Reading manifest: Value for "default_locale" property must correspond to a directory in "_locales/". Not found: "_locales/en/"
This looks like the expected behavior, unless you have a _locales/en/ directory in your extension.
Source for notify-link-clicks-i18n is at
https://github.com/mdn/webextensions-examples/tree/master/notify-link-clicks-i18n

_locales/en/ does exist.
That add-on works as expected for me.
That add-on work for me.
That add-on works for me. Does your local cope of it have a _locales/en directory, matching what's on github? Are those files readable by Firefox?
Flags: needinfo?(javier--Lx3u0NnX4Eug3hq7uRufuMOjDIwrH7)
>Does your local cope of it have a _locales/en directory, matching what's on github?

Yes.

> Are those files readable by Firefox?

Yes.

I have asked if this bug is specific to Debian (Linux) at
https://bugs.debian.org/884640
Flags: needinfo?(javier--Lx3u0NnX4Eug3hq7uRufuMOjDIwrH7)
Thanks Javier. But at this point we can't reproduce this problem, its more likely something to do with the platform the user is running or local to their install. If we get more information we can re-open.
Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Resolution: --- → WORKSFORME
Attached patch Fix the DT_UNKNOWN case. (obsolete) — Splinter Review
This bug is specific to Linux. The problem is that directories under _locales are not detected as directories. This happens because code relies on d_type, but does not check the DT_UNKNOWN case. Please consider applying this patch.
Nice work tracking that down, sorry for not making the connection sooner but this looks like bug 1343572.
Attachment #8938453 - Flags: review?(dteller)
Re-opening since there's a patch on this one, although it might not be needed if bug 1343572 is used instead.
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: WORKSFORME → ---
Summary: Internationalized extensions are broken → Problems reading _locales directories on Linux
Comment on attachment 8938453 [details] [diff] [review]
Fix the DT_UNKNOWN case.

Review of attachment 8938453 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, this makes sense. However, rather than hardcoding that DT_UNKNOWN is 0, we should rather export DT_UNKNOWN using OSFileConstants.cpp and then reuse it inside osfile_unix_front.jsm. Could you do this?
Attachment #8938453 - Flags: review?(dteller) → feedback+
Flags: needinfo?(javier--Lx3u0NnX4Eug3hq7uRufuMOjDIwrH7)
Duplicate of this bug: 1343572
Attached patch Fix the DT_UNKNOWN case (2) (obsolete) — Splinter Review
DT_UNKNOWN is in OSFileConstants.cpp already. Is this new patch right?
Attachment #8938453 - Attachment is obsolete: true
Flags: needinfo?(javier--Lx3u0NnX4Eug3hq7uRufuMOjDIwrH7) → needinfo?(dteller)
Comment on attachment 8938677 [details] [diff] [review]
Fix the DT_UNKNOWN case (2)

Review of attachment 8938677 [details] [diff] [review]:
-----------------------------------------------------------------

Please don't touch mod.rs, it's for something entirely unrelated.

Also, have you already checked whether your code passes the osfile tests?

After rebuilding, you can do this through

./mach test toolkit/components/osfile/tests/xpcshell/

::: firefox-esr-52.5.2esr.orig/toolkit/components/osfile/modules/osfile_unix_front.jsm
@@ +772,4 @@
>           }
>  
>           let isDir, isSymLink;
> +         if (!("d_type" in contents) || contents.d_type == Const.DT_UNKNOWN) {

Let's first check that "DT_UNKNOWN" is defined (that's not the case under mingw32, which shouldn't be a proble, but just in case).

So, something along the lines of

if (!("d_type" in contents) || ("DT_UNKNOWN" in Const && contents.d_type == Const.DT_UNKNOWN))
Attachment #8938677 - Flags: feedback+
Attached patch Fix the DT_UNKNOWN case (3) (obsolete) — Splinter Review
This patch checks that "DT_UNKNOWN" is defined, although this was assumed by the "else" block. osfile tests pass.
Attachment #8938677 - Attachment is obsolete: true
Comment on attachment 8938716 [details] [diff] [review]
Fix the DT_UNKNOWN case (3)

Review of attachment 8938716 [details] [diff] [review]:
-----------------------------------------------------------------

Why would `!("DT_UNKNOWN" in Const)` be sufficient to ensure that it's a directory?
I am not sure about your question. `!("DT_UNKNOWN" in Const)` does not check whether it is directory, but it checks whether to run the lstat test. On the other hand, the "else" block assumes that "DT_UNKNOWN" is defined because it assumes that "DT_DIR" is defined too.
No, my bad, I misread your code. Could you amend the comment inside the then branch to make sure that it still makes sense?
Flags: needinfo?(dteller) → needinfo?(javier--Lx3u0NnX4Eug3hq7uRufuMOjDIwrH7)
Comment updated.
Attachment #8938716 - Attachment is obsolete: true
Flags: needinfo?(javier--Lx3u0NnX4Eug3hq7uRufuMOjDIwrH7) → needinfo?(dteller)
Comment on attachment 8938895 [details] [diff] [review]
Fix the DT_UNKNOWN case (4)

Review of attachment 8938895 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. Do you have the rights to land this?
Attachment #8938895 - Flags: review+
Flags: needinfo?(dteller)
> Do you have the rights to land this?

I do not have any commit access level. Do I need it?
You may want to request it if you have the intention of writing additional patches. I'll handle this one.

Regardless, thanks for the patch :)
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bd466716765
Fix problems reading _locales directories on Linux. r=Yoric
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5bd466716765
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(dteller)
Flags: needinfo?(dteller) → needinfo?(javier--Lx3u0NnX4Eug3hq7uRufuMOjDIwrH7)
I do not think manual testing is necessary. I cannot set "qe-verify-", though.
Flags: needinfo?(javier--Lx3u0NnX4Eug3hq7uRufuMOjDIwrH7)
Flags: qe-verify-
Assignee: nobody → javier--Lx3u0NnX4Eug3hq7uRufuMOjDIwrH7
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.