1611462 - We end up reframing the URL bar during startup

Status: RESOLVED FIXED

(Core :: Layout: Positioned, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

When I start up the browser, I see a reframe of the URL bar. The JS stack leading to it is this:

0 select() ["resource:///modules/UrlbarInput.jsm":251:20]
    <failed to get 'this' value>
1 focusAndSelectUrlBar() ["chrome://browser/content/browser.js":2904:10]
    <failed to get 'this' value>
2 _setInitialFocus/<(uriToLoad = ""about:blank"") ["chrome://browser/content/browser.js":2119:8]
3 _callWithURIToLoad(callback = [function]) ["chrome://browser/content/browser.js":2380:14]
    <failed to get 'this' value>
4 _setInitialFocus() ["chrome://browser/content/browser.js":2117:9]
    <failed to get 'this' value>
5 onDOMContentLoaded(    <Failed to get argument while inspecting stack frame>
) ["chrome://browser/content/browser.js":1787:9]
    <failed to get 'this' value>
6 onDOMContentLoaded(    <Failed to get argument while inspecting stack frame>
) ["self-hosted":876:16]
    <failed to get 'this' value>

The C++ stack, starting at the input.select() call the script makes, is this:

#30 0x00007fa40abd1c41 in nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*, nsCSSFrameConstructor::InsertionKind)
    (this=0x7fa3eb874d00, aContent=0x7fa3e5408af0, aInsertionKind=nsCSSFrameConstructor::InsertionKind::Sync) at /home/bzbarsky/mozilla/dom-work/mozilla/layout/base/nsCSSFrameConstructor.cpp:8616
#31 0x00007fa40ab97c63 in mozilla::RestyleManager::ProcessRestyledFrames(nsStyleChangeList&) (this=<optimized out>, aChangeList=...) at /home/bzbarsky/mozilla/dom-work/mozilla/layout/base/RestyleManager.cpp:1536
#32 0x00007fa40ab9c984 in mozilla::RestyleManager::DoProcessPendingRestyles(mozilla::ServoTraversalFlags) (this=<optimized out>, aFlags=<optimized out>)
    at /home/bzbarsky/mozilla/dom-work/mozilla/layout/base/RestyleManager.cpp:3081
#33 0x00007fa40ab82607 in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) (this=0x7fa3eb84f000, aFlush=...) at /home/bzbarsky/mozilla/dom-work/mozilla/layout/base/PresShell.cpp:4072
#34 0x00007fa40884f7ca in mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush) (this=0x7fa3ee5a4000, aFlush=...) at /home/bzbarsky/mozilla/dom-work/mozilla/dom/base/Document.cpp:10034
#35 0x00007fa40883ee9d in mozilla::dom::Document::FlushPendingNotifications(mozilla::FlushType) (this=0x2, aType=<optimized out>) at /home/bzbarsky/mozilla/dom-work/mozilla/dom/base/Document.cpp:9955
#36 0x00007fa408993579 in nsFocusManager::FlushAndCheckIfFocusable(mozilla::dom::Element*, unsigned int) (this=0x7fa3f2ad7510, aElement=0x7fa3f03447d0, aFlags=2)
    at /home/bzbarsky/mozilla/dom-work/mozilla/dom/base/nsFocusManager.cpp:1518
#37 0x00007fa40899225e in nsFocusManager::SetFocusInner(mozilla::dom::Element*, int, bool, bool) (this=0x7fa3f2ad7510, aNewContent=0x7fa3f03447d0, aFlags=2, aFocusChanged=true, aAdjustWidget=true)
    at /home/bzbarsky/mozilla/dom-work/mozilla/dom/base/nsFocusManager.cpp:1160
#38 0x00007fa408993412 in nsFocusManager::SetFocus(mozilla::dom::Element*, unsigned int) (this=0x7fa3f2ad7510, aElement=0x7fa3f03447d0, aFlags=2)
    at /home/bzbarsky/mozilla/dom-work/mozilla/dom/base/nsFocusManager.cpp:452
#39 0x00007fa409ba8733 in mozilla::dom::HTMLInputElement::Select() (this=0x7fa3f03447d0) at /home/bzbarsky/mozilla/dom-work/mozilla/dom/html/HTMLInputElement.cpp:3005

Looking into why it needed reframing.

Comment 1

[Boris Zbarsky [:bzbarsky]]

So far I've tracked this to us ending up with a reframe hint for <hbox id="urlbar">.

Unfortunately, rr gets pretty confused trying to track stuff through rust; I start getting "No frame is currently executing in specified block". I'll see if I can upload this into pernosco...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Alright, that worked. https://pernos.co/debug/XB1nEzV1RXE-GcUHdg5_5A/index.html#f{m[jdE,59J5_,t[AQ,UNs_,f{e[jdE,59Ju_,s{af7ZqCdAA,bAYY,oEZxd5A,uEY6nVQ___ shows what's going on: the id="urlbar" thing changes from position: static to position: relative. The real question is why this is happening dynamically. Did we start layout before all our stylesheets were loaded?

Comment 3

[Boris Zbarsky [:bzbarsky]]

No, I just checked and we don't seem to obviously do that. Or at least we're not force-starting layout: we start it when the XML is done parsing, and there are no pending sheets at that point.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Emilio, do you have any bright ideas on figuring out why we're getting this post-frame-construction restyle and reframe?

I do see some pending restyles get queued up from various shadow DOM stuff, but I think all of those happen before StartLayout....

Comment 5

[Emilio Cobos Álvarez (:emilio)]

I don't think restyles should be queued up before StartLayout, because we bail out at various places if the element is not yet styled (like here or here.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Unrelatedly we should probably not reframe for position changes if there are no placeholders in the subtree or such...

Comment 7

[Emilio Cobos Álvarez (:emilio)]

The relevant rule is:

#urlbar.megabar, #urlbar.megabar > #urlbar-input-container, #urlbar.megabar > .urlbarView {
    position: relative;
}

fwiw.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

The .megabar class gets added here, which runs lazily from here, so there's no guarantee of that code running before the initial layout, as far as I can tell. That seems a bit fishy.

So I think there are two things that should happen:

• Probably the front-end should be adjusted to ensure either that change is done before layout, or style the non-megabar with position: relative too.
• Changes from position: static to position: relative should probably not trigger an unconditional reframe, but an UpdateContainingBlock hint, which will only reframe when needed... Is there any reason that wouldn't work? Not sure it will prevent the reframe, depends on the frame tree below, but it seems an easy and nice optimization to have at least.

Comment 9

[Boris Zbarsky [:bzbarsky]]

The .megabar class gets added

Ah, I see.

Changes from position: static to position: relative should probably not trigger an unconditional reframe, but an UpdateContainingBlock hint, which will only reframe when needed... Is there any reason that wouldn't work?

I think it should. Worth trying!

Comment 10

[Emilio Cobos Álvarez (:emilio)]

To the queue then.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1611462 - Optimize position changes better. r=dholbert,dbaron,bzbarsky

When our position changes from / to absolute / fixed, then we need to reframe
(most likely, at least). But in some easier cases we can just live with a reflow
plus (optionally) a containing-block update.

We need to delete the normal position property, as described by the comment, and
also need to update the handling of UpdateContainingBlock to avoid making
decisions based on position not changing. This will actually not cause more
reframes, as one would expect, because that optimization became somewhat
obsolete by bug 1519371 (which made the optimization exact).

Comment 12

[:Gijs (out for now; he/him)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #2)

The real question is why this is happening dynamically.

I think this is the megabar stuff that's nightly-only, which is why it's dynamic. Dão, is that right? I don't know how hard it would be to do this before the initial layout..

Comment 13

[Dão Gottwald [:dao]]

(In reply to :Gijs (he/him) from comment #12)

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #2)

The real question is why this is happening dynamically.

I think this is the megabar stuff that's nightly-only, which is why it's dynamic. Dão, is that right?

That's right.

I don't know how hard it would be to do this before the initial layout..

Should be doable. How much of a perf problem is this in practice? Note that this setup is temporary as the megabar pref should be removed after we've successfully shipped it enabled by default.

Comment 14

[Boris Zbarsky [:bzbarsky]]

It's probably not a huge problem in practice.

Comment 15

[Dão Gottwald [:dao]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #14)

It's probably not a huge problem in practice.

Let's just treat this as a layout bug then.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ff238eaf45c0
Optimize position changes better. r=dholbert

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0832ea5b3a88
Use DeleteProperty rather than RemoveProperty to avoid leaking. CLOSED TREE

Comment 18

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/ff238eaf45c0
https://hg.mozilla.org/mozilla-central/rev/0832ea5b3a88