Closed Bug 1019113 Opened 11 years ago Closed 11 years ago

l10n.js is evaluated multiple times

Categories

(Firefox OS Graveyard :: Gaia::Contacts, defect)

Product:
Firefox OS Graveyard
Component:
Gaia::Contacts
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: stas, Assigned: mbudzynski)

References

Details

Attachments

(1 file)

19915.html
(deleted), text/html
arcturus
: review+
stas
: feedback+
Details
There are weird "L10nError: Context not ready" exceptions in Contacts in DEBUG=1 profiles in Nightly. I've been trying to track them down and it appears that they're caused by the fact that the l10n.js script is evaluated multiple times when certain HTMLs are imported. The following commit https://github.com/mozilla-b2g/gaia/commit/894c2743e14f7897b2671327895537e576370ccc#diff-2 added three imports in index.html: <link rel="import" href="/shared/pages/import/oauth.html"> <link rel="import" href="/shared/pages/import/curtain.html"> <link rel="import" href="/shared/pages/import/import.html"> The problem, AIUI, is that these HTML file a not web-component-like <element>s, but instead, full-featured HTML documents, with their own <head>, <script>s and <body>. I'll get to that in a sec. I wasn't able to find out how these imports are loaded. I suspect that somewhere something lazy-loads a valid node, which is supposed to be replaced by a valid element import. However, the way imports are implemented, the loader will look for all <link rel="import">s in the document, even if you try to load just one. https://github.com/mozilla-b2g/gaia/blob/master/shared/js/lazy_loader.js#L48 So it looks like HtmlImport.populate() might be responsible for loading these three pages above. It's just a theory though, because I wasn't able to pin point the exact line which triggers the lazy loading and also any console.logs in HtmlImport.populate remain silent. Maybe there's an addon in tools/extension which does that in DEBUG=1 builds? Now the fun/bizarre/scary part. These three imports make the parent document's l10n.js to be re-evaluated for some reason. It creates a new navigator.mozL10n, which, of course, is not ready at all. So it looks like the code follows the following path: - mozL10n.once calls init methods - something calls LazyLoader.load on a node - LazyLoader's _html calls HtmlImport.populate - which loads all imports, also those in /shared/pages - which re-evaluates l10n.js in index.html and overwrites navigator.mozL10n, which is now not ready, even though this all happened in a callback to mozL10n.once! I'm not sure what purpose those three imports serve. I naïvely tried to remove them, which fixed the issue at hand, but seems to break other things. Also note that it looks like those three files are loaded anyways via fbLoader: https://github.com/mozilla-b2g/gaia/blob/894c2743e14f7897b2671327895537e576370ccc/apps/communications/contacts/js/fb_loader.js
STR: 1. DEBUG=1 make 2. put the following line at the top of shared/js/l10n.js: console.log('L10n.js is being evaluated now ' + document.documentURI); 3. firefox -profile profile-debug 4. launch Contacts Expected results: "L10n.js is being evaluated now http://communications.gaiamobile.org:8080/contacts" Actual results: "L10n.js is being evaluated now http://communications.gaiamobile.org:8080/contacts" "L10n.js is being evaluated now http://communications.gaiamobile.org:8080/contacts" "L10n.js is being evaluated now http://communications.gaiamobile.org:8080/contacts" "L10n.js is being evaluated now http://communications.gaiamobile.org:8080/contacts" Removing the three imports mentioned in comment 0 remedies the problem.
Note: The high memory footprint of l10n.js that we saw in bug 914414 might be related to this.
(In reply to Staś Małolepszy :stas from comment #0) > <link rel="import" href="/shared/pages/import/oauth.html"> > <link rel="import" href="/shared/pages/import/curtain.html"> > <link rel="import" href="/shared/pages/import/import.html"> > > The problem, AIUI, is that these HTML file a not web-component-like > <element>s, but instead, full-featured HTML documents, with their own > <head>, <script>s and <body>. > > I wasn't able to find out how these imports are loaded. Kevin, can you help me understand how these HTMLs are imported in DEBUG=1 profiles? Is it done automatically? I only found LazyLoader's _html method but it looks like it's never called in Contacts.
Flags: needinfo?(kgrandon)
I don't think that these should be listed as an import, so I'm not sure why they would be included here. Forwarding the ni? to Jose. Jose - can you let use know why these are needed as an import? If not, let's remove them.
Flags: needinfo?(kgrandon) → needinfo?(jmcf)
referencing these files through a link is only needed to ensure they are copied to the app package. So probably would be enough to reference them commented or without any "rel" attribute. Michal was going to check that. Michal can you tell us something about if that is the solution, thanks!
Flags: needinfo?(jmcf) → needinfo?(mbudzynski)
When we comment those files in the header everything works as expected. This should be fixed by a patch prepared for Bug 1018819.
Flags: needinfo?(mbudzynski)
What has to do this bug with bug 1018819? Let's create different patches for different issues
This bug was a follow up found while working on 1018819 so I don't see a point in creating 2 lines exclusive patch just for it and waste time for reviews - this will block the whole Haidification schedule we have for this week. Trivial changes like this one already landed as parts of other patches in Gaia history.
Well, I disagree, I prefer to keep changes tracked and controlled
I side with Jose here, let's get this two liner because it may impact memory consumption numbers and potentially perf as well. Contacts have always been an outlier and I suspect it may have been due to this bug. Isolating it will make it easier to bisect and also back out if needed.
Francisco, what do you think?
Flags: needinfo?(francisco)
Assignee: nobody → mbudzynski
(In reply to Zibi Braniecki [:gandalf] from comment #10) > I side with Jose here, let's get this two liner because it may impact memory > consumption numbers and potentially perf as well. Contacts have always been > an outlier and I suspect it may have been due to this bug. > > Isolating it will make it easier to bisect and also back out if needed. oh absolutely
Attached file 19915.html (deleted) —
Stas, Please could you verify this solves the issue? thanks!
Attachment #8433081 - Flags: review?(francisco)
Attachment #8433081 - Flags: feedback?(stas)
Thanks Jose Manuel for working on this without assigning yourself or notifying anyone you plan to do it.
Blocks: 1018819
(In reply to Michał Budzyński (:michalbe) from comment #14) > Thanks Jose Manuel for working on this without assigning yourself or > notifying anyone you plan to do it. It was such a tiny patch that I did not consider it necessary ...
Attachment #8433081 - Flags: review?(francisco) → review+
Flags: needinfo?(francisco)
Comment on attachment 8433081 [details] 19915.html Yes, thanks!
Attachment #8433081 - Flags: feedback?(stas) → feedback+
https://github.com/mozilla-b2g/gaia/commit/bf90232f4b0233a7a3ff43594f83afaa0eff0972
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: