Mike Conley (:mconley) (:⚙️) Assignee
	Description • 7 years ago

There are key user interactions in the browser that we want to always be smooth.

Synchronous reflow and style flushes are performance cliffs that can work against us. We should add tests for the key interactions to record how many sync reflows and style flushes we already do, and then try to drive that number to 0 (and ensure that number never grows).

We have tests for the tab open and window open cases in

browser/base/content/test/general/browser_tabopen_reflows.js

and

browser/base/content/test/general/browser_windowopen_reflows.js


That's a great start. sfoster is working on a sync style flush test for window focus switching in bug 1334642. That's a great help too.

This bug is about writing tests like this for all[1] of our key interactions.

[1]: Where practical, naturally.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 1 • 7 years ago

Our key interactions are:

Tabs:
  * Open tab
  * Open tab causing tab squeeze
  * Open tab in an overflowed tab bar
  * Close tab
  * Close tab causing tab un-squeeze
  * Close tab in an overflowed tab bar
  * Switch tab
  * Entering the tab overflow state
  * Exiting the tab overflow state

Windows:
  * Opening a window
  * Closing a window
  * Opening the first browser window

While we're at it, there are probably a few others we should try to get in as well:

* Focusing the URL bar
* Typing in the URL bar
* Opening the default menus (hamburger, downloads, bookmarks, Pocket, etc)

	(no longer active)
	Comment 2 • 7 years ago

While we should absolutely do this, test issues are not gonna be tracked for qf.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 6 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=518f409550a6a2ef4ac316a0539310b8a8cd48a8

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 7 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5ef07f4dd261c0882feebd0eba826cc8ecd46f1

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 20 • 7 years ago

I think I'm going to scope this bug down now to the following things:

1) Make it easier to write layout flush tests with a helper function that is more strict[1] than the current approach
2) Consolidate our current style and layout flush tests into the same folder
3) Re-write our current tests to use the helper function
4) Add one new test (window opening) that uses the helper function

[1]: By "more strict" I mean, do our darndest to make the reflows that occur during the test to be as deterministic as possible. This means instrumenting the browser to dirty the layout tree as often as possible (ie, the worst-case-scenario). By being this strict, we can have a strict set of reflows that we expect, so that if one goes away unexpectedly, the test will fail (with a message saying that the stack should be removed from the whitelist).

I'll file a follow-up bug to write more reflow tests, and then look into making style flush testing easier.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 21 • 7 years ago
mozreview-review

Comment on attachment 8865856 [details]
Bug 1354194 - Make browser_windowopen_reflows.js use the reflow test helper and make the whitelist reflect reality.

https://reviewboard.mozilla.org/r/137462/#review140754

::: browser/base/content/test/performance/browser_windowopen_reflows.js:3
(Diff revision 3)
> -/* Any copyright is dedicated to the Public Domain.
> -   http://creativecommons.org/publicdomain/zero/1.0/ */
> -
>  "use strict";
>  
> +/**

Note to self - we can probably re-enable this test on OS X now (since it's currently disabled in browser.ini).

	Dão Gottwald [:dao]
	Comment 22 • 7 years ago
mozreview-review

Comment on attachment 8865856 [details]
Bug 1354194 - Make browser_windowopen_reflows.js use the reflow test helper and make the whitelist reflect reality.

https://reviewboard.mozilla.org/r/137462/#review140760

::: browser/base/content/test/performance/browser_windowopen_reflows.js
(Diff revision 3)
> -  // Focusing the content area causes a reflow.
> -  "_delayedStartup@chrome://browser/content/browser.js|",
> -
> -  // Sometimes sessionstore collects data during this test, which causes a sync reflow
> -  // (https://bugzilla.mozilla.org/show_bug.cgi?id=892154 will fix this)
> -  "ssi_getWindowDimension@resource:///modules/sessionstore/SessionStore.jsm",

I took care of this one in https://hg.mozilla.org/integration/mozilla-inbound/rev/aa210d7d1b43

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 23 • 7 years ago
mozreview-review-reply

Comment on attachment 8865856 [details]
Bug 1354194 - Make browser_windowopen_reflows.js use the reflow test helper and make the whitelist reflect reality.

https://reviewboard.mozilla.org/r/137462/#review140760

> I took care of this one in https://hg.mozilla.org/integration/mozilla-inbound/rev/aa210d7d1b43

Nicely done! :D

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 24 • 7 years ago
mozreview-review

Comment on attachment 8865856 [details]
Bug 1354194 - Make browser_windowopen_reflows.js use the reflow test helper and make the whitelist reflect reality.

https://reviewboard.mozilla.org/r/137462/#review140776

::: browser/base/content/test/performance/browser_windowopen_reflows.js:56
(Diff revision 3)
>  /*
>   * This test ensures that there are no unexpected
>   * uninterruptible reflows when opening new windows.
>   */
> -function test() {
> -  waitForExplicitFinish();
> +add_task(function*() {
> +  // Add a reflow observer and open a new tab.

Nit: probably can remove this comment.

	Dão Gottwald [:dao]
	Comment 25 • 7 years ago

(In reply to Mike Conley (:mconley) - At work week, slow to respond from comment #23)
> Comment on attachment 8865856 [details]
> Bug 1354194 - Make browser_windowopen_reflows.js use the reflow test helper
> and make the whitelist reflect reality.
> 
> https://reviewboard.mozilla.org/r/137462/#review140760
> 
> > I took care of this one in https://hg.mozilla.org/integration/mozilla-inbound/rev/aa210d7d1b43
> 
> Nicely done! :D

In case you want to rebase, this is now on central.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 26 • 7 years ago

(In reply to Dão Gottwald [::dao] from comment #25)
> In case you want to rebase, this is now on central.

Not sure I understand - in the patch, I _removed_ the ssi_getWindowDimension reflow from the whitelist, since it's not being hit anymore.

Or am I missing something?

	Dão Gottwald [:dao]
	Comment 27 • 7 years ago

(In reply to Mike Conley (:mconley) - At work week, slow to respond from comment #26)
> (In reply to Dão Gottwald [::dao] from comment #25)
> > In case you want to rebase, this is now on central.
> 
> Not sure I understand - in the patch, I _removed_ the ssi_getWindowDimension
> reflow from the whitelist, since it's not being hit anymore.
> 
> Or am I missing something?

I already removed it from the whitelist earlier today.

	Florian Quèze [:florian]
	Comment 32 • 7 years ago
mozreview-review

Comment on attachment 8863911 [details]
Bug 1354194 - Add utility method for more easily writing reflow tests.

https://reviewboard.mozilla.org/r/135638/#review142246

(I didn't get time to do a full review before my flight started boarding, but publishing now as I figured you may be interested in the comments I already had)

::: browser/base/content/test/performance/head.js:78
(Diff revision 5)
> + *                            is passed)
> + *         Callers that pass dirtyFrameFn must pass this clean up function to
> + *         return the DOM back to its normal state so that future tests don't
> + *         inherit a strangely styled browser.
> + */
> +async function withReflowObserver(window, expectedStacks, testFn, dirtyFrameFn, dirtyFrameCleanUp) {

I think head.js runs in the JS context of the browser window, so 'window' is already defined; do you really want to shadow it?

Feel free to ignore that comment if it's causing a lot of work to address: I think testFn is the only non-optional parameter here, so should it be first? I guess 'window' should default to err... window?

nit: wrap this long line?

::: browser/base/content/test/performance/head.js:87
(Diff revision 5)
> +  }
> +
> +  if (!dirtyFrameFn) {
> +    // The default dirty frame function adds an increasing margin
> +    // to the first element child of the window document to a maximum
> +    // of 15px, and then goes back to 0.

You have "% 4" in the code, so I guess the maximum is 3px rather than 15.

::: browser/base/content/test/performance/head.js:105
(Diff revision 5)
> +  let els = Cc["@mozilla.org/eventlistenerservice;1"]
> +              .getService(Ci.nsIEventListenerService);
> +
> +  // We're going to remove the stacks one by one as we see them so that
> +  // we can check for expected, unseen reflows, so let's clone the array.
> +  expectedStacks = expectedStacks.slice(0);

Shouldn't expectedStacks default to an empty array?

	Florian Quèze [:florian]
	Comment 33 • 7 years ago
mozreview-review

Comment on attachment 8865856 [details]
Bug 1354194 - Make browser_windowopen_reflows.js use the reflow test helper and make the whitelist reflect reality.

https://reviewboard.mozilla.org/r/137462/#review142250

::: browser/base/content/test/performance/browser_windowopen_reflows.js
(Diff revision 4)
> -/* Any copyright is dedicated to the Public Domain.
> -   http://creativecommons.org/publicdomain/zero/1.0/ */

Last time we discussed whether tests should have public domain headers, Gerv said having them was preferable, so let's not remove them.

::: browser/base/content/test/performance/browser_windowopen_reflows.js:55
(Diff revision 4)
>  
>  /*
>   * This test ensures that there are no unexpected
>   * uninterruptible reflows when opening new windows.
>   */
> -function test() {
> +add_task(function*() {

At this point we shouldn't introduce new generator functions as task in browser/. Please switch to async/await, and sorry I didn't get to this review early enough to avoid bitrot :(.

	Florian Quèze [:florian]
	Comment 45 • 7 years ago
mozreview-review

Comment on attachment 8863911 [details]
Bug 1354194 - Add utility method for more easily writing reflow tests.

https://reviewboard.mozilla.org/r/135638/#review143558

::: browser/base/content/test/performance/head.js:9
(Diff revision 7)
> + *
> + * The helper works by running a JS function before each event is
> + * dispatched that attempts to dirty the layout tree - the idea being
> + * that this puts us in the "worst case scenario" so that any JS
> + * that attempts to query for layout or style information will cause
> + * a reflow to fire. We also dirty the layout tree after each reflow

Is this enough to catch sync reflows by code running from timers/requestIdleCallback/dispatchToMainThread?

::: browser/base/content/test/performance/head.js:74
(Diff revision 7)
> + *        Callers can provide a custom DOM node to change some layout style
> + *        on in the event that the action being tested is occurring within
> + *        a scrollable frame. Otherwise, withReflowObserver defaults to dirtying
> + *        the first element child of the window.
> + */
> +async function withReflowObserver(testFn, expectedStacks = [], win, elemToDirty) {

I think you missed " = window" after "win".

::: browser/base/content/test/performance/head.js:102
(Diff revision 7)
> +      }).join("|");
> +
> +      let pathWithLineNumbers = (new Error().stack).split("\n").slice(1);
> +
> +      // Just in case, dirty the frame now that we've reflowed.
> +      elemToDirty.style.margin = "";

I think this should be dirtyFrameFn();, or you won't get any result when the margin was already 0.

::: browser/base/content/test/performance/head.js:105
(Diff revision 7)
> +
> +      // Just in case, dirty the frame now that we've reflowed.
> +      elemToDirty.style.margin = "";
> +
> +      // Stack trace is empty. Reflow was triggered by native code, which
> +      // we ignore.

Is this the right thing to do? If some native code was to introduce an uninterruptible reflow within one of the interactions we care about, we would want to know (and get that patch backed out), right?

Could be material for a follow-up.

::: browser/base/content/test/performance/head.js:149
(Diff revision 7)
> +                "This is probably a good thing - just remove it from the " +
> +                "expected list.");
> +    }
> +
> +    els.removeListenerForAllEvents(win, dirtyFrameFn, true);
> +    docShell.removeWeakReflowObserver(observer);

Should we return elemToDirty.style.margin to its original value here?

	Florian Quèze [:florian]
	Comment 46 • 7 years ago
mozreview-review

Comment on attachment 8863912 [details]
Bug 1354194 - Make browser_tabopen_reflows.js use the new reflow test helper, and make the whitelist reflect reality.

https://reviewboard.mozilla.org/r/135640/#review143566

::: browser/base/content/test/performance/browser_tabopen_reflows.js:111
(Diff revision 8)
> -
> -function waitForTransitionEnd() {
> -  return new Promise(resolve => {
> -    let tab = gBrowser.selectedTab;
> +  let tab = gBrowser.selectedTab;
> -    tab.addEventListener("transitionend", function onEnd(event) {
> -      if (event.propertyName === "max-width") {
> +  await BrowserTestUtils.removeTab(gBrowser.selectedTab, { animate: true });
> +  await BrowserTestUtils.waitForCondition(() => tab.parentNode == null);

What is this code doing? Why do we need anything more than just removing the tab we added?

	Florian Quèze [:florian]
	Comment 47 • 7 years ago
mozreview-review

Comment on attachment 8865856 [details]
Bug 1354194 - Make browser_windowopen_reflows.js use the reflow test helper and make the whitelist reflect reality.

https://reviewboard.mozilla.org/r/137462/#review143594

	Florian Quèze [:florian]
	Comment 48 • 7 years ago
mozreview-review

Comment on attachment 8865857 [details]
Bug 1354194 - Add reflow regression tests for tab closing.

https://reviewboard.mozilla.org/r/137464/#review143570

::: browser/base/content/test/performance/browser_tabclose_reflows.js:31
(Diff revision 7)
> +/*
> + * This test ensures that there are no unexpected
> + * uninterruptible reflows when closing new tabs.
> + */
> +add_task(async function() {
> +  // If we've got a preloaded browser, get rid of it so that it

Feel free to ignore this comment, but I wonder if this workaround related to aboutnewtab should go to head.js

I guess it depends if you expect to need it for more tests soon.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 49 • 7 years ago
mozreview-review-reply

Comment on attachment 8863911 [details]
Bug 1354194 - Add utility method for more easily writing reflow tests.

https://reviewboard.mozilla.org/r/135638/#review143558

> Is this enough to catch sync reflows by code running from timers/requestIdleCallback/dispatchToMainThread?

I'm afraid not. :( Perhaps good fodder for a follow-up. I'll add a TODO to file one once this bug lands.

> Is this the right thing to do? If some native code was to introduce an uninterruptible reflow within one of the interactions we care about, we would want to know (and get that patch backed out), right?
> 
> Could be material for a follow-up.

If there's no JS on the stack, I'm pretty sure that means that the cause of the reflow was a refresh driver tick, unless there's another case here that you're thinking of?

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 50 • 7 years ago
mozreview-review-reply

Comment on attachment 8863912 [details]
Bug 1354194 - Make browser_tabopen_reflows.js use the new reflow test helper, and make the whitelist reflect reality.

https://reviewboard.mozilla.org/r/135640/#review143566

> What is this code doing? Why do we need anything more than just removing the tab we added?

Ah, this is a leftover from when I was running this test on repeat with --run-until-failure. There are cases where after calling removeTab, the tab will continue to exist in the DOM even after the removeTab Promise resolves, which can mess up the next run of the test.

Doesn't seem to affect things when the tests are run in sequence, which is how it's done in automation.

I'll file a follow-up to change BrowserTestUtils.removeTab to ensure that the DOM node is removed.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 51 • 7 years ago

ni? to florian for comment 49.

	Florian Quèze [:florian]
	Comment 56 • 7 years ago

(In reply to Mike Conley (:mconley) from comment #49)

> If there's no JS on the stack, I'm pretty sure that means that the cause of
> the reflow was a refresh driver tick, unless there's another case here that
> you're thinking of?

I was somehow assuming that reflow observers were not notified for reflows caused by the refresh driver tick.

I don't have a specific case in mind, I was just thinking that cases like webNavigation.stop() (which could be called from C++) or moving the focus (eg. from activating the window) should be reported by the tests.

(In reply to Mike Conley (:mconley) from comment #50)

> I'll file a follow-up to change BrowserTestUtils.removeTab to ensure that
> the DOM node is removed.

Exactly what I was going to suggest :-).

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 57 • 7 years ago

(In reply to Florian Quèze [:florian] [:flo] from comment #56)
> (In reply to Mike Conley (:mconley) from comment #49)
> 
> > If there's no JS on the stack, I'm pretty sure that means that the cause of
> > the reflow was a refresh driver tick, unless there's another case here that
> > you're thinking of?
> 
> I was somehow assuming that reflow observers were not notified for reflows
> caused by the refresh driver tick.
> 
> I don't have a specific case in mind, I was just thinking that cases like
> webNavigation.stop() (which could be called from C++) or moving the focus
> (eg. from activating the window) should be reported by the tests.
> 

Ah, those. :( I wonder if there's a way to supply a "reason" to the nsIReflowObserver. That will be the follow-up bug.

	Pulsebot
	Comment 61 • 7 years ago

Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0428e82ccdaa
Add utility method for more easily writing reflow tests. r=florian
https://hg.mozilla.org/integration/autoland/rev/a6d09aa52cb5
Make browser_tabopen_reflows.js use the new reflow test helper, and make the whitelist reflect reality. r=florian
https://hg.mozilla.org/integration/autoland/rev/d537626e1a10
Make browser_windowopen_reflows.js use the reflow test helper and make the whitelist reflect reality. r=florian
https://hg.mozilla.org/integration/autoland/rev/cea53d87b4d5
Add reflow regression tests for tab closing. r=florian

	Ryan VanderMeulen [:RyanVM]
	Comment 62 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/0428e82ccdaa
https://hg.mozilla.org/mozilla-central/rev/a6d09aa52cb5
https://hg.mozilla.org/mozilla-central/rev/d537626e1a10
https://hg.mozilla.org/mozilla-central/rev/cea53d87b4d5

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 63 • 7 years ago

Filed bug 1368132 to deal with the timer/requestIdleCallback/dispatchToMainThread case.

Filed bug 1368131 to check that the tab is fully removed in BrowserTestUtils.removeTab.