Closed Bug 1595425 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: Address Bar, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Firefox 72
Tracking Status
firefox-esr68 --- unaffected
firefox70 --- unaffected
firefox71 --- wontfix
firefox72 --- fixed

People

(Reporter: mconley, Assigned: m_kato)

References

(Regression)

Details

(Keywords: regression, Whiteboard: [ohnoreflow][fxperf:p3] )

Attachments

(1 file)

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

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.

Flags: needinfo?(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.

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?

Flags: needinfo?(mconley) → needinfo?(emilio)

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

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

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 :)

Flags: needinfo?(emilio) → needinfo?(m_kato)
Regressed by: 1568996

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

Priority: -- → P3

(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.)

Flags: needinfo?(m_kato)

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

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().

Assignee: nobody → m_kato
Whiteboard: [ohnoreflow][fxperf] → [ohnoreflow][fxperf:p3]

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.

Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/bee6ebf7e96b
Don't flush layout when setting empty string on Chrome. r=masayuki
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 72

== 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

You need to log in before you can comment on or make changes to this bug.