1358832 - 0.98ms uninterruptible reflow at swapDocShells@chrome://global/content/bindings/browser.xml:1414:13

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, enhancement, P1)

Description

[hazel ward]

Here's the stack:

swapDocShells@chrome://global/content/bindings/browser.xml:1414:13
_swapBrowserDocShells@chrome://browser/content/tabbrowser.xml:3145:13
swapBrowsersAndCloseOther@chrome://browser/content/tabbrowser.xml:3042:15
adoptTab@chrome://browser/content/tabbrowser.xml:3583:11
onxbldrop@chrome://browser/content/tabbrowser.xml:6863:11

Comment 1

[Florian Quèze [:florian]]

The JS code is: http://searchfox.org/mozilla-central/rev/313e5199bf58200f158c6fcbe193e41b88ed58a6/toolkit/content/widgets/browser.xml#1413
            this.swapFrameLoaders(aOtherBrowser);

The actual flush is at: http://searchfox.org/mozilla-central/rev/313e5199bf58200f158c6fcbe193e41b88ed58a6/dom/base/nsFrameLoader.cpp#1937

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Why is this p1?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Florian Quèze [:florian] [:flo] from comment #1)
> The actual flush is at:
> http://searchfox.org/mozilla-central/rev/
> 313e5199bf58200f158c6fcbe193e41b88ed58a6/dom/base/nsFrameLoader.cpp#1937
Is that really the right place? Are you using single process FF?

http://searchfox.org/mozilla-central/rev/313e5199bf58200f158c6fcbe193e41b88ed58a6/dom/base/nsFrameLoader.cpp#1494-1495
would be for remote tabs.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Anyhow, the relevant code has been there since swapping frameloaders was added.
bz might recall the reasoning.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I wish I had documented this....

It's possible that there was a weird flash of white or whatnot when dragging between different-sized windows?  Either that or test failures would be my best guesses at this point.

If removing those flushes does not cause orange and if dragging between different-sized windows doesn't show weird visual artifacts, they can probably go away.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

ok, thanks. Let me push removal patch to try.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: no_flush_in_swap.diff

Comment 9

[Andrew Overholt [:overholt]]

[qf:p1] -> [qf] for reconsideration since (quoting Michael Layzell) "[it] only happens when dragging tabs into other windows, and is such a minor performance hitch".

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8865637 [details] [diff] [review]
no_flush_in_swap.diff

Can't see relevant failures on try, and locally this looked ok.
But this is stuff which needs some manual testing on all the desktop platforms.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8865637 [details] [diff] [review]
no_flush_in_swap.diff

r=me

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec8394bff424
don't flush layout when swapping frameloaders, r=bz

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ec8394bff424

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #5)
> I wish I had documented this....
> 
> It's possible that there was a weird flash of white or whatnot when dragging
> between different-sized windows?  Either that or test failures would be my
> best guesses at this point.
> 
> If removing those flushes does not cause orange and if dragging between
> different-sized windows doesn't show weird visual artifacts, they can
> probably go away.

(In reply to Olli Pettay [:smaug] from comment #10)
> Can't see relevant failures on try, and locally this looked ok.
> But this is stuff which needs some manual testing on all the desktop
> platforms.

Marco, can we get some QA help to verify nothing regressed here?

Comment 15

[Mike Conley (:mconley) (:⚙️)]

For QA:

For testing this one, you'll want to test dragging tabs from one window into another pre-existing window.

Might also be worth testing the case where a tab is dragged into a window on a different display.

Comment 16

[Adrian Florinescu [:aflorinescu] [PTO until Aug 26]]]

Environment:
57.0a1 20170824100243
55.0.2 20170814142917

Ubuntu 16.04, Windows 10x64, Mac OSX 10.12

Tested on: 
-multi-display drag and drop and/or different display resolutions ;
-drag and drop + resizing up and down;