Make it easy to write synchronous reflow tests for key interactions

RESOLVED FIXED in Firefox 55

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

(Depends on 1 bug, Blocks 2 bugs)

unspecified
Firefox 55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [photon-performance][qf-])

Attachments

(4 attachments, 1 obsolete attachment)

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

Comment 2

2 years ago
While we should absolutely do this, test issues are not gonna be tracked for qf.
Whiteboard: [qf] → [qf-]
Whiteboard: [qf-] → [photon-performance][qf-]
Flags: qe-verify?
Priority: -- → P2
Depends on: 1356271
Flags: qe-verify? → qe-verify-
Assignee

Updated

2 years ago
Depends on: 1356684
See Also: 1334642
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Iteration: --- → 55.5 - May 15
Priority: P2 → P1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8863910 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8863911 - Flags: review?(mconley)
Assignee

Updated

2 years ago
Attachment #8863911 - Flags: review?(mconley)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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.
Summary: Write synchronous reflow and synchronous style flush tests for key interactions → Make it easy to write synchronous reflow tests for key interactions
Assignee

Updated

2 years ago
Blocks: 1363505
Assignee

Updated

2 years ago
Blocks: 1363507
Assignee

Updated

2 years ago
No longer depends on: 1356271
Assignee

Comment 21

2 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).

Comment 22

2 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
Assignee

Comment 23

2 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
Assignee

Comment 24

2 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.
(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.
(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?
Flags: needinfo?(dao+bmo)
(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.
Flags: needinfo?(dao+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 32

2 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?

Comment 33

2 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 :(.
Iteration: 55.5 - May 15 → 55.6 - May 29
Assignee

Updated

2 years ago
Attachment #8863911 - Flags: review?(florian)
Attachment #8863912 - Flags: review?(florian)
Attachment #8865856 - Flags: review?(florian)
Attachment #8865857 - Flags: review?(florian)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 45

2 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?
Attachment #8863911 - Flags: review?(florian) → review+

Comment 46

2 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?
Attachment #8863912 - Flags: review?(florian) → review+

Comment 47

2 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
Attachment #8865856 - Flags: review?(florian) → review+

Comment 48

2 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.
Attachment #8865857 - Flags: review?(florian) → review+
Assignee

Comment 49

2 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?
Assignee

Comment 50

2 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.
ni? to florian for comment 49.
Flags: needinfo?(florian)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(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 :-).
Flags: needinfo?(florian)
(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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 61

2 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
Depends on: 1367941
Assignee

Updated

2 years ago
Blocks: 1367797
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.
Depends on: 1368132
Assignee

Updated

2 years ago
Blocks: 1369491
You need to log in before you can comment on or make changes to this bug.