1717726 - Add a (debug-only) assertion disallowing <deck> has any <browser>s or <iframe>s

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Hiroyuki Ikezoe (:hiro)]

PresShell::ClearMouseCapture() has a nsLayoutUtils::IsAncestorFrameCrossDoc call which is used when a nsDeckFrame hides a panel to release mouse-captured state happened in a descendant of the panel.

Fortunately as of now, there's no <deck> having <browser> or <iframe> in our production code, there are two in our test code, test_deck_has_out_of_process_iframe.xhtml and bug454235-subframe.xhtml. The first one was added in bug 1541256 for Fission, so it's okay to remove the test. The latter was added in bug 454235, it's not clear to me but seems like it was added for Fennec in the non-E10S era so I guess it also can be obsoleted.

Note that <tabpanels> is also a -moz-deck which uses nsDeckFrame and our browser's tab uses <tabpanels> so I am going to do a special handling in the assertion to allow having <browser> elements in id="tabbrowser-tabpanels" tabpanels.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Masayuki, is test_bug454235.xhtml still valid for us? It looks like it was added for a fix IME related issue for Fennec. If it's no longer necessary I'd like to drop it.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Masayuki, is test_bug454235.xhtml still valid for us? It looks like it was added for a fix IME related issue for Fennec. If it's no longer necessary I'd like to drop it.

I don't know. But according to the test code, the expected behavior should still be valid. Shouldn't nsLayoutUtils::IsAncestorFrameCrossDoc have a new argument for making it ignore or respect thing which you want in PresShell::ClearMouseCapture?

Comment 3

[Hiroyuki Ikezoe (:hiro)]

We could change the ClearMouseCapture function to care about cases where the capturingContent in an out-of-process iframe/browser. But I don't want to spend time on the change since we will obsolete nsDeckFrame (bug 1689817) sooner or later.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

My original question is somewhat vague.

What I really wanted to ask is; once after we disallow having any <browser> or <iframe> inside <deck>, how can we test the case what test_bug454235.xhtml tried to test?

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

If it becomes an impossible case, of course, it's fine to delete the test.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Thank you Masayuki! that's being said, I found Thunderbird has <iframe> and <browser> inside <tabpanels>. Also Nika told me on #DOM channel that;

For now if the window has no remote attribute and also doesn't have a maychangeremoteness attribute it can't be remote

So I will also check the remoteness in the assertion I am going to add in this bug, so the test_bug454235.xhtml works as it is with the assertion since it doesn't have remote browsers.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Looks like Thunderbird sets maychangeremoteness attribute here.

Hey Geoff or Magnus, is this browser element inside <deck> or <tabpanels>? If so, is there any way to identify the <tabpanels> (or the <deck>)?

The background; nsDeckFrame which is used for <deck> or <tabpanels> has a special handling when an active tab (or panel) gets hidden to release captured state by mouse drag or some such, but with Fission it won't work if the captured state happens inside out-of-process iframe. So for example, once after user started dragging something in an OOP iframe inside <deck> or <tabpanels>, say resizing an textarea in the iframe, if the <tabpanel> (or <deck>) which is having the OOP iframe switches to another tab, then the textarea's resizer keeps receiving mouse mouse events even if the tab is actually background. TBH I am not sure this kind of issue happen on Thunderbird or not. In Firefox if the tabpanels is our browser's tab it should just work since bug 1680405 (TBH I am not 100% it works or not, Edgar might know), but if in other <tabpanels> or <deck> having remote <browser> or remote <iframe> it won't work either. Fortunately as of now in Firefox code base we don't have such <tabpanels>s or <deck>s either. So I'd like to avoid having such <tabpanels> and <deck> in future, thus I'd like to disallow them by adding an assertion here in this bug. So, if Thunderbird has such <tabpanels> or <deck> we need to allow them having remote <browser> or <iframe> even if the relasing captured state doesn't work currently, otherwise Thunderbird will crash on debug builds.

Note that if the releasing captured state doesn't work on Thunderbird, Edger might know how to make it work (I am CCing him).

Thanks for cooperation!

Comment 8

[Hiroyuki Ikezoe (:hiro)]

FWIW, here is the change adding the assertion what I have in my mind;
https://hg.mozilla.org/try/rev/663e8c27e4235fe91bf581f525ce05f80d618fc2

Comment 9

[Magnus Melin [:mkmelin]]

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

Thank you Masayuki! that's being said, I found Thunderbird has <iframe> and <browser> inside <tabpanels>.

Note that stuff under suite/ is SeaMonkey only, and they are still on m-c 57... (and no real plans to go further than 60 AFAIK)
Wrong file, but correct about the structure. https://searchfox.org/comm-central/source/mail/base/content/messenger.xhtml#609

Also note premalinks from searchfox comm-central mozilla dir do not work.
You asked about the browser at https://searchfox.org/mozilla-central/rev/9975889f5c0d5c59bd22121a454beba774cbae71/browser/base/content/tabbrowser.js#2056 - but this is browser/ so Firefox-only code.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1717726 - Remove browser_deck_has_out_of_process_iframe.js. r?tnikkel

We are going to disallow having any remote <browser>s or <iframe>s inside
nsDeckFrame, so this test is no longer necessary.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1717726 - Add a debug-only assertion disallowing remote <browser>s or <iframe>s inside nsDeckFrame. r?tnikkel

The background to have this assertion is that nsDeckFrame::HideBox calls
PresShell::ClearMouseCapture and ClearMouseCapture checks whether the
being-hidden panel has a content capturing mouse events or not but the check
doesn't work if the content is in a remote process. In our current
mozilla-central tree, there is no such nsDeckFrame other than our browser's
tab. In the case of our browser's tab when switching tabs, i.e. hiding an
active tab, clearing the mouse capturing state has (should have) worked since
E10S (Note for Fission cases it has worked since bug 1680405). So, because
nsDeckFrame will be obsoleted sooner or later, we disallow the situation
for other cases instead of adding special handling for the other case.

Depends on D119066

Comment 12

[Hiroyuki Ikezoe (:hiro)]

(In reply to Magnus Melin [:mkmelin] from comment #9)

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

Thank you Masayuki! that's being said, I found Thunderbird has <iframe> and <browser> inside <tabpanels>.

Note that stuff under suite/ is SeaMonkey only, and they are still on m-c 57... (and no real plans to go further than 60 AFAIK)
Wrong file, but correct about the structure. https://searchfox.org/comm-central/source/mail/base/content/messenger.xhtml#609

Thank you Magnus! Then I am going to land the change to add the assertion in m-c.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8255bf62f070
Remove browser_deck_has_out_of_process_iframe.js. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/a83914c4bef7
Add a debug-only assertion disallowing remote <browser>s or <iframe>s inside nsDeckFrame. r=tnikkel

Comment 14

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/8255bf62f070
https://hg.mozilla.org/mozilla-central/rev/a83914c4bef7

Comment 15

[Geoff Lankow (:darktrojan)]

Sorry I didn't reply earlier, I thought Magnus had answered adequately. Apparently not.

Our tabpanels element has id tabpanelcontainer instead of tabbrowser-tabpanels so it gets caught out here. I'm happy to start another bug and make a patch for it.