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

RESOLVED FIXED in Firefox 64

Status

()

defect
P3
normal
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: Felipe, Assigned: Felipe)

Tracking

unspecified
Firefox 64
Points:
---

Firefox Tracking Flags

(firefox64 fixed)

Details

(Whiteboard: [ohnoreflow][fxperf:p1][qf:p1:f64])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 months ago
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
Whiteboard: [ohnoreflow][fxperf:p1] → [ohnoreflow][fxperf:p1][qf:p1:f64]
Priority: -- → P3
(Assignee)

Comment 1

6 months ago
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
(Assignee)

Comment 4

6 months ago
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).
(Assignee)

Comment 5

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

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

Comment 6

6 months ago
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?
(Assignee)

Comment 8

6 months ago
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 10

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f4873573579f
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
(Assignee)

Updated

6 months ago
Blocks: 1501761
Attachment #9018507 - Attachment is obsolete: true
Attachment #9018505 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.