1505750 - When resizing about:performance quickly, every once in a while a frame is painted without localizable strings

Status: RESOLVED FIXED

(Toolkit :: Performance Monitoring, defect)

Description

[Florian Quèze [:florian]]

Attachment: Screenshot of a frame without Fluent strings

I noticed that about:performance sometimes flickers when resizing the window. Using a screen recording, I saw that the flickers is caused by frames being painted without the localizable strings. See attached screenshot.

Comment 1

[Florian Quèze [:florian]]

Attachment: Screenshot of the next frame

Comment 2

[Florian Quèze [:florian]]

If I had to make a guess about what's going on, I would say that Fluent is replacing strings whenever the refresh driver ticks, and that sometimes we paint outside of refresh driver ticks due to receiving resize events from the OS.

Comment 3

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

We translate mutations in rAF - https://searchfox.org/mozilla-central/source/intl/l10n/DOMLocalization.jsm#650-664

Is that what you're talking about? Is there a better way to cluster translations to happen once per animation frame?

Comment 4

[Florian Quèze [:florian]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #3)
> We translate mutations in rAF -
> https://searchfox.org/mozilla-central/source/intl/l10n/DOMLocalization.
> jsm#650-664
> 
> Is that what you're talking about? Is there a better way to cluster
> translations to happen once per animation frame?

I suspect we don't have an animation frame when the paint is triggered by a resize notification received from the OS rather than by the refresh driver.

For this specific bug, it looks like I could just call document.l10n.translateFragment near https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/toolkit/components/aboutperformance/content/aboutPerformance.js#1020 but if possible I think it would be better to address the problem more generally, as you may see other instances of the same problem when other pieces of the UI are resized.

Comment 5

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

I agree we should try to tackle it in general.

So, what I'm confused is why is there a retranslation happening when you resize the window? Or is it just a coincidence that resize happens as translation happens, and in result the paint from resize happens before the paint from l10n?

Comment 6

[Florian Quèze [:florian]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #5)

> So, what I'm confused is why is there a retranslation happening when you
> resize the window? Or is it just a coincidence that resize happens as
> translation happens, and in result the paint from resize happens before the
> paint from l10n?

The latter. The about:performance table is refreshed (and so the DOM is rebuilt and retranslated) every 2s. When we refresh while the user is resizing, I think it's a matter of luck whether the next paint will be from the refresh driver (in that case strings will be correct) or from a resize event (in that case the strings will be missing for one frame).

Comment 7

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] from comment #4)

> For this specific bug, it looks like I could just call
> document.l10n.translateFragment near
> https://searchfox.org/mozilla-central/rev/
> 17f55aee76b7c4610a974cffd3453454e0c8de7b/toolkit/components/aboutperformance/
> content/aboutPerformance.js#1020

Thinking about it some more, I could also do the DOM update from a rAF callback.

Comment 8

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

ok. so we know why. I have no idea what's the right way to solve it.

MDN doc says:

```
The window.requestAnimationFrame() method tells the browser that you wish to perform an animation and requests that the browser call a specified function to update an animation **before the next repaint**.
```

Does it mean its a bug in rAF implementation in Gecko?

Comment 9

[Florian Quèze [:florian]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #8)
> ok. so we know why. I have no idea what's the right way to solve it.
> 
> MDN doc says:
> 
> ```
> The window.requestAnimationFrame() method tells the browser that you wish to
> perform an animation and requests that the browser call a specified function
> to update an animation **before the next repaint**.
> ```
> 
> Does it mean its a bug in rAF implementation in Gecko?

Assuming my guess in comment 2 is correct, then either there's the bug in Gecko's rAF implementation, or the MDN doc is misleading (eg. it could be that the intended behavior of rAF is to target 60 calls per second, and skip additional calls, as they shouldn't affect the user perception of the animation smoothness).

Comment 10

[Florian Quèze [:florian]]

Attachment: Reduced test case

Comment 11

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

Hi Boris,

Can you advise here? We're using rAF to coalesce localizations trigger by mutations so that they happen once per paint.

Florian noticed that when resizing paints can happen independently skipping rAF which results in paints without translations.

In particular, the minimized testcase shows that the "red" background can result in a paint when the user resizes the window to be larger, and the rAF guard which switches to green will not happen then.

Is that a bug in our implementation of rAF, or is it per design and we should accept that paints can happen independently?

Comment 12

[Boris Zbarsky [:bzbarsky]]

I am not up to date on how that stuff works nowadays.

It does look like nsView::WillPaintWindow can get called from widget code, plausibly from resizes, and trigger painting.   I don't know how or whether that interacts with vsync, which is what drives the refresh driver.

Matt, do you know offhand, or know who might know?

Comment 13

[Matt Woodrow (:mattwoodrow)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #12)
> I am not up to date on how that stuff works nowadays.
> 
> It does look like nsView::WillPaintWindow can get called from widget code,
> plausibly from resizes, and trigger painting.   I don't know how or whether
> that interacts with vsync, which is what drives the refresh driver.
> 
> Matt, do you know offhand, or know who might know?

The details are platform-specific, but we generally do call WillPaintWindow to force an immediate out-of-band paint when resizing so that the OS doesn't end up presenting our Window with uninitialized contents.

This is independent of the vsync-triggered refresh driver update and paint cycle, and doesn't notify refresh observers (like rAF).

I'm less sure about whether this behaviour is buggy wrt the rAF spec though and the rAF spec [1] doesn't mention anything about this. The Web App APIs spec [2] defines painting from an event loop that seems to require firing rAF before an update, but doesn't really define what to do when we need to update the rendering from an external trigger.

I've tried in the past to make us flush more from the resize paint (to fix an issue with async animations) and encountered a lot of test failures, so this probably isn't super trivial to change from the gecko end.


[1] https://html.spec.whatwg.org/multipage/imagebitmap-and-animations.html#animation-frames
[2] https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-model

Comment 14

[Florian Quèze [:florian]]

Attachment: non-working WIP - attempt to use requestAnimationFrame

I tried the approach I suggested in comment 7, and the result is consistent flicker even when not resizing. I think this is because the Fluent code calls rAF before my code, and so is called first. I'm attaching this so that you can try it, as I'm afraid any browser UI using rAF would encounter the same problem.

Comment 15

[Florian Quèze [:florian]]

Attachment: Use the Fluent API directly

This worked well in my local testing but feels like wallpapering over the problem.

Comment 16

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

Continue the investigation.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/69de0ff59928
Use the Fluent API directly to avoid flicker when resizing the window, r=gandalf.

Comment 18

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/69de0ff59928

Comment 19

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

I can't reproduce it anymore.

Florian, could you try if you can reproduce it somehow? Over the recent year we migrated all of the DOM bindings for Fluent to C++ and we use the internal mutation observer with refresh driver so my hope is that the mitigations are not needed anymore and it won't be a papercut in the future.

Comment 20

[Florian Quèze [:florian]]

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #19)

I can't reproduce it anymore.

Florian, could you try if you can reproduce it somehow? Over the recent year we migrated all of the DOM bindings for Fluent to C++ and we use the internal mutation observer with refresh driver so my hope is that the mitigations are not needed anymore and it won't be a papercut in the future.

In about:processes I removed the mitigations as part of bug 1705827, because we no longer re-display the entire table, so in the worst case we would have only a few blank lines, not a flicker of the entire table.

I don't think using the mutation observer with the refresh driver would help, as the source of the problem was that the refresh driver code path isn't used when a synchronous repaint is triggered by a resize event coming from the OS.

Comment 21

[Florian Quèze [:florian]]

Also note that the reduced test case in attachment 9023798 [details] still produces red flashes for me when resizing the window.

Comment 22

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

Also note that the reduced test case in attachment 9023798 [details] still produces red flashes for me when resizing the window.

Interesting. I can't repro (linux).

I don't think using the mutation observer with the refresh driver would help, as the source of the problem was that the refresh driver code path isn't used when a synchronous repaint is triggered by a resize event coming from the OS.

I see. I'm not sure how to make this information actionable. Do you have any suggestion? Otherwise I'm afraid this may be an inherent limitation of rAF and Fluent DOM relying on it.