Extra microtask is pushing fluent translation from InitialLayout to DOMContentLoaded

RESOLVED FIXED in Firefox 60

Status

()

defect
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

(Blocks 1 bug)

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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.
(Assignee)

Updated

a year ago
Blocks: 1365426
(Assignee)

Updated

a year ago
Priority: -- → P3
(Assignee)

Comment 1

a year ago
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
(Assignee)

Comment 2

a year ago
Here's a version with sync callback that does not push us to after layout.
(Assignee)

Comment 3

a year ago
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?
Flags: needinfo?(bugs)
Our Promises don't use microtasks.
(See the too long delayed bug 1193394)
Flags: needinfo?(bugs)
(Assignee)

Updated

a year ago
Depends on: 1193394
(Assignee)

Updated

a year ago
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 6

a year ago
mozreview-review
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+
Comment hidden (mozreview-request)

Comment 8

a year ago
Pushed by zbraniecki@mozilla.com:
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!
Flags: needinfo?(alessio.placitelli)
(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 [1] is failing for some reason. Maybe just comment out [2] and see if the problem is still there. My suspicion is that your patch makes the window not clear up properly in that case.

[1] - https://searchfox.org/mozilla-central/rev/9d47b5fb14152865ad1375eb3ee2e571d81ecdb9/browser/base/content/test/general/browser_datachoices_notification.js#205
[2] - https://searchfox.org/mozilla-central/source/browser/base/content/test/general/browser_datachoices_notification.js#161-215
Flags: needinfo?(alessio.placitelli)
(Assignee)

Updated

a year ago
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. :(
(Assignee)

Updated

a year ago
Depends on: 1438055
Comment hidden (mozreview-request)
Switching to weak ref observers fixes the issue. I filed bug 1438055 to make the switch and then we can land this patch.
Comment hidden (mozreview-request)

Comment 17

a year ago
mozreview-review
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+

Comment 18

a year ago
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/780f43334807
Workaround promise/microtask bug with a callback in Fluent runtime. r=Pike

Comment 19

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/780f43334807
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
(Assignee)

Updated

a year ago
See Also: → 1455048
You need to log in before you can comment on or make changes to this bug.