1595425 - 0.13ms uninterruptible reflow at _removeURLFormat@resource:///modules/UrlbarValueFormatter.jsm:181:5

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P3)

Description

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

Here's the stack:

_removeURLFormat@resource:///modules/UrlbarValueFormatter.jsm:181:5
update@resource:///modules/UrlbarValueFormatter.jsm:55:10
formatValue@resource:///modules/UrlbarInput.jsm:257:27
onSecurityChange@chrome://browser/content/browser.js:5660:13
callListeners@chrome://browser/content/tabbrowser.js:823:31
_callProgressListeners@chrome://browser/content/tabbrowser.js:837:22
updateCurrentBrowser@chrome://browser/content/tabbrowser.js:1093:14
_setupEventListeners/<@chrome://browser/content/tabbrowser.js:5104:16
set selectedIndex@chrome://global/content/elements/tabbox.js:208:14
set selectedPanel@chrome://global/content/elements/tabbox.js:227:7
set selectedIndex@chrome://global/content/elements/tabbox.js:559:11
set selectedItem@chrome://global/content/elements/tabbox.js:579:35
_selectNewTab@chrome://global/content/elements/tabbox.js:749:7
on_mousedown@chrome://global/content/elements/tabbox.js:345:28
on_mousedown@chrome://browser/content/tabbrowser-tab.js:361:15
handleEvent@chrome://global/content/customElements.js:468:27

Version: 72.0a1
Build: 20191108095936

Comment 1

[Florian Quèze [:florian]]

Mike, do you have a guess at what in this code https://searchfox.org/mozilla-central/rev/6566d92dd46417a2f57e75c515135ebe84c9cef5/browser/components/urlbar/UrlbarValueFormatter.jsm#181-192 could trigger a layout flush? It's not obvious to me. Steps to reproduce, or even better a profile would be very useful here.

Comment 2

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

Yes, I have STR.

1. Install OhNoReflow in Firefox Nightly: https://mikeconley.github.io/ohnoreflow/
2. Ensure the add-on is installed, enabled and recording
3. Have multiple tabs open to various web pages
4. Select the URL bar in one such tab
5. Switch to a different tab via mouse

ER:

Ideally, no reflows being recorded.

AR:

A number of reflows recorded, including the one in comment 0.

Comment 3

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

The line in question is:

https://searchfox.org/mozilla-central/rev/6566d92dd46417a2f57e75c515135ebe84c9cef5/browser/components/urlbar/UrlbarValueFormatter.jsm#181

I'm not sure why getting at the value of an HTMLInputElement would flush. Maybe emilio would know?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

That's a setter, right? Off-hand I don't know, but I could take a look.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

maybe it's related to the fact we use the :valid pseudo-class to check if the element contains a value, we make the element visible only if it has a value.
https://searchfox.org/mozilla-central/rev/e7c61f4a68b974d5fecd216dc7407b631a24eb8f/browser/base/content/browser.css#651

Comment 6

[Florian Quèze [:florian]]

Here is a profile of it: https://perfht.ml/379V3g3

The flush is at https://searchfox.org/mozilla-central/rev/e7c61f4a68b974d5fecd216dc7407b631a24eb8f/editor/libeditor/TextEditor.cpp#533

emilio, does this help?

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Ah, yes, it does help a bunch!

I see... So this should only happen when there's text selected, or the input is focused, right?

This seems to have been introduced in bug 1568996, and it seems expected as far as I can tell... But I don't know if we could avoid it in some cases? It does seem unfortunate than setting the value to the empty string has to flush layout, as the selection should be completely removed.

Probably Makoto or Masayuki, which are way more familiar with this code, could help more than me :)

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Alternatively you could avoid style changes before clearing the value, but that seems it may be hard, I dunno.

Comment 9

[Makoto Kato [:m_kato]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)

Ah, yes, it does help a bunch!

I see... So this should only happen when there's text selected, or the input is focused, right?

This seems to have been introduced in bug 1568996, and it seems expected as far as I can tell... But I don't know if we could avoid it in some cases? It does seem unfortunate than setting the value to the empty string has to flush layout, as the selection should be completely removed.

Probably Makoto or Masayuki, which are way more familiar with this code, could help more than me :)

FrameSelection::MoveCaret doesn't work when frame is dirty since editor holds script blocker when callign FrameSeleciton::MoveCaret. (Before landing bug 1568996, although MoveCaret calls pending flush, layout won't flushed due to holding script block.)

Also, If this script is from content, this doesn't occur. Because we don't use DeleteSelectionAsAction and doesn't add transaction history. If from XUL, since we have to add transaction, we use DeleteSelectionAsAction now. But this case may have to add new method to clear value, not using mozilla::TextEditor::DeleteSelectionAsAction in mozilla::TextControlState::SetValueWithTextEditor (or add something hold to block flush layout.)

Comment 10

[Makoto Kato [:m_kato]]

Bug 1358025 is this case's text transaction, but this is removed since we don't create an traction to set value.

Comment 11

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

Well, TextEditor::ExtendSelectionForDelete() does nothing if selection is collapsed. Perhaps, we could avoid the flush with making it wraps with a method and use it in DeleteSelectionAsAction().

Comment 12

[Makoto Kato [:m_kato]]

https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=276723419&revision=adbb265a99d1e692331818ae14be10282d99f976

Comment 13

[Makoto Kato [:m_kato]]

Attachment: Bug 1595425 - Don't flush layout when setting empty string on Chrome. r=masayuki

This is a regression by bug 1568996. Although editor uses nsFrameSelection to
move caret, if frame is dirty, nsFrameSelection returns error.
So by bug 1568996, we flush layout before calling nsFrameSelection. But we
should stop flushing layout when we don't use nsFrameSelection.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bee6ebf7e96b
Don't flush layout when setting empty string on Chrome. r=masayuki

Comment 15

[Cristina Coroiu [:ccoroiu]]

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

Comment 16

[Alexandru Ionescu (needinfo me) [:alexandrui]]

== Change summary for alert #24120 (as of Mon, 25 Nov 2019 04:55:21 GMT) ==

Improvements:

14% raptor-tp6-twitch-firefox-cold fcp windows10-64-shippable opt 87.83 -> 75.42
10% raptor-tp6-twitch-firefox-cold windows10-64-shippable opt 274.17 -> 247.64
9% raptor-tp6-twitch-firefox-cold windows10-64-shippable opt 273.32 -> 248.34

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=24120