Affected versions

• 80.0a1 (20200714083249)

Affected platforms

• Ubuntu 18.04.

Steps to reproduce

1. Open Firefox with a new profile.
2. Click the Hamburger menu and observe the arrow.

Expected result

• Arrow is displayed as expected.

Actual result

• Arrow it’s truncated.

Regression range

• Last good revision: 6087e976924f95018479c6f5881878c95b8bd8e2 (2020-07-05)
• First bad revision: 6cedb9c51fd839f6de2812cac6b95c5b9f4c7716 (2020-07-06)
• Pushlog:
  https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6087e976924f95018479c6f5881878c95b8bd8e2&tochange=6cedb9c51fd839f6de2812cac6b95c5b9f4c7716
• First regression search mozregression pointed out to bug 1635153, and then to bug 1647556 due to intermittency.

Notes

• Attached a screen recording.

Severity S3

Nical, this looks like something that's potentially caused by bug 1635153 - can you take a look?

(Looks like I have no permissions to make comments on D83908, so I'll write here.)

I've never tapped into lower-level GTK bits, so I'm not sure either. But from what I found in the GTK documentation, the patch does the right thing. However the comment about gdk_window_get_origin() is a bit off. GTK documentation says that gdk_window_get_origin() obtains the position of a window in root window coordinates. And it emphasizes that it's relative to root window, not relative to parent window coordinates that gdk_window_get_position() returns.

On the other hand, docs also mention that GDK_CONFIGURE events are not issued for windows which type is GDK_WINDOW_CHILD. So looks like recursive update of children is the right thing to do: there is no other way for children to invalidate cached positions.

By the way, gdk_window_get_position() looks a lot cheaper to call. It returns relative coordinates, yes. But absolute coordinates can be calculated by querying parents recursively. And for the topmost parent coordinates can be updated in .OnConfigureEvent(). From what I understand, calls to gdk_window_get_origin() can be avoided completely.

(Looks like I have no permissions to make comments on D83908, so I'll write here.)

huh, odd I didn't find any weird flag on the review request, oh well.

Thanks for looking at it, I updated the comment. It looks like gdk_window_get_position doesn't chat with the x server so it indeed sounds like it'd be even better. Right now I'm mostly interested in fixing the regression but I'll file a bug about this and perhaps look into it if it shows up during profiling.

(In reply to Nicolas Silva [:nical] from comment #4)

I updated the comment.

I forgot about gdk_window_get_children(). It returns a list that should be freed with g_list_free().
And I can't find anything else that needs to be changed in the patch. Looks good.

Does this patch actually fix the bug? I can't reproduce the issue locally, so can't verify. And I doubt that browser window was moved, so coordinate caching shouldn't have any effect.
Perhaps, it is caused by menu animation missing a last frame? Looks very similar to bug 1650246.

I forgot about gdk_window_get_children(). It returns a list that should be freed with g_list_free().
And I can't find anything else that needs to be changed in the patch. Looks good.

Thanks, fixed the patch.

Does this patch actually fix the bug?

I couldn't reproduce this particular instance of the bug either, but by stuffing the whole file with checks that the cached origin equals the result of gdk_window_get_origin, I could see before the patch that the checks would fail a lot, and with this patch it mostly doesn't fail. Why there are still failing checks I am not certain, but I suspect the x server isn't maintaining a "transactional" state accounting for changes that are in the event queue but not processed yet, so when you query something synchronously you might race with configure events that are still in the queue. That doesn't necessarily matter when we handle size-related things in configure events but breaks the way I was testing for this bug.

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

Hi! Unfortunately, I can still reproduce the issue on my Ubuntu 18.0.4 with Firefox 80.0a1 (20200726214746). Attached a screen recording. Should we reopen this bug? Thanks!

This bug is frustrating. I can't actually reproduce this one but I can fairly easily reproduce bug 1653711 which I am pretty sure is the same thing. I tried implementing gdk_window_get_origin in terms of gdk_window_get_position as suggested by Rinat, and I still get the same issue happening about 50% of the time. It's interesting because that new version doesn't do any caching, so it isn't a classical cache invalidation bug.

My current theory is that there is something very racy happening towards the creation of widgets, and this raciness is hidden by the many synchronous queries we are issuing via gdk_window_get_origin. I'm going to be away for two weeks starting next, so I'll probably have to back out the window origin caching and reevaluate later.

In fact it was pretty easy to confirm that the problem is caused by the absence of xserver syncrhonization rather than how the window origin is calculated: I tried first calling gdk_window_get_origin, then throwing away the result and computing the window origin by traversing the parent hierarchy and accumulating gdk_window_get_position, and the bug doesn't reproduce anymore.

The new implementation is based on traversing the parent window hierarchy and accumulating relative positions. The implementation unfortunately has to be disabled while another issue is investigated.

This patch will need an uplift to 80.

Julien, FYI, potential uplift request ^ .

(In reply to Nicolas Silva [:nical] from comment #12)

Created attachment 9166811 [details]
Bug 1652743 - Replace Window origin caching with another implementation. r=ibragimovrinat

Looks good to me. As far as I understand, this change effectively reverts window coordinate caching, right?

(In reply to Rinat from comment #15)

Looks good to me. As far as I understand, this change effectively reverts window coordinate caching, right?

Yes (sadly), I'll have a look at the race condition after I come back from PTO, and hopefully enable the non-blocking implementation.

https://hg.mozilla.org/mozilla-central/rev/038c4d4d8fb0

Comment on attachment 9166811 [details]
Bug 1652743 - Replace Window origin caching with another implementation. r=ibragimovrinat

Beta/Release Uplift Approval Request

• User impact if declined: On linux popup[ windows render incorrectly around creation, until hovered.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This change is simple and reverts the code to a state very close to before the regressing patch/
• String changes made/needed: None.

Comment on attachment 9166811 [details]
Bug 1652743 - Replace Window origin caching with another implementation. r=ibragimovrinat

regression fix on gtk, approved for 80.0b4

https://hg.mozilla.org/releases/mozilla-beta/rev/a7392cef6680

I can no longer reproduce the issue with Firefox 81.0a1 (20200806033456) and Firefox 80.0b4 (20200804180257) on Ubuntu 18.04.