Closed Bug 1614268 Opened 5 years ago Closed 5 years ago

Land a mochitest for bug 1550800

Categories

(Core :: Layout, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla77
Fission Milestone M6
Tracking Status
firefox77 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(2 files)

Attached patch The mochitest in question (deleted) — Splinter Review

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.

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

Fission Milestone: --- → M6
Flags: needinfo?(hikezoe.birchill)

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!

Flags: needinfo?(hsivonen)
Flags: needinfo?(hikezoe.birchill)
Flags: needinfo?(enndeakin)

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

Flags: needinfo?(enndeakin)

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.

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?

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

Flags: needinfo?(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.

Flags: needinfo?(hikezoe.birchill)
Assignee: nobody → hikezoe.birchill
Status: NEW → ASSIGNED
Flags: needinfo?(hikezoe.birchill)
No longer blocks: 1617983
Pushed by hsivonen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e26494488d1f A mochitest for drop down menu position in OOP iframes. r=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?

Flags: needinfo?(hsivonen) → needinfo?(hikezoe.birchill)

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.

Flags: needinfo?(hikezoe.birchill)

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

Attachment #9133984 - Attachment description: Bug 1614268 - A mochitest for drop down menu position in OOP iframes. r?hsivonen → Bug 1614268 - A mochitest for drop down menu position in OOP iframes. r=hsivonen
Pushed by hikezoe.birchill@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/828ed052b1a9 A mochitest for drop down menu position in OOP iframes. r=hsivonen
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla77
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: