1614268 - Land a mochitest for bug 1550800

Status: RESOLVED FIXED

(Core :: Layout, task, P3)

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: The mochitest in question

I wrote a mochitest based on what Neil provided to me in bug 1550800 comment 9. But the mochitest calls Element.focus() which doesn't yet work in fission due to bug 1556627.

Comment 1

[Chris Peterson [:cpeterson]]

Hiro, SetFocus bug 1556627 has been fixed. Is your new <select> popup mochitest ready to land now?

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Henri and Neil, now this test works properly in terms of the test purpose, but the test leaks a window object which is for the data URL in the mochitest.

:kmag identified the leaked window is mActiveWindow in nsFocusManager and he thinks it's a regression from bug 1556627. It does quite make sense to me because I didn't see the leak before bug 1556627 even if there is Element.focus() call in the OOP iframe.

Can you guys please look the leak? You can easily see it by run the mochitest locally. Thanks!

Comment 3

[Henri Sivonen (:hsivonen)]

We set mFocusedWindow in WindowRaised in content processes without clearing it.

Comment 4

[Henri Sivonen (:hsivonen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd76290b1750284b0dd1b1cb0996e0919945a97e

Comment 5

[Henri Sivonen (:hsivonen)]

The patch in the try run addresses the problem with the mochitest attached here, but pretty much everything else goes very orange. This indicates, unfortunately, that mActiveWindow is still being relied upon in content processes.

Comment 6

[Andrew McCreight [:mccr8]]

Can mActiveWindow be made into a weak reference? Is there a scenario where we want the focus manager to be the only thing keeping a window alive?

Comment 7

[Henri Sivonen (:hsivonen)]

(In reply to Andrew McCreight [:mccr8] from comment #6)

Can mActiveWindow be made into a weak reference?

Possibly, but window proxies are already rather special, so I'm not confident that making them support weak references would be OK.

Anyway, I understand where this problem is coming from. However, the dependencies of fixing it are frustratingly numerous, so fixing everything that blocks fixing this properly takes more time than anyone would like. No show-stoppers so far, though!

Is there a scenario where we want the focus manager to be the only thing keeping a window alive?

No.

Comment 8

[Henri Sivonen (:hsivonen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=14f7ab6969cb13e2e6ea999fcb19aba83db03294

Comment 9

[Henri Sivonen (:hsivonen)]

I've now posted patches for the blockers for review. hiro, could you, please, upload the mochitest patch to Phabricator. I think if I do it, the Phabricator metadata might get confusing.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1614268 - A mochitest for drop down menu position in OOP iframes. r=hsivonen

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e26494488d1f
A mochitest for drop down menu position in OOP iframes. r=hsivonen

Comment 12

[Andrei Ciure[:aciure]]

Backed out changeset e26494488d1f (bug 1614268) for causing browser_select_popup_position_in_out_of_process_iframe.js failures
https://hg.mozilla.org/integration/autoland/rev/52c711dbc6f86783c4c47622e06a3577bf99e634

push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=295803377&resultStatus=testfailed%2Cbusted%2Cexception&revision=e26494488d1f3cd51f915b32db7089716ba9786b

Comment 13

[Henri Sivonen (:hsivonen)]

The reason for failure is not the window leak until shutdown that was blocking this earlier. The failure is on Mac. The try run from comment 8 was on Linux.

hiro, now the test seems to be failing on what it was supposed to be testing. Can you take a look, please?

Comment 14

[Hiroyuki Ikezoe (:hiro)]

There are three issues I've noticed;

1. On MacOSX the popup menu is positioned at (0,0) of select element whereas on other platforms the popup is positioned below at the select slement
2. It seems like the second argument for BrowserTestUtils.browserLoaded should be true which is to wait for sub frame's load completion (that said I've never seen failures caused by this)
3. At least on my macbook screenPixelsPerCSSPixel needs to be factored into positions to be used by synthesizeNativeMouseClick

But still there are two different failures on MacOSX _verify runs;

TEST-UNEXPECTED-FAIL | layout/base/tests/browser_select_popup_position_in_out_of_process_iframe.js | Uncaught exception - at chrome://mochitests/content/browser/layout/base/tests/browser_select_popup_position_in_out_of_process_iframe.js:72 - TypeError: can't access property "browsingContext", content.document.querySelector(...) is null

and

TEST-UNEXPECTED-FAIL | layout/base/tests/browser_select_popup_position_in_out_of_process_iframe.js | leaked 1 window(s) until shutdown [url = chrome://browser/content/browser.xhtml]

The former looks like bug 1444703, it seems like BrowserTestUtils.browserLoaded doesn't wait properly for a subdocument load specified by data url.

I have no idea about the latter failure, maybe we still have a race condition that a window is leaked?

Anyways, I am going to skip this test on MacOSX verify runs for now.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #14)

Anyways, I am going to skip this test on MacOSX verify runs for now.

Filed bug 1627874 to enable this test on MacOSX verify runs.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/828ed052b1a9
A mochitest for drop down menu position in OOP iframes. r=hsivonen

Comment 17

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/828ed052b1a9