1421456 - Add a test to prevent flickering regressions on window opening

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[Florian Quèze [:florian]]

I believe it's possible to use a browser mochitest to detect flickering during startup and when opening a new window.

Comment 1

[Florian Quèze [:florian]]

Attachment: Patch

Comment 2

[Florian Quèze [:florian]]

Attachment: Patch v2

Try push for this version of the patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=33bc0b04b49a7937412ef15a138effb69ea62188

A previous try run with lots of retriggers was mostly green https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a57eae2221ed3b0605833a5c13283bab29befbd except for:
- intermittent leak on Windows debug (about once out of 5 runs). No idea how to debug that, so I just disabled the test on Win debug.
- 2 timeouts on linux debug due to lack of output for 370s. I added an info() call when we find an identical frame, hopefully that should fix it.
- a change on the urlbar down arrow being 1px smaller than usual (due to the CSS transition being almost finished) on a Linux debug build. I've adjusted the range for it to take this case into account.

Given that the test takes a while (about 30-60s on opt, and up to 5-10 minutes on debug), I wonder if we should just disable it on debug on all platforms to save resources.

I would like to profile this to see what takes so long, but I haven't found a way to use the profiler on a bc test yet. And we can improve the test's performance in a follow-up if needed.

Comment 3

[Florian Quèze [:florian]]

Attachment: Patch v3

Now that I've figured out how to profile a bc test (see bug 1421972), it became possible to fix the perf issues of these new tests. New version taking less than 5s on try on debug :-).

Still leaking intermittently (more frequently than before actually) on Windows debug, but green otherwise on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=073f93c3c86208195f20bb72be583d312286f753
This patch keeps the leaky test disabled on Windows debug.

Comment 4

[Johann Hofmann [:johannh]]

Comment on attachment 8933283 [details] [diff] [review]
Patch v3

Review of attachment 8933283 [details] [diff] [review]:
-----------------------------------------------------------------

This seems to work fine, so I'm r+ing because none of the points below really block landing, but it would be great to see them addressed anyway (obviously).

Thanks! This is a really good idea, IMO.

::: browser/base/content/test/performance/browser.ini
@@ +1,4 @@
>  [DEFAULT]
>  prefs =
>    browser.startup.record=true
> +  gfx.canvas.willReadFrequently.enable=true

I'm not super happy about setting prefs that are need by individual test files for the whole directory. This pref needs to be set before startup, right? If that's the case, please at least add a comment what it does and for which test, so that other people are less likely to be scratching their heads (e.g. when this folder at some point tests canvas performance for some strange reason).

@@ +7,5 @@
>  [browser_appmenu_reflows.js]
>  skip-if = asan || debug # Bug 1382809, bug 1369959
>  [browser_favicon_load.js]
>  [browser_startup.js]
> +[browser_startup_flicker.js]

Nit: since this list looks alphabetically ordered, this should be below browser_startup_content

::: browser/base/content/test/performance/browser_startup_flicker.js
@@ +20,5 @@
> +  let alreadyFocused = false;
> +  for (let i = 1; i < frames.length; ++i) {
> +    let frame = frames[i], previousFrame = frames[i - 1];
> +    let rects = compareFrames(frame, previousFrame);
> +    if (!alreadyFocused && rects.length > 5 && rects.every(r => r.y2 < 100)) {

It's probably just me but I don't really understand this condition.

@@ +36,5 @@
> +      let width = frame.width;
> +      r.w = w;
> +      r.h = h;
> +      let rectText = `${r.toSource()}, window width: ${width}`;
> +      if (h == 5 && inRange(w, 8, 9) && inRange(r.y1, 40, 80) &&

I have to say I'm really not a big fan of the way these exceptions are inlined here, how about a configuration object that includes a function which gets passed the different parameters and figures out if the criteria for the bug apply?

@@ +76,5 @@
> +    }
> +    unexpectedRects += rects.length;
> +    dumpFrame(frame);
> +  }
> +  is(unexpectedRects, 0, "should have 0 unknown flickering areas");

I assume the "expected" rects are intermittent, otherwise it might have been worth adding them up to this number to notice when something gets fixed.

::: browser/base/content/test/performance/browser_windowopen_flicker.js
@@ +76,5 @@
> +    let frame = frames[i], previousFrame = frames[i - 1];
> +    if (!foundTinyPaint &&
> +        previousFrame.width == 1 && previousFrame.height == 1) {
> +      foundTinyPaint = true;
> +      todo(false, "shouldn't first paint a 1x1px window");

Hah, is there a bug for this?

@@ +92,5 @@
> +      let rectText = `${r.toSource()}, window width: ${width}`;
> +
> +      if (h == 5 && inRange(w, 8, 9) && inRange(r.y1, 40, 80) &&
> +          inRange(r.x1, width * .75, width * .9)) {
> +        todo(false, "bug 1403648 - urlbar down arrow shouldn't flicker, " + rectText);

See above, this would be great to put in a JS object.

::: browser/base/content/test/performance/head.js
@@ +332,5 @@
> +    if (r1.y2 >= r2.y1 - 1 - maxEmptyPixels &&
> +        r2.x1 - 1 - maxEmptyPixels <= r1.x2 &&
> +        r2.x2 >= r1.x1 - 1 - maxEmptyPixels)
> +      return true;
> +    return false;

Nit: you could just write return (r1.y2 >= r2.y1 ....

@@ +352,5 @@
> +          break;
> +        }
> +      }
> +    }
> +  } while (hasMergedRects);

This loop would be much more pleasant to read with a short comment, especially as to why you're merging the rects.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd4a422f41cf
Add a test to prevent flickering regressions on window opening, r=johannh.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/273ef38bfd2b
Follow-up to take into account that the favicon height is sometimes 15px, rs=bustage-fix.

Comment 7

[Florian Quèze [:florian]]

(In reply to Johann Hofmann [:johannh] from comment #4)

> Thanks! This is a really good idea, IMO.

:-)

> ::: browser/base/content/test/performance/browser.ini
> @@ +1,4 @@
> >  [DEFAULT]
> >  prefs =
> >    browser.startup.record=true
> > +  gfx.canvas.willReadFrequently.enable=true
> 
> I'm not super happy about setting prefs that are need by individual test
> files for the whole directory. This pref needs to be set before startup,
> right?

Yes, it needs to be set before startupRecorder.js starts its work, so before the tests run. I added a comment.

> @@ +76,5 @@
> > +    }
> > +    unexpectedRects += rects.length;
> > +    dumpFrame(frame);
> > +  }
> > +  is(unexpectedRects, 0, "should have 0 unknown flickering areas");
> 
> I assume the "expected" rects are intermittent, otherwise it might have been
> worth adding them up to this number to notice when something gets fixed.

Indeed, some are intermittent.

I would like to notice when something is fixed, so that we remove the exception and don't let it regress. I think we can revisit this after fixing a couple of the bugs that have exceptions currently.

> ::: browser/base/content/test/performance/browser_windowopen_flicker.js
> @@ +76,5 @@
> > +    let frame = frames[i], previousFrame = frames[i - 1];
> > +    if (!foundTinyPaint &&
> > +        previousFrame.width == 1 && previousFrame.height == 1) {
> > +      foundTinyPaint = true;
> > +      todo(false, "shouldn't first paint a 1x1px window");
> 
> Hah, is there a bug for this?

I'm not exactly sure of what the problem is. Is there a spurious MozAfterPaint event? Are we actually painting a 1x1px window? Is the window height/width not set correctly yet?

I think I would need a better understanding of what happens to know how to file a useful bug.

All the other comments were addressed before landing. Thanks for the review!

Comment 8

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/fd4a422f41cf
https://hg.mozilla.org/mozilla-central/rev/273ef38bfd2b