1499883 - 1.41ms uninterruptible reflow at onxbldragend@chrome://browser/content/tabbrowser.xml:1825:15

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect, P3)

Description

[:Felipe Gomes (needinfo for replies!)]

Here's the stack:

onxbldragend@chrome://browser/content/tabbrowser.xml:1825:15

Version: 64.0a1
Build: 20181017165417


Steps to reproduce:

 - Have an overflowed tab bar
 - Detach tab to a new window


The reflow is triggered by the code that checks if the drag position is close enough to the tab strip to decide if it should be a detach or not

https://searchfox.org/mozilla-central/rev/eef79962ba73f7759fd74da658f6e5ceae0fc730/browser/base/content/tabbrowser.xml#1824

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1499883 - Fix EventUtils.synthesizePlainDragAndDrop to enable simulating tab detaching. r=florian

This fixes several problems with this function:
 - the parameters destX and destY were advertised but were actually unused
 - X/Y values are now assigned for all drag events (previously only on mouse events)
 - added support for dropping on an invalid drop target (by passing destElement = null)
 - implements the dragend event

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1499883 - Avoid a reflow on tab detaching by calculating in advance the value needed by ondragend. r=florian

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1499883 - Add reflow tests for tab detaching. r=florian

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Fixing this turned into a rabbit hole of trying to emulate proper tab detaching via drag and drop. There are several tests in the tree that claim they do that, but none actually does :P

(it also bumped into some confusion due to wrong documentation on MDN about dragend, which I'll fix soon).


The patch doesn't get 100% rid of the chance for this reflow, but it significantly reduces it. We wanted to get this one out of the way since we identified it much more common to happen in practice.

A more general solution, which should also get rid of the whitelisted reflows on the test, will be done in a separate bug (likely bug 1358816).

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Updated patch to use getBoundsWithoutFlushing, which should not totally eliminate this reflow.

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

Comment 6

[:Felipe Gomes (needinfo for replies!)]

The test works well on all platforms (it's stable on test-verify), but for some reason it leaks a tab/window at the end, and I can't figure out why.

Is it fine to land it with skip-if = debug while I investigate it?

Comment 7

[:Felipe Gomes (needinfo for replies!)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=37b529cf3c545c46ae8765253d1ddc2b75e94165&selectedJob=206687732

Comment 8

[:Felipe Gomes (needinfo for replies!)]

I haven't been able to figure out the test leak yet.. I even rewrote the test to start from a (second) new window instead of using the existing window, but the problem still happens [1]. I believe it could be a real leak related to tab detaching, but I'm not sure yet.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=c81564c6630b7a819c03c2c00d68a020f71a4992


I think the actual fix is definitely reasonable, and I didn't want it to miss 64, so I landed it and will follow-up on a separate bug to continue the work on this reflow test for tab detaching.

Comment 9

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4873573579f0a2937036a45009802541cda2d3a

Comment 10

[Noemi Erli[:noemi_erli]]

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