Synchronous flush when closing a window

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Bookmarks & History
P1
normal
RESOLVED FIXED
4 months ago
16 days ago

People

(Reporter: Ehsan, Assigned: Neil Deakin (not available until Aug 9))

Tracking

(Blocks: 1 bug)

unspecified
Firefox 55
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox55 fixed)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

See this profile: <https://clptr.io/2jFRaGq>

tree.xml calls this.view <http://searchfox.org/mozilla-central/rev/8fa84ca6444e2e01fb405e078f6d2c8da0e55723/browser/components/places/content/tree.xml#25> which calls nsITreeBoxObject::GetView, which either does a layout or a style flush, and they're both guaranteed to do work since at this point we've torn down stuff so frames will be dirty.

It seems easy to cache this.treeBoxObject.view in a field here since it's not realistically going to change as the result of a flush.
Blocks: 1336241
I assume this was when closing the Places window?
Flags: needinfo?(ehsan)

Comment 2

4 months ago
(In reply to Mike Conley (:mconley) from comment #1)
> I assume this was when closing the Places window?

We also use tree in the Star button panel and in the sidebar. I assume this comes from the Star panel.
Though I'm not sure why this bug is under Location Bar, it should probably be under Bookmarks & History.

(In reply to :Ehsan Akhgari from comment #0)
> calls nsITreeBoxObject::GetView, which either does a
> layout or a style flush, and they're both guaranteed to do work since at
> this point we've torn down stuff so frames will be dirty.

Wouldn't be better to try to fix the tree implementation to avoid pointless flushes, than to fix every consumer? Why does GetView causes a flush if it's unlikely to change the view? Is it possible to detect that we are at a point where the flush is pointless?
(In reply to Mike Conley (:mconley) from comment #1)
> I assume this was when closing the Places window?

No, a browser window.

(In reply to Marco Bonardo [::mak] from comment #2)
> (In reply to Mike Conley (:mconley) from comment #1)
> > I assume this was when closing the Places window?
> 
> We also use tree in the Star button panel and in the sidebar. I assume this
> comes from the Star panel.
> Though I'm not sure why this bug is under Location Bar, it should probably
> be under Bookmarks & History.

OK.

> (In reply to :Ehsan Akhgari from comment #0)
> > calls nsITreeBoxObject::GetView, which either does a
> > layout or a style flush, and they're both guaranteed to do work since at
> > this point we've torn down stuff so frames will be dirty.
> 
> Wouldn't be better to try to fix the tree implementation to avoid pointless
> flushes, than to fix every consumer? Why does GetView causes a flush if it's
> unlikely to change the view?

For correctness.  Those unlikely cases do happen in practice, it's just that in the case of closing a window they don't matter.

> Is it possible to detect that we are at a point
> where the flush is pointless?

In the general case without doing part of the work required to flush the pending notifications, no.
Component: Location Bar → Bookmarks & History
Flags: needinfo?(ehsan)
Blocks: 1338595

Updated

3 months ago
Priority: -- → P3
(In reply to Marco Bonardo [::mak] from comment #2)
> Wouldn't be better to try to fix the tree implementation to avoid pointless
> flushes, than to fix every consumer?

Who is the right person to fix the tree implementation?

Updated

3 months ago
Whiteboard: [qf:p1]
Flags: needinfo?(mak77)

Comment 5

2 months ago
I don't have resources to work on this atm (unless it becomes a necessity), but I'm happy to review patches.
The only person I know with good knowledge of the tree cpp implementation is Neil Deakin, though from what Ehsan said in comment 3 it may be hard to detect if we are at a point where GetView shouldn't cause a flush.

I was mostly wondering if the tree implementation could detect that its parent window is closing and avoid the flush. But I don't know the answer off-hand.

I'm setting a needinfo to Neil in case he has ideas, otherwise someone will have to read the code and figure that out.
Flags: needinfo?(mak77) → needinfo?(enndeakin)
Created attachment 8848529 [details] [diff] [review]
treeflush-noscrollchange

I see a layout flush happening from nsTreeBodyFrame::CheckOverflow when I close the library window or quit while the tree in the star panel is open.

This looks to have been added by bug 905909 to avoid recursion when overflow/underflow events are fired. Yet, the flush occurs even when neither event is or either direction is fired. So we should be able to eliminate this at least.

I've also seen in some cases a style flush when closing the library window when clearing the view. It might be possible to use some unloaded state on the document or presshell to eliminate this. I can't reproduce it often enough to tell.

I can't load the linked profile so I can't tell which of these is the case here. This patch addresses the first case.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Flags: needinfo?(enndeakin)
Created attachment 8852726 [details] [diff] [review]
Don't do the flush if the scroll overflow hasn't changed

Pretty straight-forward fix I think.
Attachment #8848529 - Attachment is obsolete: true
Attachment #8852726 - Flags: review?(jaws)
Comment on attachment 8852726 [details] [diff] [review]
Don't do the flush if the scroll overflow hasn't changed

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

r=me on the test change. Adding Masayuki to review the nsTreeBodyFrame.cpp changes.
Attachment #8852726 - Flags: review?(masayuki)
Attachment #8852726 - Flags: review?(jaws)
Attachment #8852726 - Flags: review+
It'd be lovely if we could add a test for this to ensure that we don't accidentally introduce more sync style flushes down the road. Please see bug 1334642 comment 51.

Updated

2 months ago
Whiteboard: [qf:p1] → [photon] [qf:p1]
Comment on attachment 8852726 [details] [diff] [review]
Don't do the flush if the scroll overflow hasn't changed

Although, I'm not familiar with this class, but I don't find a better reviewer from the log...

>diff --git a/layout/xul/tree/nsTreeBodyFrame.cpp b/layout/xul/tree/nsTreeBodyFrame.cpp
>--- a/layout/xul/tree/nsTreeBodyFrame.cpp
>+++ b/layout/xul/tree/nsTreeBodyFrame.cpp
>@@ -902,16 +902,20 @@ nsTreeBodyFrame::CheckOverflow(const Scr
>         horizontalOverflowChanged = true;
>       } else if (mHorizontalOverflow && bounds.width >= mHorzWidth) {
>         mHorizontalOverflow = false;
>         horizontalOverflowChanged = true;
>       }
>     }
>   }
> 
>+  if (!horizontalOverflowChanged && !verticalOverflowChanged) {
>+    return;
>+  }

Looks reasonable to me at least in this method. However, I have a concern.

It's called by nsTreeBodyFrame::FullScrollbarsUpdate() via nsOverflowChecker. If there is no script blocker, flushing the layout synchronously.

nsTreeBodyFrame::FullScrollbarsUpdate() is called by nsTreeBodyFrame::EnsureView() via nsTreeBodyFrame::SetView(), after that it tries to update scroll position to show the selected item. However, if there is pending reflow, I'm not sure if this works as expected because gfxScrollFrame tries to clamp the scroll destination into current range.
http://searchfox.org/mozilla-central/rev/72fe012899a1b27d34838ab463ad1ae5b116d76b/layout/generic/nsGfxScrollFrame.cpp#2242,2255-2256

So, perhaps, this should be reviewed by gfxScrollFrame peer, mstange or mattwoodrow? (according to the log.)
Attachment #8852726 - Flags: review?(masayuki) → review?(mstange)

Updated

2 months ago
No longer blocks: 1338595
Comment on attachment 8852726 [details] [diff] [review]
Don't do the flush if the scroll overflow hasn't changed

Redirecting to Matt Woodrow, maybe he is more available for the review?
Attachment #8852726 - Flags: review?(mstange) → review?(matt.woodrow)
Comment on attachment 8852726 [details] [diff] [review]
Don't do the flush if the scroll overflow hasn't changed

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

I really don't know this code at all sorry.

I think Masayuki's concern is valid, but we can possibly fix that by adding a synchronous flush further up the callstack for the 'EnsureView' case (which I believe *isn't* the callstack that is slowing us down here).

Daniel reviewed the change that put this flush in, so maybe he has more ideas.
Attachment #8852726 - Flags: review?(matt.woodrow) → review?(dholbert)

Updated

a month ago
Whiteboard: [photon] [qf:p1] → [qf:p1]
Comment on attachment 8852726 [details] [diff] [review]
Don't do the flush if the scroll overflow hasn't changed

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

One commit message nit:
> Bug 1334635, don't do the flush if the scroll overflow hasn't changed
Perhaps s/don't do the flush/Don't flush layout in nsTreeBodyFrame/

Anyway - this change makes sense to me, from the perspective of what we're doing in this function at least.   Seems like we could've/should've originally made the flush in bug 905909 slightly more targeted by checking these bools (seeing if we queued up events) before we flushed.

So: r=me from that perspective.

Having said that: we probably need a "part 2" here to address the potential side effects of no-longer-reflowing-already-pending-stuff here, which masayuki hinted at in comment 10.  It sounds like we might need a second patch (or really a "part 0" earlier patch, so that we don't create an intermediate broken state in the commit log), to handle that.  mattwoodrow would probably be better suited to reviewing that part, since he already explored that callstack a bit it seems.
Attachment #8852726 - Flags: review?(dholbert) → review+

Comment 14

a month ago
Pushed by neil@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fb8edcd2a7c
don't flush layout in the tree's CheckOverflow method if the scroll overflow hasn't changed, r=jaws, dholbert
I think we need an additional patch here to add a flush in a more targeted spot, right? (per comment 12 / end of comment 13)
Flags: needinfo?(enndeakin)

Updated

a month ago
Flags: qe-verify?
Priority: P3 → P1
Whiteboard: [qf:p1] → [photon-performance] [qf:p1]
https://hg.mozilla.org/mozilla-central/rev/1fb8edcd2a7c
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

a month ago
Iteration: --- → 55.3 - Apr 17

Updated

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

Updated

a month ago
See Also: → bug 1354194
See Also: → bug 1363341
I filed bug 1363341 about this extra bit.
Flags: needinfo?(enndeakin)
You need to log in before you can comment on or make changes to this bug.