1369488 - test_panel_anchoradjust.xul times out when OMTC is enabled for popups

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Martin Stránský [:stransky] (ni? me)]

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

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

Comment 1

[Martin Stránský [:stransky] (ni? me)]

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.

Comment 2

[Martin Stránský [:stransky] (ni? me)]

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;

Comment 3

[Martin Stránský [:stransky] (ni? me)]

I'm not sure but this may be related:

https://bugzilla.mozilla.org/show_bug.cgi?id=1241851#c4

Comment 4

[Martin Stránský [:stransky] (ni? me)]

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.

Comment 5

[Martin Stránský [:stransky] (ni? me)]

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.

Comment 6

[Martin Stránský [:stransky] (ni? me)]

seems to be related to Bug 1109868

Comment 7

[Martin Stránský [:stransky] (ni? me)]

Attachment: simple-1369488.patch

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.

Comment 8

[Timothy Nikkel (: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?

Comment 9

[Martin Stránský [:stransky] (ni? me)]

(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.

Comment 10

[Martin Stránský [:stransky] (ni? me)]

Not sure it's related but the mViewManagerFlushIsPending is reset by RevokeViewManagerFlush() call from nsViewManager::ProcessPendingUpdates() so the nsRefreshDriver is stopped and popup hangs.

Comment 11

[Timothy Nikkel (:tnikkel)]

(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?

Comment 12

[Martin Stránský [:stransky] (ni? me)]

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 *******************************

Comment 13

[Timothy Nikkel (:tnikkel)]

(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.

Comment 14

[Martin Stránský [:stransky] (ni? me)]

(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.

Comment 15

[Timothy Nikkel (: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.

Comment 16

[Martin Stránský [:stransky] (ni? me)]

Thanks, I'll do the debugging then.

Comment 17

[Martin Stránský [:stransky] (ni? me)]

(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.

Comment 18

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1369488 - Force update popups anchored to hidden deck box, @gmail.com

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

Comment 19

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8877038 [details]
Bug 1369488 - Force update popups anchored to hidden deck box, @gmail.com

https://reviewboard.mozilla.org/r/148384/#review153036

Comment 20

[Carsten Book [:Tomcat]]

need final review in mozreview so that we can push this. Can you fix this, thanks!

Comment 21

[Martin Stránský [:stransky] (ni? me)]

(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.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/33b0a8337b9e
Force update popups anchored to hidden deck box, r=[e-mail]

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/33b0a8337b9e