1382918 - Clicking bookmarks button in overflow menu hangs Firefox

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Winson Wen]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170720030203

Steps to reproduce:

Pin bookmarks button to Overflow Menu.
Open overflow menu and click bookmarks button.


Actual results:

Firefox hangs.


Expected results:

Bookmarks menu is shown.

Comment 1

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

I'm unable to reproduce this. The "star" button can't be pinned to the overflow menu. Are you talking about the "star on top of a box" icon (the one that shows the bookmarks list when clicked on)?

Can you use mozregression to find when this broke? http://mozilla.github.io/mozregression/

Comment 2

[Winson Wen]

Yeah, I'm talking about the star on a box icon. For some reason, the issue doesn't appear with the default bookmarks, but it does when I import bookmarks from my main profile. Firefox also doesn't hang if I open the library > bookmarks menu in the ☰ menu before clicking the bookmarks button in the overflow menu.

mozregression-gui pointed to this build:

app_name: firefox
build_date: 2017-07-20 02:37:42.695000
build_file: C:\Users\Winson Wen\.mozilla\mozregression\persist\0e770947be4b--autoland--firefox-56.0a1.en-US.win64.zip
build_type: inbound
build_url: https://queue.taskcluster.net/v1/task/bYjucGpGQQefSyHEAmAE-Q/runs/0/artifacts/public%2Fbuild%2Ffirefox-56.0a1.en-US.win64.zip
changeset: 0e770947be4b90869917c6afc5c45690f041bdc5
pushlog_url: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=b626aca18e669be6957857b06779cd8b3bb8dbb7&tochange=0e770947be4b90869917c6afc5c45690f041bdc5
repo_name: autoland
repo_url: https://hg.mozilla.org/integration/autoland
task_id: bYjucGpGQQefSyHEAmAE-Q

Comment 3

[Winson Wen]

Attachment: bookmarks.json

So I managed to hang Firefox with this set of bookmarks. I think the hanging is related to the amount, since Firefox didn't hang when I tried a smaller amount.

Comment 4

[:Gijs (he/him)]

bug 1354086 was mostly styling changes, so I'm very impressed if that's the cause. I'll try to have to look at this in the near future.

Comment 5

[:Gijs (he/him)]

(In reply to winson.wen1 from comment #3)
> Created attachment 8888718 [details]
> bookmarks.json
> 
> So I managed to hang Firefox with this set of bookmarks. I think the hanging
> is related to the amount, since Firefox didn't hang when I tried a smaller
> amount.

So... if you import this set of bookmarks into a clean profile, you get a hang? I just tried to do exactly that (also on win10, on today's nightly, 07-21), and can't reproduce. I also can't reproduce with my default set of bookmarks. :zombie said he saw something similar... Do you have stylo turned on? Anything else that could be related here?

Comment 6

[:Gijs (he/him)]

I also tried duplicating the bookmarks on this bug so I had about 600 instead of 50, and I still didn't get a hang.

Comment 7

[:Gijs (he/him)]

So with some more help from zombie, I can reproduce. It seems to only happen the first time you open the menu once Firefox is up and running. Subsequent opens are fine.

https://perfht.ml/2tOO3QE

This seems to be a multi-second paint? I, uh, don't know what's causing that. Markus, am I reading this right? Any ideas who could look into this?

Comment 8

[Markus Stange [:mstange]]

It's waiting for the compositor process. And the profile doesn't contain any samples from the compositor process during that time. So maybe the compositor process hasn't started up yet? I don't know why it would be taking so long, though...

You can try to reproduce this with MOZ_PROFILER_STARTUP=1 (in order to work around the second part of bug 1382910), and see if you get samples from the compositor process that way.

Comment 9

[:Gijs (he/him)]

Attachment: Bigger bookmarks.json

:rhunt, Markus suggested I ping you re: this profile. It seems like there's something wrong with the compositor process, causing very long (30s; perceived - nothing is painting) hangs. Steps are roughly as follows (on win10):

1. clean profile
2. open the bookmarks manager / library (ctrl-shift-b)
3. click the import/backup button
4. restore > choose file...
5. select this json file
6. once done, right click the bookmarks menu button (bookmarks star over a 'tray'/box), click 'pin to overflow menu'
7. restart firefox
8. click the overflow button (chevron, far right, it should have animated in step 6)
9. click the bookmarks item

I created one profile that is linked in my earlier comment, and based on Markus' instruction, another one here: https://perfht.ml/2tOuEiN 

I would be much obliged if you can make heads/tails of it and/or redirect to an appropriate person. :-)

Comment 10

[Ryan Hunt [:rhunt] (on leave until early May)]

I was able to reproduce this. Thanks for the detailed steps!

Here's a profile of the issue with compositor process samples: https://perfht.ml/2vGW5xa

The compositor is running as fast as it can, and everything seems to be waiting on it.

Talking with Markus, it looks like the main thread may be getting into a loop of requesting composites. Possibly from resize events.

I'm looking into it more so I'll leave the needinfo flag.

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Could it just be the continuous appending of the bookmark DOM nodes? We don't really do any special treatment there, like using a fragment, collapsed or such.

Comment 12

[Ryan Hunt [:rhunt] (on leave until early May)]

> Could it just be the continuous appending of the bookmark DOM nodes? We don't really do any special
> treatment there, like using a fragment, collapsed or such.

It could be possibly. Would there be a simple way to batch appending these elements? To test whether that is the problem.

My suspicion is that this is a gfx/widget bug. I'm noticing the widget is getting invalidated frequently from resize requests. These invalidates are causing us to force a composite. But the resize requests seem to be bugged and generated when nothing has actually changed.

Still looking into this though.

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

To speed up insertions it should be possible to change the code here http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/browser/components/places/content/browserPlacesViews.js#283 and add to a documentFragment rather than directly adding to the popup.
Nobody ever looked at optimizing this part of the code, but it sounds feasible. It would require some refactoring of the code (like _insertNewItemToPopup should probably get a DOM node or documentFrament as input, rather than creating it internally, the consumer will build the appropriate thing).

Comment 15

[:Gijs (he/him)]

In the dupe, Asa is seeing basically the same symptoms with the library button's bookmarks view, but those symptoms don't reproduce in the hamburger menu (which comes with a library item builtin). I think this corroborates the suspicion in comment #12 that there's something in the overflow menu that triggers a bug in widget/gfx, rather than an issue with how frontend inserts nodes.

Comment 16

[:Gijs (he/him)]

Maybe totally unrelated, maybe not: in bug 1382680 comment 3, pchang says that OMTC is turned off for small popups. Maybe the overflow panel counts as a 'small' popup and the hamburger one as a 'big' popup, and that explains the difference, or something?

Comment 17

[Ryan Hunt [:rhunt] (on leave until early May)]

In nsMenuPopupFrame we set the max and min size on the widget directly [1], then we request a resize on the view [2]. Resizing the view happens in a ViewManagerFlush, where it checks if the client bounds of the widget match the requested size of the widget. If they don't, it resizes the widget and then invalidates it, forcing the compositor to do a composite.

In this case, the popup is trying to set the size of the widget to be smaller than the minimum size of the widget. The widget clamps the size in this case to the minimum. So every ViewManagerFlush, the widget tries to resize the widget to it's desired size, but it can't because it will be clamped. It then forces a composite by invalidating the widget. These ViewManagerFlushes seems to happen often enough that it spams the compositor.

I have a fix for this in nsMenuPopupFrame that clamps the size of view that we request to avoid this. It seems to resolve this problem from my testing.

Try run here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d26b1db5910e4ac9a78679e54470026ac35a5a3

[1] http://searchfox.org/mozilla-central/rev/8a61c71153a79cda2e1ae7d477564347c607cc5f/layout/xul/nsMenuPopupFrame.cpp#558
[2] http://searchfox.org/mozilla-central/rev/8a61c71153a79cda2e1ae7d477564347c607cc5f/layout/xul/nsMenuPopupFrame.cpp#566

Comment 18

[Ryan Hunt [:rhunt] (on leave until early May)]

I forgot to add, if someone else could test the build from try and see if it resolves the problem, I'll post it for review.

Comment 19

[:Gijs (he/him)]

(In reply to Ryan Hunt [:rhunt] from comment #18)
> I forgot to add, if someone else could test the build from try and see if it
> resolves the problem, I'll post it for review.

Looks much better to me for both the bookmarks menu and the library's bookmarks view, yep!

Comment 20

[Ryan Hunt [:rhunt] (on leave until early May)]

Attachment: popup-resize.patch

Comment 21

[Markus Stange [:mstange]]

Comment on attachment 8890908 [details] [diff] [review]
popup-resize.patch

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

Seems reasonable to me. Let's land this now, and if Neil has other comments once he comes back, we can address them at that point.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a37abf878197
Respect widget min and max size in nsMenuPopupFrame to prevent excessive compositing. r=mstange

Comment 23

[Milan Sreckovic [:milan] (needinfo for best results)]

I did hit this once today as well; let's see if this patch takes care of it.

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a37abf878197