1334635 - Synchronous flush when closing a window

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

[(no longer active)]

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.

Comment 1

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

I assume this was when closing the Places window?

Comment 2

[Marco Bonardo [:mak]]

(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?

Comment 3

[(no longer active)]

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

Comment 4

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(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?

Comment 5

[Marco Bonardo [:mak]]

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.

Comment 6

[Neil Deakin]

Attachment: 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.

Comment 7

[Neil Deakin]

Attachment: Don't do the flush if the scroll overflow hasn't changed

Pretty straight-forward fix I think.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 9

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

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.

Comment 10

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

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

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

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?

Comment 12

[Matt Woodrow (:mattwoodrow)]

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.

Comment 13

[Daniel Holbert [:dholbert]]

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.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

Comment 15

[Daniel Holbert [: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)

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1fb8edcd2a7c

Comment 17

[Neil Deakin]

I filed bug 1363341 about this extra bit.