1125183 - Exception: TypeError: meta.availableLanguages is not an object

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::L10n, defect, P1)

Description

[Michael Vines [:m1] [:evilmachines]]

+++ This bug was initially created as a clone of Bug #1115796 +++

Bug 1115796 seems to have broken both our master and v2.2 builds with a:

Exception: TypeError: meta.availableLanguages is not an object
initResources@resource://gre/modules/commonjs/toolkit/loader.js -> file:///build/v2.2/build/gaia/build/utils-xpc.js -> file:///build/v2.2/build/gaia/build/l10n.js?d=1422011516614:76:1
navigator.mozL10n.bootstrap@resource://gre/modules/commonjs/toolkit/loader.js -> file:///build/v2.2/build/gaia/build/utils-xpc.js -> file:///build/v2.2/build/gaia/build/l10n.js?d=1422011516614:43:5
HTMLOptimizer.prototype.process@resource://gre/modules/commonjs/toolkit/loader.js -> file:///build/v2.2/build/gaia/build/webapp-optimize.js:71:3
WebappOptimize.prototype.processFile@resource://gre/modules/commonjs/toolkit/loader.js -> file:///build/v2.2/build/gaia/build/webapp-optimize.js:632:3
WebappOptimize.prototype.execute@resource://gre/modules/commonjs/toolkit/loader.js -> file:///build/v2.2/build/gaia/build/webapp-optimize.js:699:3
execute@resource://gre/modules/commonjs/toolkit/loader.js -> file:///build/v2.2/build/gaia/build/webapp-optimize.js:729:1
exports.execute@resource://gre/modules/commonjs/toolkit/loader.js -> file:///build/v2.2/build/gaia/build/post-app.js:24:3
buildApps/<@resource://gre/modules/commonjs/toolkit/loader.js -> file:///build/v2.2/build/gaia/build/app.js:100:7
buildApps@resource://gre/modules/commonjs/toolkit/loader.js -> file:///build/v2.2/build/gaia/build/app.js:56:3
exports.execute@resource://gre/modules/commonjs/toolkit/loader.js -> file:///build/v2.2/build/gaia/build/app.js:148:3
CommonjsRunner.prototype.run@/build/v2.2/build/gaia/build/xpcshell-commonjs.js:169:5
run@/build/v2.2/build/gaia/build/xpcshell-commonjs.js:184:3
@-e:1:1
make[1]: *** [app] Error 1
make[1]: Leaving directory `/build/v2.2/build/gaia'
make: *** [gaia/profile.tar.gz] Error 2
make: *** Waiting for unfinished jobs....

---

Where does meta.availableLanguages come from?

Comment 1

[Michael Vines [:m1] [:evilmachines]]

Probably the wrong way to avoid this exception:

diff --git a/build/l10n.js b/build/l10n.js
index 982e181..fc46718 100644
--- a/build/l10n.js
+++ b/build/l10n.js
@@ -72,8 +72,10 @@
       document.documentElement.dataset.noCompleteBug = true;
     }
 
-    this.ctx.registerLocales(meta.defaultLanguage,
-                             Object.keys(meta.availableLanguages));
+    if (meta.availableLanguages) {
+      this.ctx.registerLocales(meta.defaultLanguage,
+                               Object.keys(meta.availableLanguages));
+    }
   }

Comment 2

[Staś Małolepszy :stas]

I think a possibly better way to avoid this would be to create empty properties of the meta object here:

https://github.com/mozilla-b2g/gaia/blob/c322468a7fc9e12a59774d34147f9c1754f1bf30/shared/js/l10n.js#L1638

I seems to me that for some reason this line is not run and meta.availableLanguages stays undefined:

https://github.com/mozilla-b2g/gaia/blob/c322468a7fc9e12a59774d34147f9c1754f1bf30/shared/js/l10n.js#L1731

Comment 3

[Michael Vines [:m1] [:evilmachines]]

Would it help if I attached the output from the full gaia build?  or something else?  Thanks!

Comment 4

[Staś Małolepszy :stas]

Do you know which app is being processed when this error occurs?  WebappOptimize or HTMLOptimizer might have the reference to the HTML file in question.

Comment 5

[Michael Vines [:m1] [:evilmachines]]

(In reply to Staś Małolepszy :stas from comment #4)
> Do you know which app is being processed when this error occurs? 

I'll check in a bit, but that's brings up a possible lead.  We have some outoftree/ [1] apps in our build.  Does bug 1115796 (or some other recent l10n change on master/v2.2) assume something new of apps?


[1] https://github.com/mozilla-b2g/gaia/blob/b07dc29eb/Makefile#L427

Comment 6

[Staś Małolepszy :stas]

On buildtime, each app is run through build/multilocale.js which extracts the information about the languages the app is available in from LOCALES_FILE (typically shared/resources/languages.json) and puts it in a <meta name="availableLanguages" …> for fast sync access.

One potential troublemaker might be that in bug 1115807, we assumed every app would already have that <meta> element present.  In that very bug Zibi added the required <meta> elements to all HTML files in Gaia:

  https://github.com/mozilla-b2g/gaia/commit/8ef355889432359407039bf2e1dc69106979d7cd

It's possible that an outoftree app doesn't have it defined and the querySelector in 

  https://github.com/mozilla-b2g/gaia/commit/8ef355889432359407039bf2e1dc69106979d7cd#diff-e4109b329f82ce3c6b17eff9a10ed670R230

returns null.  Zibi, does this sound like a possible cause of the failure?

Comment 7

[Staś Małolepszy :stas]

Attachment: Patch 1

Michael, can you test if the error persists with this patch?

It would be helpful to get the source of one of the outoftree apps.  Can you confirm that index.html does not contain <meta name="availableLanguages" …>?

Comment 8

[Staś Małolepszy :stas]

PR: https://github.com/mozilla-b2g/gaia/pull/27642

Comment 9

[Michael Vines [:m1] [:evilmachines]]

(In reply to Staś Małolepszy :stas from comment #7)
>
> It would be helpful to get the source of one of the outoftree apps.

Sure, here's one - https://github.com/mozilla-b2g/firefoxos-loop-client

> Can you confirm that index.html does not contain <meta name="availableLanguages" …>?

Yes, there's nothing like this in these apps - https://github.com/mozilla-b2g/gaia/commit/00f8a78ad81

Comment 10

[Michael Vines [:m1] [:evilmachines]]

I still hit the same exception with attachment 8553872 [details] [diff] [review].  Looks like l10n.js is trying to process /build_stage/loop/app/call_screen/call.html [1] at the time (although we have several other outoftree apps as well).

[1] https://github.com/mozilla-b2g/firefoxos-loop-client/blob/1.1/app/call_screen/call.html

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

The problem is deeper. Localization will *not* work for apps like https://github.com/mozilla-b2g/firefoxos-loop-client in 2.2 or master they still link to the manifest.

Yes, we can sugarcoat the error to stop breaking the build, but the outoftree apps still need to be updated.
It's as simple as replacing:

https://github.com/mozilla-b2g/firefoxos-loop-client/blob/a38d96add0e7d5f9efd677f80a27a258a056c5e4/app/index.html#L41

with

<meta name="defaultLanguage" content="en-US">
<meta name="availableLanguages" content="de, en-US, es, it">

(the locales are taken from https://github.com/mozilla-b2g/firefoxos-loop-client/tree/master/app/locales )

So, I would even advice against fixing this bug, because if we fix it we will have to wait for l10n QA tests to report that those apps don't work.

I would suggest we instead place a warning for apps that link l10n.js and manifest.webapp stating that missing meta.availableLanguages and defaultLocale will break localizability.

Comment 12

[Staś Małolepszy :stas]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #10)
> I still hit the same exception with attachment 8553872 [details] [diff] [review]
> [review].  Looks like l10n.js is trying to process
> /build_stage/loop/app/call_screen/call.html [1] at the time (although we
> have several other outoftree apps as well).

I linked firefoxos-loop-client/build in outoftree/firefoxos-loop-client and ran make profile.  I didn't see any error and I can confirmed that with my patch the resulting HTML as the required <meta> elements.

I'm not sure what might be causing your setup to fail :(

Comment 13

[Michael Vines [:m1] [:evilmachines]]

(In reply to Zibi Braniecki [:gandalf] from comment #11)
>
> So, I would even advice against fixing this bug, because if we fix it we
> will have to wait for l10n QA tests to report that those apps don't work.
> 
> I would suggest we instead place a warning for apps that link l10n.js and
> manifest.webapp stating that missing meta.availableLanguages and
> defaultLocale will break localizability.

Not all outoftree apps need to be localized, some are commercial apps (like the loop app) but others are just test apps and having to deal with l10n in them is an unnecessary burden.

If we can't get a quick fix/workaround here for v2.2 then can we back out bug 1115796 from v2.2 today please, while we figure out the best path.  The "[Risk to taking this patch]" field at bug 1115796 comment 15 did promise no regressions ;)

Comment 14

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Zibi Braniecki [:gandalf] from comment #11)
> The problem is deeper. Localization will *not* work for apps like
> https://github.com/mozilla-b2g/firefoxos-loop-client in 2.2 or master they
> still link to the manifest.
> 
> Yes, we can sugarcoat the error to stop breaking the build, but the
> outoftree apps still need to be updated.

Actually, I'm wrong.

If we fix the build system to generate the <meta> it will start working properly since at build time we will update languages list.

The reason for my comment was because today the localizability of those apps don't work. My patch in bug 1115796 just exposed the bug that was there before since landing of bug 1115807.

--

Bottom line is - I suggest we land Stas's patch (once it starts working for Michael), but we add a warning if manifest.webapp is linked and metas are not present.

Comment 15

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #13)
> If we can't get a quick fix/workaround here for v2.2 then can we back out
> bug 1115796 from v2.2 today please, while we figure out the best path.  The
> "[Risk to taking this patch]" field at bug 1115796 comment 15 did promise no
> regressions ;)

Bug 1115796 did not introduce a regression. It exposed a bug in the outoftree app that has been there since bug 1115807 landed.

In result, backing out bug 1115796 will just hide the error and you will discover that apps like loop are not working in any other locale once you start QA.

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #10)
> I still hit the same exception with attachment 8553872 [details] [diff] [review]
> [review].  Looks like l10n.js is trying to process
> /build_stage/loop/app/call_screen/call.html [1] at the time (although we
> have several other outoftree apps as well).
> 
> [1]
> https://github.com/mozilla-b2g/firefoxos-loop-client/blob/1.1/app/
> call_screen/call.html

Michael, can you provide me STR for building loop?

I have gaia repository, I can clone loop client repo, but I'm not sure how to link it and how you build gaia with that.

Comment 17

[Michael Vines [:m1] [:evilmachines]]

(In reply to Zibi Braniecki [:gandalf] from comment #15)
> Bug 1115796 did not introduce a regression. It exposed a bug in the
> outoftree app that has been there since bug 1115807 landed.
> 
> In result, backing out bug 1115796 will just hide the error and you will
> discover that apps like loop are not working in any other locale once you
> start QA.

Sure, I'm not saying that we don't try to fix this properly.  The point is that the patch from bug 1115796 turned our v2.2 tree red.  Put another way, if bug 1115796 caused inbound or m-c/master to be red it would have been backed out immediately.  That's the situation we are in here.  I don't mind keeping out master branch red while we figure this out but not v2.2.

Comment 18

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #17)
> Sure, I'm not saying that we don't try to fix this properly.  The point is
> that the patch from bug 1115796 turned our v2.2 tree red.  Put another way,
> if bug 1115796 caused inbound or m-c/master to be red it would have been
> backed out immediately.

True. But that's what allows us to discover bugs like this before they hit master. We can't do that for outoftree apps, so we need to rely on people discovering it before it hit them.

On the other hand, would you prefer to wait for QA round later in the cycle to discover that your apps don't work properly or have a build error that forces you to fix them?

Once again, backing out will get your a green build but a broken app because you don't test for locale change scenario.

>  That's the situation we are in here.  I don't mind
> keeping out master branch red while we figure this out but not v2.2.

The fix is the same for both. I asked for STR, can you help me here? Stas says that his patch fixed it for loop for him. You say it still does not fix it for you. If you can provide me STR, I can try to reproduce your problem. Unfortunately outoftree apps using non-frozen APIs are hard to track and fix.

Of course, we can also fix loop by properly using meta instead of manifest, but you say that there are other outoftree apps that may be broken, and from what I understand I can't get a list of them so I can't test them and fix them properly.

So my plan right now is:
 - get STR from you
 - update Stas's patch to build <meta/> for apps that use obsolete HTML API
 - add a build time warning for those apps to remind them that they need to update

This will fix it for master and 2.2.

Comment 19

[Michael Vines [:m1] [:evilmachines]]

(In reply to Zibi Braniecki [:gandalf] from comment #16)
>
> Michael, can you provide me STR for building loop?

Try this: 

$ git clone https://github.com/mozilla-b2g/gaia.git
$ git clone https://github.com/mozilla-b2g/firefoxos-loop-client.git
$ export GAIA_OUTOFTREE_APP_SRCDIRS=$(readlink -f firefoxos-loop-client)
$ cd gaia
$ make

Comment 20

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: patch2.diff

m1: can you try this patch? It fixes the bug and reports a bold warning in the build log that encourages users to fix their code and explains how. :)

Comment 21

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: pull request

PR with the fixes.

Comment 22

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ok, so the PR makes us more careful and guards against using non-initialized attributes. It also uses multilocale.js to create missing ones and build/l10n.js to work properly even if they are not there.

I also filed a follow up bug 1125405 to update loop client to use the metas.

Comment 23

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8554005 [details] [review]
pull request

I need a rescue reviewer since Stas and Ricky are asleep+weekend.

Comment 24

[Kevin Grandon :kgrandon]

Comment on attachment 8554005 [details] [review]
pull request

I am told that there is a deadline here, so going to go ahead and leave an R+. (You should probably still get a review from a build peer, even if it's a retroactive one). I also recommend adding a test, or filing a follow-up bug to add a test if there's a reason why we can't write one today.

Comment 25

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8554005 [details] [review]
pull request

Retroactive r? on ricky and stas.

What this patch does is:

 - in build/l10n.js guard against missing metas. 
   - If defaultLanguage is missing, it will pass null and registerLocales will use ctx.defaultLocale.
   - If availableLanguages is missing, it will pass null and registerLocales will skip it.
 - in build/multilocale.js it:
   - adds missing <meta> tags
   - moves the early exist to before buildMetas and updates the check so that we don't add those tags for files that don't do l10n.js
   - warns user if their HTML file is using obsolete manifest.webapp.

Comment 26

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8554005 [details] [review]
pull request

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 1115807
[User impact] if declined: apps that didn't update to bug 1115807 will fail at build time
[Testing completed]: app that was failing builds now
[Risk to taking this patch] (and alternatives if risky): not known
[String changes made]: none

Comment 27

[Michael Vines [:m1] [:evilmachines]]

Comment on attachment 8554005 [details] [review]
pull request

This patch fixes all the things over here.  Thank you.

Comment 28

[Zibi Braniecki [:zbraniecki][:gandalf]]

Commit: https://github.com/mozilla-b2g/gaia/commit/b82bfe8641fc6cce817dc5a357e782c4c55479c9
Merge: https://github.com/mozilla-b2g/gaia/commit/6b922e0837de128d7f395ccfcbf015721f88cb2d

Comment 29

[Zibi Braniecki [:zbraniecki][:gandalf]]

L20n.js: https://github.com/l20n/l20n.js/commit/c9af6033322488d463454339d57f59ea21c22e56

Comment 30

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8554005 [details] [review]
pull request

LGTM.

Comment 31

[Staś Małolepszy :stas]

Comment on attachment 8554005 [details] [review]
pull request

r=me a posteriori.  Thanks, Zibi.

I'll echo what Kevin said above:  let's file a follow-up and add a test.  I think we could use a new test-l10n-* app in https://github.com/mozilla-b2g/gaia/tree/master/dev_apps for this.

Comment 32

[Zibi Braniecki [:zbraniecki][:gandalf]]

2.2: https://github.com/mozilla-b2g/gaia/commit/cc88f3c3ffaef71a15f34e16e5cc17f289683dcc

Comment 33

[Zibi Braniecki [:zbraniecki][:gandalf]]

As per bug 1125405 this was not a regression. Loop client should only be built on v2.0.

But it's still a good patch to guard against obsolete l10n API use for outoftree apps, so I'm glad we have it :)