Closed Bug 1369488 Opened 3 years ago Closed 3 years ago

test_panel_anchoradjust.xul times out when OMTC is enabled for popups

Categories

(Core :: Layout, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: stransky, Assigned: stransky)

References

(Blocks 1 open bug)

Details

(Whiteboard: tpi:+)

Attachments

(1 file, 1 obsolete file)

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

Follow up from https://bugzilla.mozilla.org/show_bug.cgi?id=1364355#c20
This may be related:

[task 2017-05-27T03:15:35.348220Z] 03:15:35     INFO - TEST-START | toolkit/content/tests/chrome/test_panel_anchoradjust.xul
[task 2017-05-27T03:21:02.641661Z] 03:21:02     INFO - TEST-INFO | started process screentopng
[task 2017-05-27T03:21:02.981855Z] 03:21:02     INFO - TEST-INFO | screentopng: exit 0
[task 2017-05-27T03:21:02.983073Z] 03:21:02     INFO - Buffered messages logged at 03:15:35
[task 2017-05-27T03:21:02.983912Z] 03:21:02     INFO - must wait for load
[task 2017-05-27T03:21:02.984659Z] 03:21:02     INFO - must wait for focus

I can reproduce the timeout locally and there's also focus mentioned:

74 INFO TEST-OK | toolkit/content/tests/chrome/test_panel_anchoradjust.xul | took 264562ms
75 INFO Error: Unable to restore focus, expect failures and timeouts.

Also when the test is running without user interaction it times out. When I hit keyboard/mouse it continues immediately which looks like a focus problem.
The test freezes here:

// When the deck containing the anchor changes to a different page, the panel should be hidden.
popuphidden = waitForPanel(panel, "popuphidden");
document.getElementById("deck").selectedIndex = 1;
await popuphidden;
I'm not sure but this may be related:

https://bugzilla.mozilla.org/show_bug.cgi?id=1241851#c4
Depends on: 630346
As far as I can tell the bug involves nsXULPopupManager::UpdatePopupPositions() / nsRefreshDriver.

The (item->Frame()->PresContext()->RefreshDriver() == aRefreshDriver) check fails at nsXULPopupManager::UpdatePopupPositions() so item->CheckForAnchorChange() which trigger popup close is never called.

I still have no idea why aRefreshDriver for OMTC popups does not match here.
Summary: test_panel_anchoradjust.xul times out when OMTC is enabled on ARGB windows → test_panel_anchoradjust.xul times out when OMTC is enabled for popups
Depends on: 1356317
Component: Widget: Gtk → Layout
So there's the situation why the test hangs:

when nsRefreshDriver::Tick() runs, nsRefreshDriver connected to pending nsXULPopupManager requests is removed at the beginning because (ObserverCount() == 0 && ImageRequestCount() == 0). 

so

nsXULPopupManager* pm = nsXULPopupManager::GetInstance();
if (pm) {
   pm->UpdatePopupPositions(this);
}
 
is never called then and the popup hangs unless user trigger any event (mouse/keyboard/focus) which restarts the nsRefreshDriver which is added back. I don't know why this scenario happens only for OMTC popups.
seems to be related to Bug 1109868
Depends on: 1109868
Attached patch simple-1369488.patch (obsolete) — Splinter Review
Asking for feedback since you reviewed the patch which caused this regression. Not sure it's a correct one (actually I doubt it's correct) but merely a starting point.
Attachment #8874835 - Flags: feedback?(tnikkel)
Hmm, changing the selected index for deck could likely happen without causing any refresh observers to be added. So I think we need to call UpdatePopupPositions in nsDeckFrame::IndexChanged similar to how we call it in nsXULPopupManager::HidePopupCallback for when a popup is anchored to something in another popup that has closed. Does that fix the problem?
Attachment #8874835 - Flags: feedback?(tnikkel)
(In reply to Timothy Nikkel (:tnikkel) from comment #8)
> Hmm, changing the selected index for deck could likely happen without
> causing any refresh observers to be added. So I think we need to call
> UpdatePopupPositions in nsDeckFrame::IndexChanged similar to how we call it
> in nsXULPopupManager::HidePopupCallback for when a popup is anchored to
> something in another popup that has closed. Does that fix the problem?

No that does not help here - nsDeckFrame::IndexChanged is called before the actual popup update so UpdatePopupPositions() does not have any effect here. Also the popup nsRefreshDriver is actually running at the time.

I compared the OMTC and basic paths and looks like the OMTC popup nsRefreshDriver is removed due to zero observers. When running the basic compositor path, nsRefreshDriver attached to the popup has mViewManagerFlushIsPending flag set which causes the referenced nsRefreshDriver is not removed but performs the popup hidding.
Not sure it's related but the mViewManagerFlushIsPending is reset by RevokeViewManagerFlush() call from nsViewManager::ProcessPendingUpdates() so the nsRefreshDriver is stopped and popup hangs.
(In reply to Martin Stránský from comment #9)
> No that does not help here - nsDeckFrame::IndexChanged is called before the
> actual popup update so UpdatePopupPositions() does not have any effect here.
> Also the popup nsRefreshDriver is actually running at the time.

What do you mean by "the actual popup update"? Is the popup already hidden before we get to this code somehow?

(In reply to Martin Stránský from comment #2)
> The test freezes here:
> 
> // When the deck containing the anchor changes to a different page, the
> panel should be hidden.
> popuphidden = waitForPanel(panel, "popuphidden");
> document.getElementById("deck").selectedIndex = 1;
> await popuphidden;

Presumably the popup should get hidden as a result of the line setting selectedIndex because it changes the deck to show a "card" that does not have the anchor of the popup.

So we want the popup to be hidden as a result of calling nsDeckFrame::IndexChanged. Am I misunderstanding?
Sorry, that may be caused by my misunderstanding to that code. There's the failing sequence, a log from test + my debug prints:

GECKO(10105) | nsDeckFrame::IndexChanged()
GECKO(10105) | [0x7f7256258400] nsRefreshDriver::ScheduleViewManagerFlush
GECKO(10105) | [0x7f7256258400] nsRefreshDriver::ScheduleViewManagerFlush -> mViewManagerFlushIsPending = 1
GECKO(10105) | nsDeckFrame::HideBox()
GECKO(10105) | nsDeckFrame::IndexChanged - driver 0x7f7256258400, frame_hidding = 0x7f7252fb29e0
GECKO(10105) | nsXULPopupManager::UpdatePopupPositions(): 1369, item = 0x7f7255247b40
GECKO(10105) | popup = 0x7f7255247b40
GECKO(10105) | item driver = 0x7f7256258400
GECKO(10105) | given driver = 0x7f7256258400
GECKO(10105) | nsXULPopupManager::UpdatePopupPositions(): 1374

-> There's the match, after nsDeckFrame::IndexChanged() the nsRefreshDriver 0x7f7256258400 is called and popup 0x7f7255247b40 is processed in UpdatePopupPositions(). I wonder why the popup is not closed here?

GECKO(10105) | nsMenuChainItem::CheckForAnchorChange(): 123
GECKO(10105) | nsMenuPopupFrame::CheckForAnchorChange()
GECKO(10105) | [0x7f7259acd400] nsRefreshDriver::ScheduleViewManagerFlush
GECKO(10105) | [0x7f7259acd400] nsRefreshDriver::ScheduleViewManagerFlush -> mViewManagerFlushIsPending = 1
GECKO(10105) | 0x7f7256258400 nsViewManager::ProcessPendingUpdates() - RevokeViewManagerFlush()

mViewManagerFlushIsPending is revoked here, nsRefreshDriver has no observers so it's removed.

GECKO(10105) | [0x7f7256258400] ObserverCount() mViewManagerFlushIsPending = 0
GECKO(10105) | [0x7f7256258400] ObserverCount() = 0
GECKO(10105) | [0x7f7256258400] ImageRequestCount() = 0
GECKO(10105) | [0x7f7256258400] ObserverCount() mViewManagerFlushIsPending = 0
GECKO(10105) | [0x7f7256258400] nsRefreshDriver line 1778
GECKO(10105) | [0x7f7256258400] nsRefreshDriver::StopTimer() 0x7f7256258400
GECKO(10105) | [0x7f726a164700] RemoveRefreshDriver 0x7f7256258400

so we don't have any nsRefreshDriver for our pending popup = 0x7f7255247b40. Next call of nsXULPopupManager::UpdatePopupPositions() does not process it because it's called from different driver. I don't know why the popup is still here and not closed by the previous update.

GECKO(10105) | nsXULPopupManager::UpdatePopupPositions(): 1369, item = 0x7f7255247b40
GECKO(10105) | popup = 0x7f7255247b40
GECKO(10105) | item driver = 0x7f7256258400
GECKO(10105) | given driver = 0x7f7259acd400
GECKO(10105) | **************** Hang *******************************
(In reply to Martin Stránský from comment #12)
> GECKO(10105) | nsDeckFrame::IndexChanged()
> GECKO(10105) | [0x7f7256258400] nsRefreshDriver::ScheduleViewManagerFlush
> GECKO(10105) | [0x7f7256258400] nsRefreshDriver::ScheduleViewManagerFlush ->
> mViewManagerFlushIsPending = 1
> GECKO(10105) | nsDeckFrame::HideBox()
> GECKO(10105) | nsDeckFrame::IndexChanged - driver 0x7f7256258400,
> frame_hidding = 0x7f7252fb29e0
> GECKO(10105) | nsXULPopupManager::UpdatePopupPositions(): 1369, item =
> 0x7f7255247b40
> GECKO(10105) | popup = 0x7f7255247b40
> GECKO(10105) | item driver = 0x7f7256258400
> GECKO(10105) | given driver = 0x7f7256258400
> GECKO(10105) | nsXULPopupManager::UpdatePopupPositions(): 1374
> 
> -> There's the match, after nsDeckFrame::IndexChanged() the nsRefreshDriver
> 0x7f7256258400 is called and popup 0x7f7255247b40 is processed in
> UpdatePopupPositions(). I wonder why the popup is not closed here?

This seems to be where the problem is happening. If the "card" of the deck frame that contains the anchor for the popup is not the selected card then the IsVisibleConsideringAncestors call in nsMenuPopupFrame::CheckForAnchorChange should be returning false and the popup hidden based on that.
(In reply to Timothy Nikkel (:tnikkel) from comment #13)
> This seems to be where the problem is happening. If the "card" of the deck
> frame that contains the anchor for the popup is not the selected card then
> the IsVisibleConsideringAncestors call in
> nsMenuPopupFrame::CheckForAnchorChange should be returning false and the
> popup hidden based on that.

Can you please more concrete how to fix that? I can write a patch for it but I'm a total newbie at the layout code. Thanks.
Flags: needinfo?(tnikkel)
I can't be more concrete about how to fix it because I don't know what the problem is yet. More debugging is needed to know what the root problem is. Finding out if indeed the IsVisibleConsideringAncestors call is returning false is happening, and why that is would be the first step to that.
Flags: needinfo?(tnikkel)
Thanks, I'll do the debugging then.
(In reply to Timothy Nikkel (:tnikkel) from comment #8)
> Hmm, changing the selected index for deck could likely happen without
> causing any refresh observers to be added. So I think we need to call
> UpdatePopupPositions in nsDeckFrame::IndexChanged similar to how we call it
> in nsXULPopupManager::HidePopupCallback for when a popup is anchored to
> something in another popup that has closed. Does that fix the problem?

Actually this is a correct fix. I had a wrong patch where UpdatePopupPositions() was called before mIndex update, so the popup was actually still anchored to the visible element. When calling UpdatePopupPositions() after mIndex set the testcase works fine. I'll attach a patch for it.
Attachment #8874835 - Attachment is obsolete: true
Comment on attachment 8877038 [details]
Bug 1369488 - Force update popups anchored to hidden deck box, @gmail.com

https://reviewboard.mozilla.org/r/148384/#review153036
Attachment #8877038 - Flags: review?(tnikkel) → review+
Keywords: checkin-needed
need final review in mozreview so that we can push this. Can you fix this, thanks!
Flags: needinfo?(stransky)
Keywords: checkin-needed
(In reply to Carsten Book [:Tomcat] from comment #20)
> need final review in mozreview so that we can push this. Can you fix this,
> thanks!

Seems to be a bug in mozreview, filed as Bug 1372858.
Flags: needinfo?(stransky)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/33b0a8337b9e
Force update popups anchored to hidden deck box, r=tnikkel@gmail.com
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/33b0a8337b9e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Assignee: nobody → stransky
You need to log in before you can comment on or make changes to this bug.