0.96ms uninterruptible reflow at gotoPref@chrome://browser/content/preferences/in-content/preferences.js:195:3

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM
P1
normal
RESOLVED FIXED
2 months ago
24 days ago

People

(Reporter: Robert Ward, Assigned: bz)

Tracking

unspecified
mozilla55
Points:
---
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [ohnoreflow][qf:p1] [reserve-photon-performance])

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

2 months ago
Here's the stack:

gotoPref@chrome://browser/content/preferences/in-content/preferences.js:195:3
init_all/<@chrome://browser/content/preferences/in-content/preferences.js:67:50
_fireOnSelect@chrome://global/content/bindings/richlistbox.xml:78:13
selectItem@chrome://global/content/bindings/listbox.xml:255:11
onxblmousedown@chrome://global/content/bindings/listbox.xml:1033:13


bug 750519 related? 

preferences.js:67:
categories.addEventListener("select", event => gotoPref(event.target.value));


preferences.js:195:
  mainContent.scrollTop = 0;

Updated

2 months ago
Flags: qe-verify?
Priority: -- → P2

Comment 1

2 months ago
It's not worth doing anything about this since the reflow is 1) cheap and 2) happening inside the preferences window rather than the browser chrome window.
Blocks: 1348289
No longer blocks: 1363750
Status: NEW → RESOLVED
Last Resolved: 2 months ago
Resolution: --- → WONTFIX
(In reply to Dão Gottwald [::dao] from comment #1)
> and 2)
> happening inside the preferences window rather than the browser chrome
> window.

I don't think I understand this point - about:preferences runs by default in the parent process, and so a reflow like this will also affect the browser chrome, despite not technically being contained within it.

Updated

2 months ago
Flags: qe-verify?
Priority: P2 → --
Whiteboard: [ohnoreflow][qf][photon-performance] → [ohnoreflow][qf]

Comment 3

2 months ago
(In reply to Mike Conley (:mconley) - At work week, slow to respond from comment #2)
> (In reply to Dão Gottwald [::dao] from comment #1)
> > and 2)
> > happening inside the preferences window rather than the browser chrome
> > window.
> 
> I don't think I understand this point - about:preferences runs by default in
> the parent process, and so a reflow like this will also affect the browser
> chrome, despite not technically being contained within it.

It's related to (1) as the preferences document is simpler than the browser document, so reflows will be cheaper. It's true that about:preferences can block the browser UI, but that's doesn't mean we can't do any work in this document. In all likelihood, it's already doing way more expensive things than this reflow.

I'm assuming that the reflow happens while the user is interacting with about:preferences. If this happens at random times even if about:preferences is not the foreground tab, that would probably be something we should fix.
(In reply to Dão Gottwald [::dao] from comment #1)
> It's not worth doing anything about this since the reflow is 1) cheap

I'm not sure this point is correct, as the time in ms isn't very relevant without knowing how fast the machine was.

I would also really be curious to know why setting scrollTop to 0 causes a sync reflow; is that a platform bug? (I'll try to profile this to find a stack of the actual reflow)
Component: Untriaged → Preferences
Here is the relevant platform part of the stack from the profiler:
PresShell::Flush Layout
nsDocument::FlushPendingNotifications(mozilla::FlushType)
mozilla::dom::Element::GetScrollFrame(nsIFrame**, bool)
mozilla::dom::Element::SetScrollTop(int)
mozilla::dom::ElementBinding::set_scrollTop
mozilla::dom::GenericBindingSetter(JSContext*, unsigned int, JS::Value*)
js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct)
js::CallSetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<JS::Value>)
js::NativeSetProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::QualifiedBool, JS::ObjectOpResult&)
Interpret
js::RunScript(JSContext*, js::RunState&)
gotoPref
Created attachment 8867543 [details] [diff] [review]
Possible patch

Maybe it's as simple as not flushing in the special case where this is set to 0? What I would really like is if someone who understands this code could review all the GetScrollFrame calls in that file to verify if there are not other cases where we should skip flushing.

And btw, I agree that the preferences window case itself may not be worth a lot of effort, but web content may also use the scrollTop setter, so this may still be worth optimizing on the platform side.
Discussed in Performance Team meeting and bug has been reopened.
No longer blocks: 1348289
Status: RESOLVED → REOPENED
Flags: qe-verify?
Priority: -- → P2
Resolution: WONTFIX → ---
Whiteboard: [ohnoreflow][qf] → [ohnoreflow][qf] [photon-performance]

Updated

a month ago
Status: REOPENED → NEW

Updated

a month ago
Flags: qe-verify? → qe-verify-

Comment 8

a month ago
Are there some edge cases where flushing is needed even with value 0?
I'm thinking a case where the element doesn't yet have nsIFrame, and flushing would create it and then .scrollTop = 0 would scroll to 0. But if we don't flush, there won't be nsIFrame at all, and scrolling wouldn't happen, and when we later flush, we scroll down.
But I can't now think of way to scroll down when frame is created.
Right, if a new frame is created it will be scrolled to top, so as if scrollTop got set to 0 anyway.

There's an edge case here which can go wrong, though.  Say this is a quirks mode document, the element is the <body>, there is content in the <html> that is _not_ inside the <body>, the <body> is currently display:none, and someone first sets the <body> to display:block and then sets scrollTop on it to 0.

In quirks mode, if the body doesn't have a scrollframe of its own, this needs to scroll the _viewport_.  With the patch as attached it won't scroll anything, because Element::GetScrollFrame is arguably buggy: it returns immediately if !frame instead of doing viewport stuff.  But even if we fixed that, it would not help much because in the example above the frame would not be around for us to do checks on.

So what I think we should probably do is:

1) Change Element::GetScrollFrame to press on to the "is this the root thing?" if !frame.
2) Change Element::GetScrollFrame to actually use IsPotentiallyScrollable on the <body> to
   get the right behavior around this stuff.
3) We should also check why nsIDocument::GetScrollingElement does a layout, not frame,
   flush; that part seems wrong.
4) We should change Element::GetScrollFrame to take a FlushType, not a boolean.  Callers
   that currently pass true can pass FlushType::Layout.  Callers that currently pass false
   can pass FlushType::None.  The caller we're looking at can pass FlushType::Frames when
   setting scrollTop to 0, and FlushType::Layout otherwise, with a comment explaining why.
I think ehsan added a fast path for window.scrollTo(0, 0) in bug 1365830. Can we use that here instead?
(In reply to Mike Conley (:mconley) from comment #10)
> I think ehsan added a fast path for window.scrollTo(0, 0) in bug 1365830.
> Can we use that here instead?

Apparently not. And apparently the bug is in DOM and can affect not just about:preferences but actual websites.
Component: Preferences → DOM
Product: Firefox → Core
Whiteboard: [ohnoreflow][qf] [photon-performance] → [ohnoreflow][qf:p1] [photon-performance]

Updated

a month ago
Priority: P2 → P3
Whiteboard: [ohnoreflow][qf:p1] [photon-performance] → [ohnoreflow][qf:p1] [reserve-photon-performance]
Created attachment 8871032 [details] [diff] [review]
part 1.  Make Element::GetScrollFrame follow the spec more closely in the quirks mode case
Attachment #8871032 - Flags: review?(ehsan)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Created attachment 8871033 [details] [diff] [review]
part 2.  Change GetScrollingElement to just flush frames, not layout

We care about the state of frames (and styles, but flushing frames of course
requires flushing styles) to determine IsPotentiallyScrollable, but we don't
care about layout information.
Attachment #8871033 - Flags: review?(ehsan)
Created attachment 8871035 [details] [diff] [review]
part 3.  Make Element::GetScrollFrame take a flush type, not a "should I flush?" boolean
Attachment #8871035 - Flags: review?(ehsan)
Created attachment 8871036 [details] [diff] [review]
part 4.  Don't flush layout when setting scrollTop to 0
Attachment #8871036 - Flags: review?(ehsan)

Updated

a month ago
Priority: P3 → P1
Attachment #8867543 - Attachment is obsolete: true
Comment on attachment 8871033 [details] [diff] [review]
part 2.  Change GetScrollingElement to just flush frames, not layout

Review of attachment 8871033 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDocument.cpp
@@ +10609,2 @@
>      HTMLBodyElement* body = GetBodyElement();
>      if (body && !IsPotentiallyScrollable(body)) {

We can still avoid this flush if there is no body in this case, and it seems like we shouldn't leave this optimization opportunity on the table here.
Attachment #8871033 - Flags: review?(ehsan) → review+
Comment on attachment 8871032 [details] [diff] [review]
part 1.  Make Element::GetScrollFrame follow the spec more closely in the quirks mode case

Review of attachment 8871032 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/Element.cpp
@@ +700,4 @@
>    }
>  
>    nsIDocument* doc = OwnerDoc();
> +  if (this == OwnerDoc()->GetScrollingElement()) {

Nit: it may not be apparent to the reader of this code that there is a flush happening in the callee here, do you mind adding a comment about that here?
Attachment #8871032 - Flags: review?(ehsan) → review+
Attachment #8871035 - Flags: review?(ehsan) → review+
Comment on attachment 8871036 [details] [diff] [review]
part 4.  Don't flush layout when setting scrollTop to 0

Review of attachment 8871036 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks a lot!
Attachment #8871036 - Flags: review?(ehsan) → review+
Created attachment 8871332 [details] [diff] [review]
Interdiff for part 1 per our discussion and the review comments
Created attachment 8871334 [details] [diff] [review]
part 1.  Make Element::GetScrollFrame follow the spec more closely in the quirks mode case
Attachment #8871032 - Attachment is obsolete: true
Attachment #8871332 - Flags: review?(ehsan)
Comment on attachment 8871332 [details] [diff] [review]
Interdiff for part 1 per our discussion and the review comments

Review of attachment 8871332 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you!
Attachment #8871332 - Flags: review?(ehsan) → review+

Comment 22

a month ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c315bd6cf00
part 1.  Make Element::GetScrollFrame follow the spec more closely in the quirks mode case.  r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/e670c0dcaa9f
part 2.  Make Element::GetScrollFrame take a flush type, not a "should I flush?" boolean.  r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd1c5aecc373
part 3.  Don't flush layout when setting scrollTop to 0.  r=ehsan

Comment 23

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3c315bd6cf00
https://hg.mozilla.org/mozilla-central/rev/e670c0dcaa9f
https://hg.mozilla.org/mozilla-central/rev/dd1c5aecc373
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months agoa month ago
status-firefox55: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

a month ago
Iteration: --- → 55.6 - May 29
Depends on: 1370072
You need to log in before you can comment on or make changes to this bug.