1424823 - Opening Synced Tabs menu is too slow after the second time

Status: RESOLVED WORKSFORME

(Firefox :: Toolbars and Customization, defect)

Description

[magicp]

Attachment: library-synced-tabs.mp4

Steps to reproduce:
1. Launch the latest Nightly with a new profile
2. Navigate to Library button > Synced Tabs > Go back to Library > Synced Tabs again

Actual results:
After the second time, Opening Synced Tabs takes 3-4 seconds. It looks like freezing. But if clicking twice, it will be opened immediately.

Expected results:
Synced Tabs menu is opened quickly.

Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f249f5238ef931d68cd8ede3c3617825a06290c0&tochange=35425d2a0d903e361910194df47e829c590f3a2b

Comment 1

[:Gijs (he/him)]

Any idea what's going on here, Paolo?

Comment 2

[magicp]

(In reply to magicp from comment #0)
> Actual results:
> After the second time, Opening Synced Tabs takes 3-4 seconds. It looks like
> freezing. But if clicking twice, it will be opened immediately.

After the second time, Synced Tabs never open if mouse pointer stay inside menu.

Comment 3

[:Paolo Amadini]

We are waiting for a layout flush, but the observer never gets called.

If we know a layout flush will not happen, promiseLayoutFlushed should return immediately:

https://dxr.mozilla.org/mozilla-central/rev/02bc97729801a3b45d876f493c89260fa18ac170/toolkit/modules/BrowserUtils.jsm#716-725

I took a look at the platform code used by nsIDOMWindowUtils::NeedsFlush, and this is as far as I got:

https://dxr.mozilla.org/mozilla-central/rev/02bc97729801a3b45d876f493c89260fa18ac170/layout/base/nsIPresShell.h#595-616

Maybe mNeedThrottledAnimationFlush or mInFlush are interfering with the expected result?

Cameron, do you know what may be causing this issue?

Comment 4

[:Paolo Amadini]

Cameron, this is a regression surfaced by bug 1392340 but related, as far as I can tell, to bug 1334735.

Basically we have to ensure that promiseLayoutFlushed resolves immediately, instead of blocking indefinitely, if there is no need for a layout flush. This is the intended design, but at the moment per comment 3 it works in some cases but not all.

Do you have any input about this, or can redirect to someone knowledgeable in the area? Firefox 59 is moving to Beta and we'd like to prevent this annoyance if possible.

Comment 5

[Cameron McCormack (:heycam)]

I can't look into this right now, maybe David can find someone to?

Comment 6

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
The view not opening at all anymore in some cases is a regression that we can't ship.

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

I don't think this needs to block beta 59, but it should probably block release 59.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So from a quick look, this code is pretty far away from doing what I think it intends to do.

promiseLayoutFlushed takes a flushType argument that can be one of three things:  "style", "layout", or "display".

The underling code calls PresShell::NeedsFlush:
https://searchfox.org/mozilla-central/rev/4611b9541894e90a421debb57ddbbcff55c2f369/layout/base/nsIPresShell.h#603-616
which:
 * if the argument is "style", returns true if a style flush is pending
 * if the argument is "layout", returns true if a style or layout flush is pending
 * if the argument is "display", always returns true

It then expects a reflow observer to be called later.  A reflow observer will be called here, I think:
https://searchfox.org/mozilla-central/rev/4611b9541894e90a421debb57ddbbcff55c2f369/layout/base/PresShell.cpp#8804
which means when a reflow actually happens.

Now a style flush being pending doesn't mean that reflow will happen.  It could cause reflow to happen, or it might not -- it depends on what the style changes do, and whether they trigger reflow.

It's perhaps a plausible claim that a layout flush being pending means reflow will happen.  On the other hand, it might be easily refutable (for example, by checking that all callers of https://searchfox.org/mozilla-central/search?q=SetNeedLayoutFlush&case=true&path= actually do something that causes a reflow).  And proving it to be true would require thinking through a lot of edge cases.

However, despite that, it's easy to demonstrate that promiseLayoutFlushed is broken for all three argument types:
 * "style", since there's no guarantee that style being dirty will cause a reflow (for example, if only a color has been changed)
 * "layout", for the same reason as "style", since needsFlush returns true for "layout" if a style flush is needed
 * "display", since needsFlush *always* returns true, and there's certainly no guarantee there will always be a reflow.

I think if you need a general mechanism for this, it needs to be baked much further into layout code.  It's worth figuring out what the use cases are and how general that mechanism needs to be, though, before doing a lot of work.

Comment 9

[Marco Bonardo [:mak]]

This may explain why in the past when I tried to use promiseLayoutFlush sometimes the promise was apparently never resolved.
https://searchfox.org/mozilla-central/rev/4611b9541894e90a421debb57ddbbcff55c2f369/browser/components/places/content/browserPlacesViews.js#1605

This may be causing bugs in the few existing consumers
https://searchfox.org/mozilla-central/search?q=promiselayoutflush&path=

Comment 10

[:Paolo Amadini]

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #8)
> So from a quick look, this code is pretty far away from doing what I think
> it intends to do.

That's pretty grim news :-( Thanks for taking a look.

> I think if you need a general mechanism for this, it needs to be baked much
> further into layout code.  It's worth figuring out what the use cases are
> and how general that mechanism needs to be, though, before doing a lot of
> work.

Basically we want to avoid synchronous reflows. So, my understanding is that whenever we need to call .getBoundingClientRect() we have to do it in the promiseLayoutFlushed("layout") callback. I think the "style" case exists for when we need to call .getComputedStyle(). I'm not sure what the "display" case is about.

This is a pretty high priority feature to have, as avoiding jank caused by synchronous reflows is one of our main performance goals for Firefox 57 and beyond.

In the meantime, to fix the immediate shortcomings of this API, is it possible to change something in the DOM or call an API that will _guarantee_ an asynchronous reflow, even if not necessary, whenever we think we _might_ have one pending? This would probably make things slower in some cases, but is better than the current situation because these are the cases where now we have deadlocks.

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

Maybe Kris, as the author of promiseLayoutFlushed, has some ideas on how to avoid this issue.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Oof. :(

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Fx59b5+ are no longer affected via the backout in bug 1433143.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Should this just be done after a presshell flush / refresh driver tick, instead of after reflow, regardless of the type?

If so, I'm pretty sure some stuff runs in sync with the refresh driver (requestAnimationFrame IIRC?).

Why isn't something like that usable? It's effectively waiting for a refresh driver tick if there's any needed flush, but I think it may be ok? (maybe I'm totally confused about how this is used and what's its intention though)

Comment 15

[Mike Conley (:mconley) (:⚙️)]

I've filed bug 1434376 to make BrowserUtils.promiseReflowed and BrowserUtils.promiseLayoutFlushed more reliable.

Comment 16

[:Paolo Amadini]

Moving discussion to bug 1434376.

Comment 17

[:Gijs (he/him)]

So this too should be better now, like bug 1440973. Is this the case for you?

Comment 18

[magicp]

(In reply to :Gijs from comment #17)
> So this too should be better now, like bug 1440973. Is this the case for you?

Yes, works fine now. Thanks!

Comment 19

[:Gijs (he/him)]

Marking wfm per bug 1434376.