1742842 - Ensure VSync is disabled at the end of automated browser chrome tests

Status: RESOLVED FIXED

(Core :: Graphics, task)

Description

[Florian Quèze [:florian]]

We found multiple times by accident steps to reproduce bugs that cause vsync to remain enabled forever (or until a browser window is closed). We should leverage our large existing test suite to detect cases like this faster.

Try run for a first attempt: https://treeherder.mozilla.org/jobs?repo=try&tier=1%2C2%2C3&revision=55ab23e422025b695486aee32f3ec8b6c7e2b7e1

This list of failing tests contains occurences of bug 560067 and bug 1742797.

There are also test issues. For exemple some tests keep about:robots (which has an animated favicon) or a non-loading page (with the bouncing load indicator) at the end of the test.

Comment 1

[Florian Quèze [:florian]]

Attachment: Bug 1742842 - Ensure that vsync is disabled after each browser-chrome mochitest, r=mconley.

Comment 2

[Florian Quèze [:florian]]

Attachment: WIP: Bug 1742842 - debug markers for when vsync is enabled/disabled.

Depends on D132055

Comment 3

[Florian Quèze [:florian]]

Attachment: Bug 1742842 - Workaround known issues in tests that currently keep vsync enabled forever, r=mconley.

Depends on D132055

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b94a13888214
Ensure that vsync is disabled after each browser-chrome mochitest, r=mconley.
https://hg.mozilla.org/integration/autoland/rev/82364f4b3520
Workaround known issues in tests that currently keep vsync enabled forever, r=mconley.

Comment 5

[Atila Butkovits]

Backed out for causing failures complaining about vsync

Backout link: https://hg.mozilla.org/integration/autoland/rev/7617aa566da692e7afb43dfcd833653ef7dcd098

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=KKfa9j19QfSI1TCLhl6L1Q.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=82364f4b3520e2fb574cd84fe43cb7a1dc4cc154

Failure log:

• https://treeherder.mozilla.org/logviewer?job_id=385814596&repo=autoland&lineNumber=537
• https://treeherder.mozilla.org/logviewer?job_id=385816939&repo=autoland&lineNumber=43688

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3ab9dcc66a5b
Ensure that vsync is disabled after each browser-chrome mochitest, r=mconley.
https://hg.mozilla.org/integration/autoland/rev/f69a46e3eb9a
Workaround known issues in tests that currently keep vsync enabled forever, r=mconley,webdriver-reviewers,jdescottes.

Comment 7

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/3ab9dcc66a5b
https://hg.mozilla.org/mozilla-central/rev/f69a46e3eb9a

Comment 8

[Jens Stutte [:jstutte]]

Hi! I am slightly confused about the state of this bug: Is it really meant to be closed (depending on 7 open bugs)?

In any case I see waiting for vsync to be disabled in bug 1792516 quite often again. Are you aware of any recent changes here that could explain the recent spike?

Comment 9

[Florian Quèze [:florian]]

(In reply to Jens Stutte [:jstutte] from comment #8)

Hi! I am slightly confused about the state of this bug: Is it really meant to be closed (depending on 7 open bugs)?

Yes, we landed support "ensuring VSync is disabled at the end of automated browser chrome tests", so this bug is fixed we will catch regressions / new issues. The 7 dependent bugs are issues for which a workaround was found to unblock this bug.

In any case I see waiting for vsync to be disabled in bug 1792516 quite often again. Are you aware of any recent changes here that could explain the recent spike?

The failure I see in bug 1792516 is "dom/ipc/tests/browser_content_shutdown_with_endless_js.js | Test timed out". And then, because the test timed out, it didn't finish cleanly and didn't cleanup things it would otherwise cleanup. It most likely left something animated still displayed, that's what the "vsync remained enabled at the end of the test. Is there an animation still running?" failure means. Maybe I should silence this failure in the case where the test is already failing due to a timeout, as it might be confusing, and doesn't add value in that case.

Comment 10

[Jens Stutte [:jstutte]]

(In reply to Florian Quèze [:florian] from comment #9)

The failure I see in bug 1792516 is "dom/ipc/tests/browser_content_shutdown_with_endless_js.js | Test timed out". And then, because the test timed out, it didn't finish cleanly and didn't cleanup things it would otherwise cleanup. It most likely left something animated still displayed, that's what the "vsync remained enabled at the end of the test. Is there an animation still running?" failure means. Maybe I should silence this failure in the case where the test is already failing due to a timeout, as it might be confusing, and doesn't add value in that case.

Thanks! Yeah, I am not always sure to what extent I can trust the order of the lines in the log file, but if we can, this is probably just a symptom of what happened before.