1319764 - [Linux] Important graphic glitches on Developer Edition First Run UI tour

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Description

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

[Note]:
- This issue is a follow up for bug 122504
- It only affects the Developer Edition builds (e10s on/off)


[Affected versions]:
- 52.0a2 (2016-11-23)


[Affected platforms]:
- Ubuntu 14.04 x86
- Ubuntu 16.04 x64


[Steps to reproduce]:
1. Launch Developer Edition.
2. Wait for the first run page to load.
3. Click Next buttons via the First Run panel.
- inspect the First Run panel in every tour step


[Expected result]:
- The First Run UI tour is properly displayed


[Actual result]:
- Graphic glitches can be observed on the First Run panel during the UI tour (see the screenshots https://drive.google.com/open?id=0B0nYKG6PRiCcLXlNc2owSUNpc00; https://drive.google.com/open?id=0B0nYKG6PRiCcS1oxc2JkNHN2MWs)
- Firefox tour highlights are invisible


[Regression range]:
- Last good revision: 8f7f9af27cb6 (2015-08-09)
- First bad revision: 7308dd0a6c3b (2015-08-10)
- Pushlog:
https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=8f7f9af27cb6&tochange=7308dd0a6c3b

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Does the problem go away when you force the hardware acceleration in the developer edition?

Comment 2

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

(In reply to Milan Sreckovic [:milan] from comment #1)
> Does the problem go away when you force the hardware acceleration in the
> developer edition?

I set the layers.acceleration.force-enabled pref to true value and the problem is still reproducible.

Comment 3

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

(In reply to Iulia Cristescu, QA [:IuliaC] from comment #0)
> [Note]:
> - This issue is a follow up for bug 122504

I meant "bug 1225044", sorry for the mistake

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Lee, can you reproduce this?

Comment 5

[Lee Salzman [:lsalzman]]

(In reply to Milan Sreckovic [:milan] from comment #4)
> Lee, can you reproduce this?

Seems like I can, though the symptoms are not quite what was described (the power of invisibility). I occasionally see black squares where the highlights should be. Seems like the windows that pop up occasionally don't get painted, which would mean the fix Andrew Comminos made in bug 1225044 either didn't work or stopped working.

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

Jamie, if you can reproduce, can you take a look at this?  Lee's a bit swamped with printing.

Comment 7

[Jamie Nicol [:jnicol]]

Looking in to it

Comment 8

[Jamie Nicol [:jnicol]]

It appears that bug 1225044 never actually fixed the problem. The first version of Andrew's patch did, but after changing the approach later versions of the patch did not.

Comment 9

[Jamie Nicol [:jnicol]]

Actually, the first version of Andrew's patch didn't completely work either, it just almost worked in a slightly different way.

I believe the problem is because we are not incrementing mPendingConfigures on every possible type of configure request (only check_resize), but we decrement it on every configure notify. The counter drops to negative values, so that even after incrementing it in check_resize_cb() it is still negative, meaning the workaround in NativeShow() never happens.

If I am correct, ideally we would increment for every configure request. Andrew or Karl, do you know of a way to do this? Failing that, I have a patch which simply never lets mPendingConfigures drop below zero. This significantly reduces the chance of us miscounting, as the uncounted configure request would have to occur in a short window of time. With this patch I have not been able to reproduce this bug so it might well be good enough, certainly it's an improvement.

Comment 10

[Jamie Nicol [:jnicol]]

Attachment: Bug 1319764 - Ensure Gtk window unmap workaround is actually used.

We were miscounting the number of manual configure events which we
needed to send gtk as the workaround for bug 1225044, causing it not
to work in some cases. This is because configure events can come from
more sources than were counting.

By not letting the counter drop below zero we vastly reduce the
likelihood of miscounting, as the uncounted event would have to occur
in a short timeframe.

Review commit: https://reviewboard.mozilla.org/r/104528/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104528/

Comment 11

[Karl Tomlinson (:karlt)]

Comment on attachment 8826594 [details]
Bug 1319764 - Ensure Gtk window unmap workaround is actually used.

https://reviewboard.mozilla.org/r/104528/#review105528

Thank you for diagnosing this, Jamie.

This is the correct fix.

::: widget/gtk/nsWindow.h:1
(Diff revision 1)
>  /* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

> We were miscounting the number of manual configure events which we
> needed to send gtk as the workaround for bug 1225044, causing it not
> to work in some cases.  This is because configure events can come from
> more sources than were counting.

Yes, the increments and decrements do not match for this reason, but we don't
need them to balance.

> By not letting the counter drop below zero we vastly reduce the
> likelihood of miscounting, as the uncounted event would have to occur
> in a short timeframe.

The workaround is now correctly following what GTK does for configure_request_count on configure-event.  GTK will decrement a positive configure_request_count regardless of the source, and the bug only happens when the window is unmapped while this is non-zero.  That means this workaround is fine decrementing regardless of the source of the configure-event because it will not reach zero before configure_request_count reaches zero.

https://git.gnome.org/browse/gtk+/tree/gtk/gtkwindow.c?h=3.18.9#n7544

Please replace this second paragraph of the commit message with something
like "Decrement mPendingConfigures only as far as zero, like
configure_request_count in gtk_window_configure_event()."

Comment 12

[Karl Tomlinson (:karlt)]

Comment on attachment 8826594 [details]
Bug 1319764 - Ensure Gtk window unmap workaround is actually used.

https://reviewboard.mozilla.org/r/104528/#review105530

Comment 13

[Jamie Nicol [:jnicol]]

Comment on attachment 8826594 [details]
Bug 1319764 - Ensure Gtk window unmap workaround is actually used.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104528/diff/1-2/

Comment 14

[Jamie Nicol [:jnicol]]

Commit message updated. Tests look good (this only affects gtk/linux): https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca6f63834c32e479241fe4ecf99b549f6edaaa06

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae5095f7a167
Ensure Gtk window unmap workaround is actually used. r=karlt

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ae5095f7a167

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora approval on this when you get a chance.

Comment 18

[Jamie Nicol [:jnicol]]

Comment on attachment 8826594 [details]
Bug 1319764 - Ensure Gtk window unmap workaround is actually used.

Approval Request Comment
[Feature/Bug causing the regression]: Gtk3
[User impact if declined]: Graphical glitches shown in screenshot
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Fixes logic of previously safe to uplift patch so that it works more reliably. Triggers a common gtk event which is harmless.
[String changes made/needed]: N/A

Comment 19

[Julien Cristau [:jcristau]]

Comment on attachment 8826594 [details]
Bug 1319764 - Ensure Gtk window unmap workaround is actually used.

fix visual glitches on linux, aurora52+

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d73abe84cc51