1459327 - Dragging the last tab into another window temporarily breaks text editing selection and cursor

Status: NEW

(Core :: DOM: Selection, defect, P3)

Description

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Steps to reproduce:
1. Open a new window
2. Go to a page with a text field (e.g. data:text/html,<textarea> )
3. Drag that tab back into the old window

Expected result:
Text field behaves normally, with visible cursor and selection

Actual result:
Cursor and selection are invisible, though text can still be entered and edited. Spelling errors are highlighted ok. Switching to another tab fixes it, navigating to another page in the same tab does not.

Possibly bug 1399840 involves something similar?

Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=fab7bacb59284801d9c47f6ef5d0529f5ae20154&tochange=9e59ca9dd30d73c57dad6a7e6220b159dd1e6db5

Due to this range and how the bug seems to only happen with the last tab being dragged out of the window, I suspect bug 1391704 is part of the cause.

Occasionally this will cause a crash in debug at:
https://searchfox.org/mozilla-central/rev/53afcfdbabed96883126d0ebbcac499b358e32f2/gfx/layers/apz/src/FocusState.cpp#45
unfortunately `this` is optimized away.
This is the stack:
 	xul.dll!mozilla::layers::FocusState::IsCurrent(const mozilla::BaseAutoLock<mozilla::Mutex> & aProofOfLock) Line 45
 	xul.dll!mozilla::layers::FocusState::CanIgnoreKeyboardShortcutMisses() Line 236
 	xul.dll!mozilla::layers::APZCTreeManager::ReceiveInputEvent(mozilla::InputData & aEvent, mozilla::layers::ScrollableLayerGuid * aOutTargetGuid, unsigned __int64 * aOutInputBlockId) Line 1428
 	xul.dll!mozilla::layers::APZInputBridgeParent::RecvReceiveKeyboardInputEvent(const mozilla::KeyboardInput & aEvent, nsEventStatus * aOutStatus, mozilla::KeyboardInput * aOutEvent, mozilla::layers::ScrollableLayerGuid * aOutTargetGuid, unsigned __int64 * aOutInputBlockId) Line 153
 	xul.dll!mozilla::layers::PAPZInputBridgeParent::OnMessageReceived(const IPC::Message & msg__, IPC::Message * & reply__) Line 485
 	xul.dll!mozilla::gfx::PGPUParent::OnMessageReceived(const IPC::Message & msg__, IPC::Message * & reply__) Line 1111
 	xul.dll!mozilla::ipc::MessageChannel::DispatchSyncMessage(const IPC::Message & aMsg, IPC::Message * & aReply) Line 2109
 	xul.dll!mozilla::ipc::MessageChannel::DispatchMessageW(IPC::Message && aMsg) Line 2068
 	xul.dll!mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask & aTask) Line 1918
 	xul.dll!mozilla::ipc::MessageChannel::MessageTask::Run() Line 1951
 	xul.dll!nsThread::ProcessNextEvent(bool aMayWait, bool * aResult) Line 1093
 	xul.dll!NS_ProcessNextEvent(nsIThread * aThread, bool aMayWait) Line 519
 	xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate) Line 125
 	xul.dll!mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate * aDelegate) Line 301
 	xul.dll!MessageLoop::RunInternal() Line 326
 	xul.dll!MessageLoop::RunHandler() Line 320
 	xul.dll!MessageLoop::Run() Line 300
 	xul.dll!nsBaseAppShell::Run() Line 159
 	xul.dll!nsAppShell::Run() Line 401
 	xul.dll!XRE_RunAppShell() Line 893
 	xul.dll!mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate * aDelegate) Line 269
 	xul.dll!MessageLoop::RunInternal() Line 326
 	xul.dll!MessageLoop::RunHandler() Line 320
 	xul.dll!MessageLoop::Run() Line 300
 	xul.dll!XRE_InitChildProcess(int aArgc, char * * aArgv, const XREChildData * aChildData) Line 723
 	xul.dll!mozilla::BootstrapImpl::XRE_InitChildProcess(int argc, char * * argv, const XREChildData * aChildData) Line 69
 	firefox.exe!content_process_main(mozilla::Bootstrap * bootstrap, int argc, char * * argv) Line 51
 	firefox.exe!NS_internal_main(int argc, char * * argv, char * * envp) Line 280
 	firefox.exe!wmain(int argc, wchar_t * * argv) Line 132

Comment 1

[Makoto Kato [:m_kato]]

It seems to be focus manager or selection.

Comment 3

[Florian Quèze [:florian]]

This bug is caused by the hack at https://searchfox.org/mozilla-central/rev/943a6cf31a96eb439db8f241ab4df25c14003bb8/browser/base/content/tabbrowser.js#3053-3060 Commenting out this code "fixes" the bug.

Given that this hack currently covers only the case where we are tearing off the last tab of a window, and that the issue it's working around exists for other tabs too, it may be time to find a better solution.

I think the problem we have is:
- when we move a tab across windows, we first create a new tab and a new browser in the adopting window. The new browser's url is about:blank.
- then we swap the docshells of the tab that's being moved and the new about:blank tab.
- when the refresh driver ticks, we repaint in both windows, which means we paint something (about:blank?) in the content area of the old window, causing visible flicker.
- we finally close the old tab.

Here is a profile of moving a tab across windows. It shows that during the refresh driver tick, we repaint 2 browser.xul windows: https://perfht.ml/2LRaSgq

Markus, do you have any idea about how we could indicate that the old browser is about to be closed and should never get repainted (both to avoid visual flicker, and save some CPU time)? Or do you know who I should ask?

Comment 5

[Markus Stange [:mstange]]

> Markus, do you have any idea about how we could indicate that the old browser is about to be closed and should never get repainted (both to avoid visual flicker, and save some CPU time)? Or do you know who I should ask?

Maybe it's possible to suspend the refresh driver at that point. Hiro, is this something that's exposed to JS?

Comment 6

[Hiroyuki Ikezoe (:hiro)]

I don't recall what I did. :)  But if you want to stop the refresh driver work, you can expose nsRefreshDriver::Freeze() and Thaw().  Oh, nsIContentViewer.pausePainting/resumePainting [1] look exactly what you are looking for (the functions end up calling nsRefreshDriver::Freeze/Thaw).

That's said, I guess it looks not sufficient for me since setTimeout callbacks or some other UI event callbacks might still run even if the refresh driver stops, IIUC.  That might be not the case here though. :)

[1] https://searchfox.org/mozilla-central/rev/d47c829065767b6f36d29303d650bffb7c4f94a3/docshell/base/nsIContentViewer.idl#313

Comment 7

[Florian Quèze [:florian]]

This nsIContentViewer.pausePainting API looks really promising. It doesn't seem to be enough though. When I tried it in the browser console, some times some of the areas of the browser window still got repainted.

And when I replace https://searchfox.org/mozilla-central/rev/943a6cf31a96eb439db8f241ab4df25c14003bb8/browser/base/content/tabbrowser.js#3053-3060 with:
      win.document.docShell.contentViewer.pausePainting();
I still see the page being replaced with an empty gray area before the window disappears.

Are there other things that can trigger a repaint?

Comment 8

[Florian Quèze [:florian]]

I think it's this paint that's causing the flicker: https://perfht.ml/2NREuej

Comment 9

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] from comment #8)
> I think it's this paint that's causing the flicker: https://perfht.ml/2NREuej

Markus, is there any way to skip this paint? Or any other direction I should explore?

Comment 10

[Markus Stange [:mstange]]

This paint is triggered by the window focus change, I think. So we probably *do* want to paint in order to update the window's appearance to the inactive one.

It would be great if we could visually close the window before the tab swap. As far as I understand it, we'll be closing the window afterwards anyway, right? And we only keep it open for that long because closing top level windows destroys their content and would prevent the tab swap from happening, is that also right?
From the widget side, closing a window without destroying it is absolutely supported (it's what we do for menupopups - we close and reopen those windows all the time). I don't know how much work it would be to expose this functionality for top level windows.

Comment 11

[Florian Quèze [:florian]]

(In reply to Markus Stange [:mstange] from comment #10)
> This paint is triggered by the window focus change, I think. So we probably
> *do* want to paint in order to update the window's appearance to the
> inactive one.

It's a window we are about to close, so I would rather not update its appearance.

> It would be great if we could visually close the window before the tab swap.
> As far as I understand it, we'll be closing the window afterwards anyway,
> right? And we only keep it open for that long because closing top level
> windows destroys their content and would prevent the tab swap from
> happening, is that also right?

Yes.

> From the widget side, closing a window without destroying it is absolutely
> supported (it's what we do for menupopups - we close and reopen those
> windows all the time). I don't know how much work it would be to expose this
> functionality for top level windows.

The closest I've found for now is to set nsIBaseWindow.visibility to false, which makes the widget window disappear without destroying it. Actually, this bug is a side effect of setting the window visibility to false. So I guess another route would be to have someone figure out what in the platform code breaks focus when the window is hidden.

Comment 12

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Too late for a fix for 63, but we can still take a patch for 65/64.

Comment 13

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Marking fix-optional for 64. We could still take a patch for 65, and if it's verified and doesn't seem risky, could still take fixes for 64 as well.