59 bytes, text/x-review-board-request
While profiling Preferences window with a bunch of Fluent from bug 1424682 and bug 1424682 I noticed something weird. The intention for this code is to wait till the DOM is ready, but before layout and then alter the DOM with translations. But what we do in this code is actually use a Promise to react to the `BeforeInitialXULLayout` and only in `then` do we perform the translation. This unfortunately pushes the translation to past DOMContentLoaded, so past layout and past frame creation I believe. Here's a profile with added markers: https://perfht.ml/2El2F0f You can see that the BeforeInitialXULLayout event happens at 1.969 but the `translation` mark only is fired at 2.039 which is right at the end of DOMContentLoaded. Here's a different profile, with the callack moved to be fired in the same microtask: https://perfht.ml/2ETw96l Here as you can see both BeforeInitialXULlayout and translation happen at 1.747.
To reproduce: 1) Replace l10n.js in intl/l10n with this one 2) rebuild intl/l10n 3) Open Firefox with the Profiler 4) Start profiler 5) Open Preferences
Here's a version with sync callback that does not push us to after layout.
Olli - do you know if bug 1193394 will change anything here? (for example by making the task complete all microtasks within BeforeInitialXULLayout and thus not push translation to post DOMContentLoaded?
Our Promises don't use microtasks. (See the too long delayed bug 1193394)
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Comment on attachment 8950314 [details] Bug 1437427 - Workaround promise/microtask bug with a callback in Fluent runtime. https://reviewboard.mozilla.org/r/219554/#review225316 ::: intl/l10n/l10n.js:9 (Diff revision 1) > > /** > * Polyfill for document.ready polyfill. > * See: https://github.com/whatwg/html/issues/127 for details. > * > + * XXX: The callback is a temporary workaround for bug 1193394. Once Promise in Gecko nit, `Promises` in plural
Attachment #8950314 - Flags: review?(l10n) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/b3104c135520 Workaround promise/microtask bug with a callback in Fluent runtime. r=Pike
Work around one thing, and another pops up. Backed out in https://hg.mozilla.org/integration/autoland/rev/d9317a029044 for https://treeherder.mozilla.org/logviewer.html#?job_id=161838699&repo=autoland (intermittent, but about a 90% intermittent on the platforms where I retriggered enough to get a feeling about frequency).
Alessio, you're the author of the test in bug 1191336. Can you help me understand how this patch may cause a leak in it? I'm mainly trying to decide if: - your test is the only one testing leaking of Preferences window and my patch somehow does add a leak - your test is doing something wrong and my patch exposes it - neither of us can understand what's going on and we need to get help debugging it Thanks!
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #10) > Alessio, you're the author of the test in bug 1191336. Can you help me > understand how this patch may cause a leak in it? Sure, let's see if I can :) > I'm mainly trying to decide if: > > - your test is the only one testing leaking of Preferences window and my > patch somehow does add a leak No, my test is trying to make sure that we show a "data choices notification" bar, if needed, in the browser window (and also if we're using multiple windows). The data choices thing is that bar that appears on new profiles to let users know about data collection. > - your test is doing something wrong and my patch exposes it Could be :) But I'm not sure what and why would your patch expose it. The first thing I would try is to check which one of the two tests in that file is causing the problem, by commenting the other one. I would suspect  is failing for some reason. Maybe just comment out  and see if the problem is still there. My suspicion is that your patch makes the window not clear up properly in that case.  - https://searchfox.org/mozilla-central/rev/9d47b5fb14152865ad1375eb3ee2e571d81ecdb9/browser/base/content/test/general/browser_datachoices_notification.js#205  - https://searchfox.org/mozilla-central/source/browser/base/content/test/general/browser_datachoices_notification.js#161-215
Attachment #8950314 - Attachment is obsolete: true
A try build reproducing the leak: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8a7e56b3256&selectedJob=162041634 I could not reproduce it locally. either by launching just the test, or by testing the whole `general/`. :mconley suggests asking ekarhm or mccr8 or just evolving the patch and pushing to try until it stops leaking. :(
Switching to weak ref observers fixes the issue. I filed bug 1438055 to make the switch and then we can land this patch.
Comment on attachment 8950711 [details] Bug 1437427 - Workaround promise/microtask bug with a callback in Fluent runtime. https://reviewboard.mozilla.org/r/219958/#review226296
Attachment #8950711 - Flags: review?(l10n) → review+
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/780f43334807 Workaround promise/microtask bug with a callback in Fluent runtime. r=Pike
You need to log in before you can comment on or make changes to this bug.