Closed Bug 1710533 Opened 1 year ago Closed 1 year ago

Infinite repaint loop on Windows with transparent panels in test_panel.xhtml

Categories

(Core :: Widget: Win32, defect, P3)

defect

Tracking

()

RESOLVED FIXED
90 Branch
Tracking Status
firefox-esr78 --- wontfix
firefox88 --- wontfix
firefox89 --- wontfix
firefox90 --- fixed

People

(Reporter: emilio, Assigned: sotaro)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression, Whiteboard: [proton-cleanup])

Attachments

(1 file)

+++ This bug was initially created as a clone of Bug #1710486 +++

Similar to bug 1710486, there's another test failure (after applying that fix) which looks like a hang with my patch for bug 1708735 (which only tweaks frontend stuff and causes most panels to be considered non-opaque):

https://treeherder.mozilla.org/jobs?repo=try&revision=bb7b4afe31ae79d2b3ba8712af8fd4d88d2223e5&selectedTaskRun=MrC0jFZBQBiYHeNyzRA-8g.0

However, it isn't a real hang like bug 1710486. After looking a bit into it, we just get stuck doing infinite repaint messages (unless you move the window manually or something).

There are two tests which show this issue and at first I thought were intermittents, but they reproduce locally 100% of the time:

  • toolkit/content/tests/chrome/test_panel.xhtml
  • layout/xul/test/test_windowminmaxsize.xhtml

I worked around the issue in the second test in the try run above by using an opaque background in the problematic panel: https://hg.mozilla.org/try/diff/82457fcdb0ee421f469ff93714dce346c99da9bc/layout/xul/test/titledpanelwindow.xhtml

But of course that's lame and we should probably figure out the root cause. This looks a bit like bug 1709998.

Sotaro, Chris, I plan to look into this tomorrow, but any chance you have any ideas of how to debug this?

Flags: needinfo?(sotaro.ikeda.g)
Flags: needinfo?(cmartin)
Summary: Infinite repaint loop with transparent panels in test_panel.xhtml → Infinite repaint loop on Windows with transparent panels in test_panel.xhtml

(And to be clear, I confirmed that the patch in bug 1709998 doesn't help, as expected, since these are 1proc mochitests and don't even use e10s).

It might be related to Bug 1709864. I am going to look into the bug today.

nsWindow::OnPaint() flooding could happen like Bug 1709864 comment 5.

I could reproduce the problem locally with toolkit/content/tests/chrome/test_panel.xhtml.

When the problem happened, size was changed from (150, 50) to (150, 73) and re-trigger invalidation

Size min was set by nsContainerFrame::SetSizeConstraints().

Hmm, current D114501 at Bug 1709864 did not address the problem.

When the D114501 applied, repaint was triggered from nsView::DoResetWidgetBounds() since curBounds(250, 262, 150, 73) and newBounds(250, 262, 150, 50) happened.

It seems that current widget size constraint needs to be applied to newBounds. But nsBaseWidget::ResizeClient() looks a bit tricky.

Flags: needinfo?(sotaro.ikeda.g)

I am not sure why desktopDelta exists in nsBaseWidget::ResizeClient().

It is zero in normal situation on Windows. I saw non-zero value with STR of Bug 1701451. In this case, window size was updated unexpectedly by ::UpdateLayeredWindow(). And nsView::DoResetWidgetBounds() does not care the desktopDelta for comparing curBounds.TopLeft() and newBounds for triggering nsBaseWidget::ResizeClient().

See Also: → 1709864

(In reply to Sotaro Ikeda [:sotaro] from comment #7)

I am not sure why desktopDelta exists in nsBaseWidget::ResizeClient().

:jfkthame, do you know why desktopDelta is necessary?

Flags: needinfo?(jfkthame)
Attachment #9221268 - Attachment description: WIP: Bug 1710533 - Apply the widget size constraints to newBounds → Bug 1710533 - Apply the widget size constraints to newBounds

D114814 addressed Infinite repaint loop with test_panel.xhtml for me.

Thanks for looking into this Sotaro!

(In reply to Sotaro Ikeda [:sotaro] from comment #8)

(In reply to Sotaro Ikeda [:sotaro] from comment #7)

I am not sure why desktopDelta exists in nsBaseWidget::ResizeClient().

:jfkthame, do you know why desktopDelta is necessary?

I'm not sure of the details offhand but I think this may relate to how window management is different across platforms -- the "desktop" coordinate system corresponds to device pixels on Windows IIRC, but to Cocoa's "points" (logical pixels) on macOS (and Linux? I don't remember...), and the mapping to device pixels varies depending which screen the widget is on. In particular, in a mixed-DPI configuration it doesn't work to simply convert "desktop" coordinates (the things the OS APIs use to manage windows) to device pixels (which a lot of Gecko wants to use) by multiplying by the display's scale factor, because this can result in overlapping device-pixel coordinates and confusion about where the widget belongs.

Basically... it's complicated, and platform-dependent. And so when touching this stuff, it's good to test across multiple platforms, and with various multi-monitor/mixed-resolution configurations. Unfortunately, we don't have any automated test coverage for situations like that.

Flags: needinfo?(jfkthame)

Hmm, thank you for detailed explanation. Then it is not simple to remove it.

What happens with nsView::DoResetWidgetBounds() is like the following.

-[1] nsView::DoResetWidgetBounds() resizes widget to nsView::CalcWidgetBounds(nsWindowType aType) size if the size is different than widget->GetClientBounds().
-[2] nsView::DoResetWidgetBounds() calls widget->ResizeClient() to resize widget
-[3] nsBaseWidget::ResizeClient() calls Resize() to resize widget. But the ResizeClient() might change the resize value.
-[4] The Resize() might change the resize value by using nsBaseWidget::ConstrainSize()
+ For example on Windows, nsWindow::Resize()
-[5] Final resized value could be different than nsView::CalcWidgetBounds(), nsWindow::Invalidate() triggers repaint and DoResetWidgetBounds() on Windows.

Then nsView::CalcWidgetBounds() size might be applied to widget with modification. And next widget->GetClientBounds() could be different than nsView::CalcWidgetBounds() again
with several reasons. But it seems OK to apply widget->ConstrainSize() in nsView::DoResetWidgetBounds(). It could remove repaint because of widget->ConstrainSize() call in the Resize() as in [4].

Adding haik and mstange because they dealt with widget size constraints issues on mac recently.

(In reply to Sotaro Ikeda [:sotaro] from comment #14)

But it seems OK to apply widget->ConstrainSize() in nsView::DoResetWidgetBounds(). It could remove repaint because of widget->ConstrainSize() call in the Resize() as in [4].

I agree that that should at best be harmless. Do you plan to send that change for review? It'd unblock me :)

Assignee: nobody → sotaro.ikeda.g
Status: NEW → ASSIGNED
Assignee: sotaro.ikeda.g → nobody
Status: ASSIGNED → NEW
Whiteboard: [proton-cleanup]
Assignee: nobody → sotaro.ikeda.g
Status: NEW → ASSIGNED
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/64abec51f1cb
Apply the widget size constraints to newBounds r=mstange,gfx-reviewers
Backout by abutkovits@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/453466931a6c
Backed out changeset 64abec51f1cb for causing failures at test_panel.xhtml. CLOSED TREE
Priority: -- → P3

When the test failed,requested height in nsView::DoResetWidgetBounds() was less than mMinSize.height and desktopDelta was positive(16.0, 39.0). In this case, requested final widget size in nsWindow::Resize() was "mMinSize.height + desktopDelta.height". It happened because nsView::DoResetWidgetBounds() does not care about desktopDelta. This problem is ouf of scope for this bug. Then I am going to relax the size check.

Attachment #9221268 - Attachment description: Bug 1710533 - Apply the widget size constraints to newBounds → WIP: Bug 1710533 - Apply the widget size constraints to newBounds
Attachment #9221268 - Attachment description: WIP: Bug 1710533 - Apply the widget size constraints to newBounds → Bug 1710533 - Apply the widget size constraints to newBounds
Pushed by sikeda.birchill@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3f007ac3eea6
Apply the widget size constraints to newBounds r=mstange,gfx-reviewers
Flags: needinfo?(cmartin)
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch
Duplicate of this bug: 1709864
Blocks: sw-wr-popups
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.