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

RESOLVED FIXED in Firefox 52

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: JuliaC, Assigned: jnicol)

Tracking

({regression})

50 Branch
mozilla53
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 wontfix, firefox51 wontfix, firefox52 fixed, firefox53 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 attachment)

[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
Summary: {Linux} Important graphic glitches on Developer Edition First Run UI tour → [Linux] Important graphic glitches on Developer Edition First Run UI tour
Does the problem go away when you force the hardware acceleration in the developer edition?
Flags: needinfo?(iulia.cristescu)
(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.
Flags: needinfo?(iulia.cristescu)
(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
Lee, can you reproduce this?
Flags: needinfo?(lsalzman)
Priority: -- → P3
Whiteboard: [gfx-noted]
(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.
Flags: needinfo?(lsalzman)
Jamie, if you can reproduce, can you take a look at this?  Lee's a bit swamped with printing.
Flags: needinfo?(jnicol)
Looking in to it
Flags: needinfo?(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.
Assignee: nobody → jnicol
Version: Trunk → 50 Branch
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.
Flags: needinfo?(karlt)
Flags: needinfo?(andrew)
Blocks: 1225044
Flags: needinfo?(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 on attachment 8826594 [details]
Bug 1319764 - Ensure Gtk window unmap workaround is actually used.

https://reviewboard.mozilla.org/r/104528/#review105530
Attachment #8826594 - Flags: review?(karlt) → review+
Flags: needinfo?(andrew)
Commit message updated. Tests look good (this only affects gtk/linux): https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca6f63834c32e479241fe4ecf99b549f6edaaa06
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ae5095f7a167
Ensure Gtk window unmap workaround is actually used. r=karlt
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ae5095f7a167
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Please request Aurora approval on this when you get a chance.
Flags: needinfo?(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
Flags: needinfo?(jnicol)
Attachment #8826594 - Flags: approval-mozilla-aurora?
Comment on attachment 8826594 [details]
Bug 1319764 - Ensure Gtk window unmap workaround is actually used.

fix visual glitches on linux, aurora52+
Attachment #8826594 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.