ScrollSelectionIntoViewEvent runs at random times and flushes layout. I believe it could be called very early in refresh driver tick, before scroll event dispatching. Scroll event dispatch happens when WillRefresh is called http://searchfox.org/mozilla-central/rev/ae94cfb36d804727dfb38f2750bfcaab4621df6e/layout/generic/nsGfxScrollFrame.cpp#4718

Let's see what tryserver says about this. In theory this should be fine, since the call was async already, but one never knows. remote: View your change here: remote: https://hg.mozilla.org/try/rev/d322fb360a3306bdd3847b0c2ef486d70ea1efe5 remote: remote: Follow the progress of your build on Treeherder: remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d322fb360a3306bdd3847b0c2ef486d70ea1efe5 remote: recorded changegroup in replication log in 0.119s

remote: View the pushlog for these changes here: remote: https://hg.mozilla.org/try/pushloghtml?changeset=d18869c80a5052d136b689ea1109b2eb1643326e remote: remote: Follow the progress of your build on Treeherder: remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d18869c80a5052d136b689ea1109b2eb1643326e remote: recorded changegroup in replication log in 0.123s Fixed one assertion

remote: View your change here: remote: https://hg.mozilla.org/try/rev/3595c44cca2f398b834262c25e64c809af86e9cf remote: remote: Follow the progress of your build on Treeherder: remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3595c44cca2f398b834262c25e64c809af86e9cf

Comment on attachment 8880602 [details] [diff] [review] + test fixes I'm reusing the ScrollSelectionIntoViewEvent class here, since they are nicely revoked etc. We may want to change AddPendingSelectionScroll to some more generic name later (I assume we may want to reuse it for IME stuff for example), but for now, I'd prefer this very specific name. We need mPendingSelectionScrolls, because the pending scroll should be called before WillRefresh (mObservers), which ends up dispatching scroll event. AutoTArray<, 16> is perhaps a tad big, but we may end up having several runnables there per tick, and the older ones are possibly revoked. The MOZ_ASSERTION change is fine, since mPendingSelectionScrolls happen to be internally Still waiting some more test results, but doesn't look too bad. With this patch the issue-showing-patch for bug 1373023 could be simpler. The Selection related change wouldn't be needed. Based on profiler this behaves as expected. We flush during tick and then paint. There is always the risk that scroll event listener causes more stuff to flush, but we can't really prevent that.

Comment on attachment 8880602 [details] [diff] [review] + test fixes Review of attachment 8880602 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsRefreshDriver.cpp @@ +1816,5 @@ > if (gfxPrefs::APZPeekMessages()) { > nsLayoutUtils::UpdateDisplayPortMarginsFromPendingMessages(); > } > > + AutoTArray<nsCOMPtr<nsIRunnable>, 16> pendingSelectionScrolls; Nit: do you mind making a typedef for it in the header to avoid repeating the type here and for the member? @@ +1820,5 @@ > + AutoTArray<nsCOMPtr<nsIRunnable>, 16> pendingSelectionScrolls; > + pendingSelectionScrolls.SwapElements(mPendingSelectionScrolls); > + for (uint32_t i = 0; i < pendingSelectionScrolls.Length(); ++i) { > + pendingSelectionScrolls[i]->Run(); > + } I think it's probably worth adding a comment here explaining that there is the risk of these listeners themselves causing more stuff to flush, but we're OK with it since there isn't anything better to do, since without such a comment the next person to read the code may think that this is an oversight...

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #5) > > + AutoTArray<nsCOMPtr<nsIRunnable>, 16> pendingSelectionScrolls; > > Nit: do you mind making a typedef for it in the header to avoid repeating > the type here and for the member? I do mind. Such typedefs just make code harder to read ;) They hide possibly useful information without not good reasaon. And here it is rather useful to know that the array elements are nsCOMPtr and not *. > > @@ +1820,5 @@ > > + AutoTArray<nsCOMPtr<nsIRunnable>, 16> pendingSelectionScrolls; > > + pendingSelectionScrolls.SwapElements(mPendingSelectionScrolls); > > + for (uint32_t i = 0; i < pendingSelectionScrolls.Length(); ++i) { > > + pendingSelectionScrolls[i]->Run(); > > + } > > I think it's probably worth adding a comment here explaining that there is > the risk of these listeners themselves causing more stuff to flush, Eh, isn't that super obvious. We don't have such comment for all the other possible callback we have. > but > we're OK with it since there isn't anything better to do, since without such > a comment the next person to read the code may think that this is an > oversight... really? Then someone really isn't familiar with refreshdriver.

Trying to still tweak that one failing test (can't reproduce locally). remote: View your change here: remote: https://hg.mozilla.org/try/rev/d6b16daf66212404e4c50b8d7d21bd3c315f7f01 remote: remote: Follow the progress of your build on Treeherder: remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6b16daf66212404e4c50b8d7d21bd3c315f7f01

Pushed by opettay@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a3d1fb7eaac4 ScrollSelectionIntoViewEvent should be called during refresh driver tick, r=ehsan